Message-ID: <20190326162252.GB4958@nanopsycho.orion>
Date:   Tue, 26 Mar 2019 17:22:52 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Linville <linville@...driver.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v5 06/22] ethtool: helper functions for netlink
 interface

Tue, Mar 26, 2019 at 03:19:55PM CET, mkubecek@...e.cz wrote:
>On Tue, Mar 26, 2019 at 01:38:15PM +0100, Jiri Pirko wrote:
>> Mon, Mar 25, 2019 at 06:08:12PM CET, mkubecek@...e.cz wrote:
>> >+/* device specification */
>> >+
>> >+enum {
>> >+	ETHA_DEV_UNSPEC,
>> 
>> ETHNL/ETHA looks odd.
>> I would prefer prefix "ETHTOOL_* for everything in uapi:
>> ETHTOOL_CMD_* for commands.
>> ETHTOOL_ATTR_* for attributes.
>> 
>> Looks much nicer.
>
>Even with ETHA_ prefix, there are places where having to fit into
>80 columns makes the code hard to read. ETHTOOL_ATTR_ would make it
>even harder.

So cut the line. UAPI consistency is more important than to fit on a
80cols line.

ETH/ETHA is wrong prefix. This is "ETHTOOL". I can live with ETHTOOL_A_*
for attributes.


>
>> >+static const struct nla_policy dev_policy[ETHA_DEV_MAX + 1] = {
>> 
>> ethnl_dev_policy. Please maintain the namespace if possible.
>> "dev_policy" looks way too generic if anyone sees it in the code.
>
>OK
>> >+	[ETHA_DEV_UNSPEC]	= { .type = NLA_REJECT },
>> >+	[ETHA_DEV_INDEX]	= { .type = NLA_U32 },
>> >+	[ETHA_DEV_NAME]		= { .type = NLA_NUL_STRING,
>> >+				    .len = IFNAMSIZ - 1 },
>> >+};
>> >+
>> >+/**
>> >+ * ethnl_dev_get() - get device identified by nested attribute
>> >+ * @info: genetlink info (also used for extack error reporting)
>> >+ * @nest: nest attribute with device identification
>> >+ *
>> >+ * Finds the network device identified by ETHA_DEV_INDEX (ifindex) or
>> >+ * ETHA_DEV_NAME (name) attributes in a nested attribute @nest. If both
>> >+ * are supplied, they must identify the same device. If successful, takes
>> >+ * a reference to the device which is to be released by caller.
>> >+ *
>> >+ * Return: pointer to the device if successful, ERR_PTR(err) on error
>> >+ */
>> >+struct net_device *ethnl_dev_get(struct genl_info *info, struct nlattr *nest)
>> 
>> I wonder, why you have this in nested attr? Aren't ifindex/ifname always
>> used as a handle for commands/notifications? Would be just in toplevel
>> (see devlink/nl80211)
>
>It makes things only slightly easier now but I would like to make the
>API as future proof as possible. If something needs to be added later
>(e.g. netns id) or something else changes (e.g. 64-bit ifindex), only
>one enum and these two helpers would need to be adjusted.

You can still have the helper without the nest. So if you don't nest
this, everything is the same, both helper and extra attr in enum.


>
>> >+	if (!nest) {
>> >+		ETHNL_SET_ERRMSG(info,
>> >+				 "mandatory device identification missing");
>> 
>> No need to wrap.
>
>OK
>
>> >+	ret = nla_parse_nested_strict(tb, ETHA_DEV_MAX, nest, dev_policy,
>> >+				      info->extack);
>> >+	if (ret < 0)
>> 
>> "if (ret)" is enough. "Returns 0 on success or a negative error code."
>
>OK
>
>> >+			if (strncmp(dev->name, nl_name, IFNAMSIZ)) {
>> >+				dev_put(dev);
>> >+				ETHNL_SET_ERRMSG(info,
>> >+						 "ifindex and ifname do not match");
>> 
>> No need to wrap.
>
>OK
>
>> >+/**
>> >+ * ethnl_fill_dev() - Put device identification nest into a message
>> >+ * @msg:      skb with the message
>> >+ * @dev:      network device to describe
>> >+ * @attrtype: attribute type to use for the nest
>> >+ *
>> >+ * Create a nested attribute with attributes describing given network device.
>> >+ * Clean up on error.
>> >+ *
>> >+ * Return: 0 on success, error value (-EMSGSIZE only) on error
>> >+ */
>> >+int ethnl_fill_dev(struct sk_buff *msg, struct net_device *dev, u16 attrtype)
>> >+{
>> >+	struct nlattr *nest;
>> >+	int ret = -EMSGSIZE;
>> >+
>> >+	nest = ethnl_nest_start(msg, attrtype);
>> >+	if (!nest)
>> >+		return -EMSGSIZE;
>> >+
>> >+	if (nla_put_u32(msg, ETHA_DEV_INDEX, (u32)dev->ifindex))
>> >+		goto err;
>> >+	if (nla_put_string(msg, ETHA_DEV_NAME, dev->name))
>> >+		goto err;
>> >+
>> >+	nla_nest_end(msg, nest);
>> >+	return 0;
>> >+err:
>> Usually this label is called "nla_put_failure". Same for the rest.
>> "err" could be confused with error return variable.
>> 
>> have "ret = -EMSGSIZE" here. Easier to follow the error path.
>
>OK
>
>> >+	nla_nest_cancel(msg, nest);
>> >+	return ret;
>> >+}
>> >+
>> >+/**
>> >+ * ethnl_reply_init() - Create skb for a reply and fill device identification
>> >+ * @payload: payload length (without netlink and genetlink header)
>> >+ * @dev:     device the reply is about (may be null)
>> >+ * @cmd:     ETHNL_CMD_* command for reply
>> >+ * @info:    genetlink info of the received packet we respond to
>> >+ * @ehdrp:   place to store payload pointer returned by genlmsg_new()
>> >+ *
>> >+ * Return: pointer to allocated skb on success, NULL on error
>> >+ */
>> >+struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd,
>> >+				 u16 dev_attrtype, struct genl_info *info,
>> >+				 void **ehdrp)
>> >+{
>> >+	struct sk_buff *rskb;
>> 
>> Could be just "skb", or "msg". You have "msg" in ethnl_fill_dev().
>> Please have it consistent.
>
>OK
>
>> >+	rskb = genlmsg_new(payload, GFP_KERNEL);
>> >+	if (!rskb) {
>> >+		ETHNL_SET_ERRMSG(info,
>> >+				 "failed to allocate reply message");
>> 
>> No need to wrap.
>
>OK
>
>> >+		int ret = ethnl_fill_dev(rskb, dev, dev_attrtype);
>> >+
>> >+		if (ret < 0)
>> 
>> "if (ret)" is enough.
>
>OK
>
>> >+
>> >+#define ETHNL_SET_ERRMSG(info, msg) \
>> >+	do { if (info) GENL_SET_ERR_MSG(info, msg); } while (0)
>> 
>> Why do you need this macro? Can info be null?
>
>The same functions are used for generating replies to GET requests (info
>is not null) and notifications (info is null, no extack).

So again, could this be generic? GENL_SET_ERR_MSG() could accept null
and ignore.


>
>> In general, macros like this should be avoided.
>
>At the expense of repeating the same patterns. In this case it would be
>acceptable but it would mean one more indentation level for lines with
>the error/warning messages.

No if you move the check to lower layer.


>
>> >+static inline int ethnl_str_size(const char *s)
>> >+{
>> >+	return nla_total_size(strlen(s) + 1);
>> 
>> This looks like more generic helper, not tight to ethtool.
>
>OK
>
>> >+static inline int ethnl_str_ifne_size(const char *s)
>> 
>> Eh? "ifne"? What is this good for?
>
>It's for "if not empty". Structures like ethtool_drvinfo use fixed size
>char arrays for strings where empty string means the information is not
>available. So if the string is empty, netlink attribute is omitted.
>
>> >+{
>> >+	return s[0] ? ethnl_str_size(s) : 0;
>> >+}
>> >+
>> >+static inline int ethnl_put_str_ifne(struct sk_buff *skb, int attrtype,
>> >+				     const char *s)
>> >+{
>> >+	if (!s[0])
>> >+		return 0;
>> >+	return nla_put_string(skb, attrtype, s);
>> 
>> I don't like helpers like this. Do the check in the caller and put or
>> not put the string there. It's a single if.
>
>OK
>
>
>> >+static inline struct nlattr *ethnl_nest_start(struct sk_buff *skb,
>> >+					      int attrtype)
>> >+{
>> >+	return nla_nest_start(skb, attrtype | NLA_F_NESTED);
>> 
>> Please use nla_nest_start directly and avoid helpers like this.
>
>OK. It's one more thing to check (and forget) in many places, though.
>
>> >+static inline int ethnlmsg_parse(const struct nlmsghdr *nlh,
>> >+				 struct nlattr *tb[], int maxtype,
>> >+				 const struct nla_policy *policy,
>> >+				 struct genl_info *info)
>> >+{
>> >+	return nlmsg_parse_strict(nlh, GENL_HDRLEN, tb, maxtype, policy,
>> >+				  info ? info->extack : NULL);
>> 
>> Same thing, please use nlmsg_parse_strict directly.
>
>OK
>
>> >+/* ethnl_update_* return true if the value is changed */
>> >+static inline bool ethnl_update_u32(u32 *dst, struct nlattr *attr)
>> >+{
>> >+	u32 val;
>> >+
>> >+	if (!attr)
>> >+		return false;
>> >+	val = nla_get_u32(attr);
>> >+	if (*dst == val)
>> >+		return false;
>> >+
>> >+	*dst = val;
>> >+	return true;
>> >+}
>> >+
>> >+static inline bool ethnl_update_u8(u8 *dst, struct nlattr *attr)
>> >+{
>> >+	u8 val;
>> >+
>> >+	if (!attr)
>> >+		return false;
>> >+	val = nla_get_u8(attr);
>> >+	if (*dst == val)
>> >+		return false;
>> >+
>> >+	*dst = val;
>> >+	return true;
>> >+}
>> >+
>> >+/* update u32 value used as bool from NLA_U8 */
>> >+static inline bool ethnl_update_bool32(u32 *dst, struct nlattr *attr)
>> >+{
>> >+	u8 val;
>> >+
>> >+	if (!attr)
>> >+		return false;
>> >+	val = !!nla_get_u8(attr);
>> >+	if (!!*dst == val)
>> >+		return false;
>> >+
>> >+	*dst = val;
>> >+	return true;
>> >+}
>> >+
>> >+static inline bool ethnl_update_binary(u8 *dst, unsigned int len,
>> >+				       struct nlattr *attr)
>> >+{
>> >+	if (!attr)
>> >+		return false;
>> >+	if (nla_len(attr) < len)
>> >+		len = nla_len(attr);
>> >+	if (!memcmp(dst, nla_data(attr), len))
>> >+		return false;
>> >+
>> >+	memcpy(dst, nla_data(attr), len);
>> >+	return true;
>> >+}
>> >+
>> >+static inline bool ethnl_update_bitfield32(u32 *dst, struct nlattr *attr)
>> >+{
>> >+	struct nla_bitfield32 change;
>> >+	u32 newval;
>> >+
>> >+	if (!attr)
>> >+		return false;
>> >+	change = nla_get_bitfield32(attr);
>> >+	newval = (*dst & ~change.selector) | (change.value & change.selector);
>> >+	if (*dst == newval)
>> >+		return false;
>> >+
>> >+	*dst = newval;
>> >+	return true;
>> >+}
>> 
>> I don't understand puspose of these "update" helper functions. Try to
>> avoid them. In general, please try to avoid wrappers around netlink api.
>
>The purpose is to update a value according to a request attribute (if
>present) and tell caller if the value was actually modified. They are
>used like e.g.
>
>	mod = false;
>	if (ethnl_update_u32(&data.rx_pending, tb[ETHA_RING_RX_PENDING]))
>		mod = true;
>	if (ethnl_update_u32(&data.rx_mini_pending,
>			     tb[ETHA_RING_RX_MINI_PENDING]))
>		mod = true;
>	if (ethnl_update_u32(&data.rx_jumbo_pending,
>			     tb[ETHA_RING_RX_JUMBO_PENDING]))
>		mod = true;
>	if (ethnl_update_u32(&data.tx_pending, tb[ETHA_RING_TX_PENDING]))
>		mod = true;
>
>	if (!mod)
>		return 0;
>	/* call ethtool_ops handler and send notification */
>
>so that we can omit calling the handler and sending a notification if
>the request was no-op. Expanding the helpers here would make the code
>needlessly complicated and error prone.

Got it. Makes sense.


>
>> >+static inline void warn_partial_info(struct genl_info *info)
>> >+{
>> >+	ETHNL_SET_ERRMSG(info, "not all requested data could be retrieved");
>> >+}
>> >+
>> >+/* Check user privileges explicitly to allow finer access control based on
>> >+ * context of the request or hiding part of the information from unprivileged
>> >+ * users
>> >+ */
>> >+static inline bool ethnl_is_privileged(struct sk_buff *skb)
>> >+{
>> >+	struct net *net = sock_net(skb->sk);
>> >+
>> >+	return netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN);
>> 
>> Same here.
>
>It's still the same, helpers allow replacing repeating code patterns
>with just saying what you are doing. But this one is called only from
>one place so I can live without it.

It is always a tradeoff. We have to be careful not to abuse it.


>
>> >+}
>> >+
>> >+/* total size of ETHA_*_DEV nested attribute; this is an upper estimate so that
>> >+ * we do not need to hold RTNL longer than necessary to prevent rename between
>> >+ * estimating the size and composing the message
>> >+ */
>> >+static inline unsigned int dev_ident_size(void)
>> 
>> You are missing "ethnl_" prefix.
>
>OK
>
>Michal

Powered by blists - more mailing lists