diff mbox series

[RFC,1/2] net: netlink: add nla_get_*_default() accessors

Message ID 20241024131807.0a6c07355832.I3df6aac71d38a5baa1c0a03d0c7e82d4395c030e@changeid
State New
Headers show
Series [RFC,1/2] net: netlink: add nla_get_*_default() accessors | expand

Commit Message

Johannes Berg Oct. 24, 2024, 11:18 a.m. UTC 
From: Johannes Berg <johannes.berg@intel.com>

There are quite a number of places that use patterns
such as

  if (attr)
     val = nla_get_u16(attr);
  else
     val = DEFAULT;

Add nla_get_u16_default() and friends like that to
not have to type this out all the time.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/netlink.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Johannes Berg Oct. 24, 2024, 11:29 a.m. UTC | #1 
On Thu, 2024-10-24 at 13:18 +0200, Johannes Berg wrote:
> 
> +MAKE_NLA_GET_DEFAULT(u8, nla_get_u8);
> +MAKE_NLA_GET_DEFAULT(u16, nla_get_u16);
> +MAKE_NLA_GET_DEFAULT(u32, nla_get_u32);
> +MAKE_NLA_GET_DEFAULT(u64, nla_get_u64);
> +MAKE_NLA_GET_DEFAULT(unsigned long, nla_get_msecs);
> +MAKE_NLA_GET_DEFAULT(s8, nla_get_s8);
> +MAKE_NLA_GET_DEFAULT(s16, nla_get_s16);
> +MAKE_NLA_GET_DEFAULT(s32, nla_get_s32);
> +MAKE_NLA_GET_DEFAULT(s64, nla_get_s64);
> +MAKE_NLA_GET_DEFAULT(s16, nla_get_le16);
> +MAKE_NLA_GET_DEFAULT(s32, nla_get_le32);
> +MAKE_NLA_GET_DEFAULT(s64, nla_get_le64);
> +MAKE_NLA_GET_DEFAULT(s16, nla_get_be16);
> +MAKE_NLA_GET_DEFAULT(s32, nla_get_be32);
> +MAKE_NLA_GET_DEFAULT(s64, nla_get_be64);
> 

I obviously messed that up completely, but you get the point ...

johannes
Johannes Berg Oct. 24, 2024, 12:11 p.m. UTC | #2 
On Thu, 2024-10-24 at 13:18 +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> This is mostly to illustrate, done with the following spatch:
> 

And we can extend that and get bunch more:

@@
expression attr, def;
expression val;
identifier fn =~ "^nla_get_.*";
fresh identifier dfn = fn ## "_default";
@@
(
-if (attr)
-  val = fn(attr);
-else
-  val = def;
+val = dfn(attr, def);
|
-if (!attr)
-  val = def;
-else
-  val = fn(attr);
+val = dfn(attr, def);
|
-val = def;
... where != val;
-if (attr)
-  val = fn(attr);
+val = dfn(attr, def);
|
-if (!attr)
-  return def;
-return fn(attr);
+return dfn(attr, def);
)


(also just running it multiple times finds more instances?!)


johannes
Johannes Berg Oct. 24, 2024, 1:41 p.m. UTC | #3 
Also just realized that some of the conversions are wrong, e.g.

> @@ -7378,17 +7375,11 @@ static int nl80211_set_station(struct sk_buff *skb, struct genl_info *info)
>  	if (info->attrs[NL80211_ATTR_VLAN_ID])
>  		params.vlan_id = nla_get_u16(info->attrs[NL80211_ATTR_VLAN_ID]);
>  
> -	if (info->attrs[NL80211_ATTR_STA_LISTEN_INTERVAL])
> -		params.listen_interval =
> -		     nla_get_u16(info->attrs[NL80211_ATTR_STA_LISTEN_INTERVAL]);
> -	else
> -		params.listen_interval = -1;
> +	params.listen_interval = nla_get_u16_default(info->attrs[NL80211_ATTR_STA_LISTEN_INTERVAL],
> +						     -1);


this one clearly is.

So need to be more careful with that :)

But at least the spatch gives some candidates.

johannes
Sabrina Dubroca Oct. 24, 2024, 2:28 p.m. UTC | #4 
2024-10-24, 14:11:05 +0200, Johannes Berg wrote:
> On Thu, 2024-10-24 at 13:18 +0200, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > This is mostly to illustrate, done with the following spatch:
> > 
> 
> And we can extend that and get bunch more:
> 
> @@
> expression attr, def;
> expression val;
> identifier fn =~ "^nla_get_.*";
> fresh identifier dfn = fn ## "_default";
> @@
> (
> -if (attr)
> -  val = fn(attr);
> -else
> -  val = def;
> +val = dfn(attr, def);
> |
> -if (!attr)
> -  val = def;
> -else
> -  val = fn(attr);
> +val = dfn(attr, def);
> |
> -val = def;
> ... where != val;
> -if (attr)
> -  val = fn(attr);
> +val = dfn(attr, def);
> |
> -if (!attr)
> -  return def;
> -return fn(attr);
> +return dfn(attr, def);
> )

Not really familiar with spatch, but I'm guessing this won't cover:
    val = attr ? getter(attr) : default;

See macsec_validate_attr in drivers/net/macsec.c for some
examples. There are also some cases where we have "if (data &&
data[IFLA_MACSEC_*])" guarding the attribute fetch
(drivers/net/macvlan.c does that too), but I guess you can't really
cover that without adding some kind of "default_with_cond" helpers.
Johannes Berg Oct. 24, 2024, 2:40 p.m. UTC | #5 
On Thu, 2024-10-24 at 16:31 +0200, Johannes Berg wrote:
> On Thu, 2024-10-24 at 16:28 +0200, Sabrina Dubroca wrote:
> > 
> > 
> > Not really familiar with spatch, but I'm guessing this won't cover:
> >     val = attr ? getter(attr) : default;
> 
> True, we could add
> 
> -val = attr ? fn(attr) : def;
> +val = dfn(attr);
> 

There are about 50 of those :) thanks!

johannes
Johannes Berg Oct. 24, 2024, 2:52 p.m. UTC | #6 
On Thu, 2024-10-24 at 16:48 +0200, Sabrina Dubroca wrote:
> 
> If nla_get_*_default was a macro (generating an "attr ? getter :
> default" expression) you wouldn't have that problem I think, 

Hmm. Perhaps. In the conditional operator (?:) they're subject to
integer promotion though, I wonder if that could cause some subtle issue
too especially if nla_get_u*() is used with signed variables?

> but you
> couldn't nicely generate all the helpers with MAKE_NLA_GET_DEFAULT
> anymore.

Right, that too.

I think it's probably better to just review them, and only commit the
obvious ones originally?

johannes
Johannes Berg Oct. 24, 2024, 3:29 p.m. UTC | #7 
On Thu, 2024-10-24 at 17:17 +0200, Sabrina Dubroca wrote:
> 2024-10-24, 16:52:00 +0200, Johannes Berg wrote:
> > On Thu, 2024-10-24 at 16:48 +0200, Sabrina Dubroca wrote:
> > > 
> > > If nla_get_*_default was a macro (generating an "attr ? getter :
> > > default" expression) you wouldn't have that problem I think, 
> > 
> > Hmm. Perhaps. In the conditional operator (?:) they're subject to
> > integer promotion though
> 
> Is it?
> 
>     #define nla_get_u16_default(attr, d) (attr ? nla_get_u16(attr) : d)
>     int v = nla_get_u16_default(NULL, -1);
> 
> seems to put the correct value into v.
> (but -ENOFOOD and -ELOWCOFFEE here, so I don't trust this quick test
> much :))

I'm probably -ELOWBLOODSUGAR right now ;-)

But yeah, I couldn't yet construct a scenario where it mattered, but I'
also couldn't convince myself that there isn't one. Maybe too much
inspired by the enum compare warnings:

https://lore.kernel.org/linux-wireless/20241018151841.3821671-1-arnd@kernel.org/


> > , I wonder if that could cause some subtle issue
> > too especially if nla_get_u*() is used with signed variables?
> 
> The issue in that example is pretty subtle and I'm fairly sure people
> are going to mess up :/

I didn't think it was that bad, but it's well possible that my
calibration for "subtle" is way off ;-)

> But I'm not attached to that macro I just suggested, it's just a
> thought.

Sure.

> > > but you
> > > couldn't nicely generate all the helpers with MAKE_NLA_GET_DEFAULT
> > > anymore.
> > 
> > Right, that too.
> > 
> > I think it's probably better to just review them, and only commit the
> > obvious ones originally?
> 
> Well, this one looked reasonable too. I'm not convinced reviewers are
> going to catch those problems. Or authors of new code using those
> _default helpers from the start.

Fair point. I didn't think it was that bad, in fact there were some I
was far less sure about, say

> +	request->page = nla_get_u8_default(info->attrs[NL802154_ATTR_PAGE],
> +					   wpan_phy->current_page);

which really depends on the types of 'page' and 'current_page'...


I think for most cases it's probably still worth doing, and I wouldn't
be _too_ concerned about the type issues here, most places are just
using zero or a small constant as the default anyway.

Even nla_get_XX_or_zero() would be a win, but that seemed too special
...

johannes
diff mbox series

Patch

diff --git a/include/net/netlink.h b/include/net/netlink.h
index db6af207287c..b15bd0437945 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -142,6 +142,8 @@ 
  *   nla_get_flag(nla)			return 1 if flag is true
  *   nla_get_msecs(nla)			get payload for a msecs attribute
  *
+ *   The same functions also exist with _default().
+ *
  * Attribute Misc:
  *   nla_memcpy(dest, nla, count)	copy attribute into memory
  *   nla_memcmp(nla, data, size)	compare attribute with memory area
@@ -1867,6 +1869,31 @@  static inline unsigned long nla_get_msecs(const struct nlattr *nla)
 	return msecs_to_jiffies((unsigned long) msecs);
 }
 
+#define MAKE_NLA_GET_DEFAULT(tp, fn)			\
+static inline tp fn##_default(const struct nlattr *nla,	\
+			      tp defvalue)		\
+{							\
+	if (!nla)					\
+		return defvalue;			\
+	return n(nla);					\
+}
+
+MAKE_NLA_GET_DEFAULT(u8, nla_get_u8);
+MAKE_NLA_GET_DEFAULT(u16, nla_get_u16);
+MAKE_NLA_GET_DEFAULT(u32, nla_get_u32);
+MAKE_NLA_GET_DEFAULT(u64, nla_get_u64);
+MAKE_NLA_GET_DEFAULT(unsigned long, nla_get_msecs);
+MAKE_NLA_GET_DEFAULT(s8, nla_get_s8);
+MAKE_NLA_GET_DEFAULT(s16, nla_get_s16);
+MAKE_NLA_GET_DEFAULT(s32, nla_get_s32);
+MAKE_NLA_GET_DEFAULT(s64, nla_get_s64);
+MAKE_NLA_GET_DEFAULT(s16, nla_get_le16);
+MAKE_NLA_GET_DEFAULT(s32, nla_get_le32);
+MAKE_NLA_GET_DEFAULT(s64, nla_get_le64);
+MAKE_NLA_GET_DEFAULT(s16, nla_get_be16);
+MAKE_NLA_GET_DEFAULT(s32, nla_get_be32);
+MAKE_NLA_GET_DEFAULT(s64, nla_get_be64);
+
 /**
  * nla_get_in_addr - return payload of IPv4 address attribute
  * @nla: IPv4 address netlink attribute