[PATCH net-next 00/15] tools: ynl: stop using libmnl

[PATCH net-next 00/15] tools: ynl: stop using libmnl

[Jakub Kicinski]

There is no strong reason to stop using libmnl in ynl but there
are a few small ones which add up.

First, we do much more advanced netlink level parsing than libmnl
in ynl so it's hard to say that libmnl abstracts much from us.
The fact that this series, removing the libmnl dependency, only
adds <300 LoC shows that code savings aren't huge.
OTOH when new types are added (e.g. auto-int) we need to add
compatibility to deal with older version of libmnl (in fact,
even tho patches have been sent months ago, auto-ints are still
not supported in libmnl.git).

Second, the dependency makes ynl less self contained, and harder
to vendor in. Whether vendoring libraries into projects is a good
idea is a separate discussion, nonetheless, people want to do it.

Third, there are small annoyances with the libmnl APIs which
are hard to fix in backward-compatible ways.

All in all, libmnl is a great library, but with all the code
generation and structured parsing, ynl is better served by going
its own way.

Jakub Kicinski (15):
  tools: ynl: give up on libmnl for auto-ints
  tools: ynl: create local attribute helpers
  tools: ynl: create local for_each helpers
  tools: ynl: create local nlmsg access helpers
  tools: ynl: create local ARRAY_SIZE() helper
  tools: ynl: make yarg the first member of struct ynl_dump_state
  tools: ynl-gen: remove unused parse code
  tools: ynl: wrap recv() + mnl_cb_run2() into a single helper
  tools: ynl: use ynl_sock_read_msgs() for ACK handling
  tools: ynl: stop using mnl_cb_run2()
  tools: ynl: switch away from mnl_cb_t
  tools: ynl: switch away from MNL_CB_*
  tools: ynl: stop using mnl socket helpers
  tools: ynl: remove the libmnl dependency
  tools: ynl: use MSG_DONTWAIT for getting notifications

 tools/net/ynl/lib/ynl-priv.h   | 352 ++++++++++++++++++++++++++++---
 tools/net/ynl/lib/ynl.c        | 368 +++++++++++++++++----------------
 tools/net/ynl/lib/ynl.h        |   3 +-
 tools/net/ynl/samples/Makefile |   2 +-
 tools/net/ynl/ynl-gen-c.py     | 108 ++++------
 5 files changed, 565 insertions(+), 268 deletions(-)

-- 
2.43.2

[PATCH net-next 01/15] tools: ynl: give up on libmnl for auto-ints

[Jakub Kicinski]

The temporary auto-int helpers are not really correct.
We can't treat signed and unsigned ints the same when
determining whether we need full 8B. I realized this
before sending the patch to add support in libmnl.
Unfortunately, that patch has not been merged,
so time to fix our local helpers. Use the mnl* name
for now, subsequent patches will address that.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h | 45 ++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 7491da8e7555..eaa0d432366c 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -125,20 +125,47 @@ int ynl_exec_dump(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 void ynl_error_unknown_notification(struct ynl_sock *ys, __u8 cmd);
 int ynl_error_parse(struct ynl_parse_arg *yarg, const char *msg);
 
-#ifndef MNL_HAS_AUTO_SCALARS
-static inline uint64_t mnl_attr_get_uint(const struct nlattr *attr)
+/* Attribute helpers */
+
+static inline __u64 mnl_attr_get_uint(const struct nlattr *attr)
 {
-	if (mnl_attr_get_payload_len(attr) == 4)
+	switch (mnl_attr_get_payload_len(attr)) {
+	case 4:
 		return mnl_attr_get_u32(attr);
-	return mnl_attr_get_u64(attr);
+	case 8:
+		return mnl_attr_get_u64(attr);
+	default:
+		return 0;
+	}
+}
+
+static inline __s64 mnl_attr_get_sint(const struct nlattr *attr)
+{
+	switch (mnl_attr_get_payload_len(attr)) {
+	case 4:
+		return mnl_attr_get_u32(attr);
+	case 8:
+		return mnl_attr_get_u64(attr);
+	default:
+		return 0;
+	}
 }
 
 static inline void
-mnl_attr_put_uint(struct nlmsghdr *nlh, uint16_t type, uint64_t data)
+mnl_attr_put_uint(struct nlmsghdr *nlh, __u16 type, __u64 data)
 {
-	if ((uint32_t)data == (uint64_t)data)
-		return mnl_attr_put_u32(nlh, type, data);
-	return mnl_attr_put_u64(nlh, type, data);
+	if ((__u32)data == (__u64)data)
+		mnl_attr_put_u32(nlh, type, data);
+	else
+		mnl_attr_put_u64(nlh, type, data);
+}
+
+static inline void
+mnl_attr_put_sint(struct nlmsghdr *nlh, __u16 type, __s64 data)
+{
+	if ((__s32)data == (__s64)data)
+		mnl_attr_put_u32(nlh, type, data);
+	else
+		mnl_attr_put_u64(nlh, type, data);
 }
 #endif
-#endif
-- 
2.43.2

Re: [PATCH net-next 01/15] tools: ynl: give up on libmnl for auto-ints

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> The temporary auto-int helpers are not really correct.
> We can't treat signed and unsigned ints the same when
> determining whether we need full 8B. I realized this
> before sending the patch to add support in libmnl.
> Unfortunately, that patch has not been merged,
> so time to fix our local helpers. Use the mnl* name
> for now, subsequent patches will address that.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>> ---
>  tools/net/ynl/lib/ynl-priv.h | 45 ++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
> index 7491da8e7555..eaa0d432366c 100644
> --- a/tools/net/ynl/lib/ynl-priv.h
> +++ b/tools/net/ynl/lib/ynl-priv.h
> @@ -125,20 +125,47 @@ int ynl_exec_dump(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
>  void ynl_error_unknown_notification(struct ynl_sock *ys, __u8 cmd);
>  int ynl_error_parse(struct ynl_parse_arg *yarg, const char *msg);
>  
> -#ifndef MNL_HAS_AUTO_SCALARS
> -static inline uint64_t mnl_attr_get_uint(const struct nlattr *attr)
> +/* Attribute helpers */
> +
> +static inline __u64 mnl_attr_get_uint(const struct nlattr *attr)
>  {
> -	if (mnl_attr_get_payload_len(attr) == 4)
> +	switch (mnl_attr_get_payload_len(attr)) {
> +	case 4:
>  		return mnl_attr_get_u32(attr);
> -	return mnl_attr_get_u64(attr);
> +	case 8:
> +		return mnl_attr_get_u64(attr);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static inline __s64 mnl_attr_get_sint(const struct nlattr *attr)
> +{
> +	switch (mnl_attr_get_payload_len(attr)) {
> +	case 4:
> +		return mnl_attr_get_u32(attr);
> +	case 8:
> +		return mnl_attr_get_u64(attr);
> +	default:
> +		return 0;
> +	}
>  }
mnl_attr_get_uint() and mnl_attr_get_sint() are identical. What about
#define mnl_attr_get_sint mnl_attr_get_uint
?

>  
>  static inline void
> -mnl_attr_put_uint(struct nlmsghdr *nlh, uint16_t type, uint64_t data)
> +mnl_attr_put_uint(struct nlmsghdr *nlh, __u16 type, __u64 data)
Is there a reason to switch from uint*_t to __u* types?

>  {
> -	if ((uint32_t)data == (uint64_t)data)
> -		return mnl_attr_put_u32(nlh, type, data);
> -	return mnl_attr_put_u64(nlh, type, data);
> +	if ((__u32)data == (__u64)data)
> +		mnl_attr_put_u32(nlh, type, data);
> +	else
> +		mnl_attr_put_u64(nlh, type, data);
> +}
> +
> +static inline void
> +mnl_attr_put_sint(struct nlmsghdr *nlh, __u16 type, __s64 data)
> +{
> +	if ((__s32)data == (__s64)data)
> +		mnl_attr_put_u32(nlh, type, data);
> +	else
> +		mnl_attr_put_u64(nlh, type, data);
>  }
>  #endif
> -#endif

Re: [PATCH net-next 01/15] tools: ynl: give up on libmnl for auto-ints

[Jakub Kicinski]

On Fri, 23 Feb 2024 14:51:12 +0100 Nicolas Dichtel wrote:
> > +static inline __s64 mnl_attr_get_sint(const struct nlattr *attr)
> > +{
> > +	switch (mnl_attr_get_payload_len(attr)) {
> > +	case 4:
> > +		return mnl_attr_get_u32(attr);
> > +	case 8:
> > +		return mnl_attr_get_u64(attr);
> > +	default:
> > +		return 0;
> > +	}
> >  }  
> mnl_attr_get_uint() and mnl_attr_get_sint() are identical. What about
> #define mnl_attr_get_sint mnl_attr_get_uint
> ?

I like to have the helpers written out 🤷️
I really hate the *_encode_bits macros in the kernel, maybe I'm
swinging to hard in the opposite direction, but let me swing! :)

> >  static inline void
> > -mnl_attr_put_uint(struct nlmsghdr *nlh, uint16_t type, uint64_t data)
> > +mnl_attr_put_uint(struct nlmsghdr *nlh, __u16 type, __u64 data)  
> Is there a reason to switch from uint*_t to __u* types?

YNL uses the kernel __{s,u}{8,16,32,64} types everywhere.
These were an exception because they were following libmnl's types.

Re: [PATCH net-next 01/15] tools: ynl: give up on libmnl for auto-ints

[Nicolas Dichtel]

Le 23/02/2024 à 15:35, Jakub Kicinski a écrit :
> On Fri, 23 Feb 2024 14:51:12 +0100 Nicolas Dichtel wrote:
>>> +static inline __s64 mnl_attr_get_sint(const struct nlattr *attr)
>>> +{
>>> +	switch (mnl_attr_get_payload_len(attr)) {
>>> +	case 4:
>>> +		return mnl_attr_get_u32(attr);
>>> +	case 8:
>>> +		return mnl_attr_get_u64(attr);
>>> +	default:
>>> +		return 0;
>>> +	}
>>>  }  
>> mnl_attr_get_uint() and mnl_attr_get_sint() are identical. What about
>> #define mnl_attr_get_sint mnl_attr_get_uint
>> ?
> 
> I like to have the helpers written out 🤷️
> I really hate the *_encode_bits macros in the kernel, maybe I'm
> swinging to hard in the opposite direction, but let me swing! :)
No problem :)
Anyway, after the patch #2, the code in different ;-)

> 
>>>  static inline void
>>> -mnl_attr_put_uint(struct nlmsghdr *nlh, uint16_t type, uint64_t data)
>>> +mnl_attr_put_uint(struct nlmsghdr *nlh, __u16 type, __u64 data)  
>> Is there a reason to switch from uint*_t to __u* types?
> 
> YNL uses the kernel __{s,u}{8,16,32,64} types everywhere.
> These were an exception because they were following libmnl's types.
Ok.

Re: [PATCH net-next 01/15] tools: ynl: give up on libmnl for auto-ints

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> The temporary auto-int helpers are not really correct.
> We can't treat signed and unsigned ints the same when
> determining whether we need full 8B. I realized this
> before sending the patch to add support in libmnl.
> Unfortunately, that patch has not been merged,
> so time to fix our local helpers. Use the mnl* name
> for now, subsequent patches will address that.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

[PATCH net-next 02/15] tools: ynl: create local attribute helpers

[Jakub Kicinski]

Don't use mnl attr helpers, we're trying to remove the libmnl
dependency. Create both signed and unsigned helpers, libmnl
had unsigned helpers, so code generator no longer needs
the mnl_type() hack.

The new helpers are written from first principles, but are
hopefully not too buggy.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h | 215 ++++++++++++++++++++++++++++++++---
 tools/net/ynl/lib/ynl.c      |  44 +++----
 tools/net/ynl/ynl-gen-c.py   |  59 ++++------
 3 files changed, 245 insertions(+), 73 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index eaa0d432366c..b5f99521fd8d 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -127,45 +127,232 @@ int ynl_error_parse(struct ynl_parse_arg *yarg, const char *msg);
 
 /* Attribute helpers */
 
-static inline __u64 mnl_attr_get_uint(const struct nlattr *attr)
+static inline void *ynl_nlmsg_end_addr(const struct nlmsghdr *nlh)
 {
-	switch (mnl_attr_get_payload_len(attr)) {
+	return (char *)nlh + nlh->nlmsg_len;
+}
+
+static inline unsigned int ynl_attr_type(const struct nlattr *attr)
+{
+	return attr->nla_type & NLA_TYPE_MASK;
+}
+
+static inline unsigned int ynl_attr_data_len(const struct nlattr *attr)
+{
+	return attr->nla_len - NLA_ALIGN(sizeof(struct nlattr));
+}
+
+static inline void *ynl_attr_data(const struct nlattr *attr)
+{
+	return (unsigned char *)attr + NLA_ALIGN(sizeof(struct nlattr));
+}
+
+static inline struct nlattr *
+ynl_attr_nest_start(struct nlmsghdr *nlh, unsigned int attr_type)
+{
+	struct nlattr *attr;
+
+	attr = ynl_nlmsg_end_addr(nlh);
+	attr->nla_type = attr_type | NLA_F_NESTED;
+	nlh->nlmsg_len += NLMSG_ALIGN(sizeof(struct nlattr));
+
+	return attr;
+}
+
+static inline void
+ynl_attr_nest_end(struct nlmsghdr *nlh, struct nlattr *attr)
+{
+	attr->nla_len = (char *)ynl_nlmsg_end_addr(nlh) - (char *)attr;
+}
+
+static inline void
+ynl_attr_put(struct nlmsghdr *nlh, unsigned int attr_type,
+	     const void *value, size_t size)
+{
+	struct nlattr *attr;
+
+	attr = ynl_nlmsg_end_addr(nlh);
+	attr->nla_type = attr_type;
+	attr->nla_len = NLA_ALIGN(sizeof(struct nlattr)) + size;
+
+	memcpy(ynl_attr_data(attr), value, size);
+
+	nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);
+}
+
+static inline void
+ynl_attr_put_strz(struct nlmsghdr *nlh, unsigned int attr_type, const char *str)
+{
+	struct nlattr *attr;
+	const char *end;
+
+	attr = ynl_nlmsg_end_addr(nlh);
+	attr->nla_type = attr_type;
+
+	end = stpcpy(ynl_attr_data(attr), str);
+	attr->nla_len =
+		NLA_ALIGN(sizeof(struct nlattr)) +
+		NLA_ALIGN(end - (char *)ynl_attr_data(attr));
+
+	nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);
+}
+
+static inline const char *ynl_attr_get_str(const struct nlattr *attr)
+{
+	return (const char *)(attr + 1);
+}
+
+static inline __s8 ynl_attr_get_s8(const struct nlattr *attr)
+{
+	__s8 tmp;
+
+	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
+	return tmp;
+}
+
+static inline __s16 ynl_attr_get_s16(const struct nlattr *attr)
+{
+	__s16 tmp;
+
+	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
+	return tmp;
+}
+
+static inline __s32 ynl_attr_get_s32(const struct nlattr *attr)
+{
+	__s32 tmp;
+
+	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
+	return tmp;
+}
+
+static inline __s64 ynl_attr_get_s64(const struct nlattr *attr)
+{
+	__s64 tmp;
+
+	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
+	return tmp;
+}
+
+static inline __u8 ynl_attr_get_u8(const struct nlattr *attr)
+{
+	__u8 tmp;
+
+	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
+	return tmp;
+}
+
+static inline __u16 ynl_attr_get_u16(const struct nlattr *attr)
+{
+	__u16 tmp;
+
+	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
+	return tmp;
+}
+
+static inline __u32 ynl_attr_get_u32(const struct nlattr *attr)
+{
+	__u32 tmp;
+
+	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
+	return tmp;
+}
+
+static inline __u64 ynl_attr_get_u64(const struct nlattr *attr)
+{
+	__u64 tmp;
+
+	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
+	return tmp;
+}
+
+static inline void
+ynl_attr_put_s8(struct nlmsghdr *nlh, unsigned int attr_type, __s8 value)
+{
+	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
+}
+
+static inline void
+ynl_attr_put_s16(struct nlmsghdr *nlh, unsigned int attr_type, __s16 value)
+{
+	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
+}
+
+static inline void
+ynl_attr_put_s32(struct nlmsghdr *nlh, unsigned int attr_type, __s32 value)
+{
+	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
+}
+
+static inline void
+ynl_attr_put_s64(struct nlmsghdr *nlh, unsigned int attr_type, __s64 value)
+{
+	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
+}
+
+static inline void
+ynl_attr_put_u8(struct nlmsghdr *nlh, unsigned int attr_type, __u8 value)
+{
+	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
+}
+
+static inline void
+ynl_attr_put_u16(struct nlmsghdr *nlh, unsigned int attr_type, __u16 value)
+{
+	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
+}
+
+static inline void
+ynl_attr_put_u32(struct nlmsghdr *nlh, unsigned int attr_type, __u32 value)
+{
+	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
+}
+
+static inline void
+ynl_attr_put_u64(struct nlmsghdr *nlh, unsigned int attr_type, __u64 value)
+{
+	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
+}
+
+static inline __u64 ynl_attr_get_uint(const struct nlattr *attr)
+{
+	switch (ynl_attr_data_len(attr)) {
 	case 4:
-		return mnl_attr_get_u32(attr);
+		return ynl_attr_get_u32(attr);
 	case 8:
-		return mnl_attr_get_u64(attr);
+		return ynl_attr_get_u64(attr);
 	default:
 		return 0;
 	}
 }
 
-static inline __s64 mnl_attr_get_sint(const struct nlattr *attr)
+static inline __s64 ynl_attr_get_sint(const struct nlattr *attr)
 {
-	switch (mnl_attr_get_payload_len(attr)) {
+	switch (ynl_attr_data_len(attr)) {
 	case 4:
-		return mnl_attr_get_u32(attr);
+		return ynl_attr_get_u32(attr);
 	case 8:
-		return mnl_attr_get_u64(attr);
+		return ynl_attr_get_u64(attr);
 	default:
 		return 0;
 	}
 }
 
 static inline void
-mnl_attr_put_uint(struct nlmsghdr *nlh, __u16 type, __u64 data)
+ynl_attr_put_uint(struct nlmsghdr *nlh, __u16 type, __u64 data)
 {
 	if ((__u32)data == (__u64)data)
-		mnl_attr_put_u32(nlh, type, data);
+		ynl_attr_put_u32(nlh, type, data);
 	else
-		mnl_attr_put_u64(nlh, type, data);
+		ynl_attr_put_u64(nlh, type, data);
 }
 
 static inline void
-mnl_attr_put_sint(struct nlmsghdr *nlh, __u16 type, __s64 data)
+ynl_attr_put_sint(struct nlmsghdr *nlh, __u16 type, __s64 data)
 {
 	if ((__s32)data == (__s64)data)
-		mnl_attr_put_u32(nlh, type, data);
+		ynl_attr_put_u32(nlh, type, data);
 	else
-		mnl_attr_put_u64(nlh, type, data);
+		ynl_attr_put_u64(nlh, type, data);
 }
 #endif
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 6e6d474c8366..f16297713c0e 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -94,7 +94,7 @@ ynl_err_walk(struct ynl_sock *ys, void *start, void *end, unsigned int off,
 
 	mnl_attr_for_each_payload(start, data_len) {
 		astart_off = (char *)attr - (char *)start;
-		aend_off = astart_off + mnl_attr_get_payload_len(attr);
+		aend_off = astart_off + ynl_attr_data_len(attr);
 		if (aend_off <= off)
 			continue;
 
@@ -106,7 +106,7 @@ ynl_err_walk(struct ynl_sock *ys, void *start, void *end, unsigned int off,
 
 	off -= astart_off;
 
-	type = mnl_attr_get_type(attr);
+	type = ynl_attr_type(attr);
 
 	if (ynl_err_walk_report_one(policy, type, str, str_sz, &n))
 		return n;
@@ -124,8 +124,8 @@ ynl_err_walk(struct ynl_sock *ys, void *start, void *end, unsigned int off,
 	}
 
 	off -= sizeof(struct nlattr);
-	start =  mnl_attr_get_payload(attr);
-	end = start + mnl_attr_get_payload_len(attr);
+	start =  ynl_attr_data(attr);
+	end = start + ynl_attr_data_len(attr);
 
 	return n + ynl_err_walk(ys, start, end, off, policy->table[type].nest,
 				&str[n], str_sz - n, nest_pol);
@@ -153,8 +153,8 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 	mnl_attr_for_each(attr, nlh, hlen) {
 		unsigned int len, type;
 
-		len = mnl_attr_get_payload_len(attr);
-		type = mnl_attr_get_type(attr);
+		len = ynl_attr_data_len(attr);
+		type = ynl_attr_type(attr);
 
 		if (type > NLMSGERR_ATTR_MAX)
 			continue;
@@ -169,7 +169,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 				return MNL_CB_ERROR;
 			break;
 		case NLMSGERR_ATTR_MSG:
-			str = mnl_attr_get_payload(attr);
+			str = ynl_attr_data(attr);
 			if (str[len - 1])
 				return MNL_CB_ERROR;
 			break;
@@ -185,7 +185,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 		unsigned int n, off;
 		void *start, *end;
 
-		ys->err.attr_offs = mnl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]);
+		ys->err.attr_offs = ynl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]);
 
 		n = snprintf(bad_attr, sizeof(bad_attr), "%sbad attribute: ",
 			     str ? " (" : "");
@@ -211,7 +211,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 		void *start, *end;
 		int n2;
 
-		type = mnl_attr_get_u32(tb[NLMSGERR_ATTR_MISS_TYPE]);
+		type = ynl_attr_get_u32(tb[NLMSGERR_ATTR_MISS_TYPE]);
 
 		n = snprintf(miss_attr, sizeof(miss_attr), "%smissing attribute: ",
 			     bad_attr[0] ? ", " : (str ? " (" : ""));
@@ -222,7 +222,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 
 		nest_pol = ys->req_policy;
 		if (tb[NLMSGERR_ATTR_MISS_NEST]) {
-			off = mnl_attr_get_u32(tb[NLMSGERR_ATTR_MISS_NEST]);
+			off = ynl_attr_get_u32(tb[NLMSGERR_ATTR_MISS_NEST]);
 			off -= sizeof(struct nlmsghdr);
 			off -= ys->family->hdr_len;
 
@@ -314,9 +314,9 @@ int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
 	unsigned int type, len;
 	unsigned char *data;
 
-	data = mnl_attr_get_payload(attr);
-	len = mnl_attr_get_payload_len(attr);
-	type = mnl_attr_get_type(attr);
+	data = ynl_attr_data(attr);
+	len = ynl_attr_data_len(attr);
+	type = ynl_attr_type(attr);
 	if (type > yarg->rsp_policy->max_attr) {
 		yerr(yarg->ys, YNL_ERROR_INTERNAL,
 		     "Internal error, validating unknown attribute");
@@ -514,11 +514,11 @@ ynl_get_family_info_mcast(struct ynl_sock *ys, const struct nlattr *mcasts)
 	i = 0;
 	mnl_attr_for_each_nested(entry, mcasts) {
 		mnl_attr_for_each_nested(attr, entry) {
-			if (mnl_attr_get_type(attr) == CTRL_ATTR_MCAST_GRP_ID)
-				ys->mcast_groups[i].id = mnl_attr_get_u32(attr);
-			if (mnl_attr_get_type(attr) == CTRL_ATTR_MCAST_GRP_NAME) {
+			if (ynl_attr_type(attr) == CTRL_ATTR_MCAST_GRP_ID)
+				ys->mcast_groups[i].id = ynl_attr_get_u32(attr);
+			if (ynl_attr_type(attr) == CTRL_ATTR_MCAST_GRP_NAME) {
 				strncpy(ys->mcast_groups[i].name,
-					mnl_attr_get_str(attr),
+					ynl_attr_get_str(attr),
 					GENL_NAMSIZ - 1);
 				ys->mcast_groups[i].name[GENL_NAMSIZ - 1] = 0;
 			}
@@ -536,19 +536,19 @@ static int ynl_get_family_info_cb(const struct nlmsghdr *nlh, void *data)
 	bool found_id = true;
 
 	mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
-		if (mnl_attr_get_type(attr) == CTRL_ATTR_MCAST_GROUPS)
+		if (ynl_attr_type(attr) == CTRL_ATTR_MCAST_GROUPS)
 			if (ynl_get_family_info_mcast(ys, attr))
 				return MNL_CB_ERROR;
 
-		if (mnl_attr_get_type(attr) != CTRL_ATTR_FAMILY_ID)
+		if (ynl_attr_type(attr) != CTRL_ATTR_FAMILY_ID)
 			continue;
 
-		if (mnl_attr_get_payload_len(attr) != sizeof(__u16)) {
+		if (ynl_attr_data_len(attr) != sizeof(__u16)) {
 			yerr(ys, YNL_ERROR_ATTR_INVALID, "Invalid family ID");
 			return MNL_CB_ERROR;
 		}
 
-		ys->family_id = mnl_attr_get_u16(attr);
+		ys->family_id = ynl_attr_get_u16(attr);
 		found_id = true;
 	}
 
@@ -566,7 +566,7 @@ static int ynl_sock_read_family(struct ynl_sock *ys, const char *family_name)
 	int err;
 
 	nlh = ynl_gemsg_start_req(ys, GENL_ID_CTRL, CTRL_CMD_GETFAMILY, 1);
-	mnl_attr_put_strz(nlh, CTRL_ATTR_FAMILY_NAME, family_name);
+	ynl_attr_put_strz(nlh, CTRL_ATTR_FAMILY_NAME, family_name);
 
 	err = mnl_socket_sendto(ys->sock, nlh, nlh->nlmsg_len);
 	if (err < 0) {
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 7fc1aa788f6f..4f19c959ee7e 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -168,15 +168,6 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         spec = self._attr_policy(policy)
         cw.p(f"\t[{self.enum_name}] = {spec},")
 
-    def _mnl_type(self):
-        # mnl does not have helpers for signed integer types
-        # turn signed type into unsigned
-        # this only makes sense for scalar types
-        t = self.type
-        if t[0] == 's':
-            t = 'u' + t[1:]
-        return t
-
     def _attr_typol(self):
         raise Exception(f"Type policy not implemented for class type {self.type}")
 
@@ -192,7 +183,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         ri.cw.p(f"{line};")
 
     def _attr_put_simple(self, ri, var, put_type):
-        line = f"mnl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name})"
+        line = f"ynl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name})"
         self._attr_put_line(ri, var, line)
 
     def attr_put(self, ri, var):
@@ -357,9 +348,6 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         else:
             self.type_name = '__' + self.type
 
-    def mnl_type(self):
-        return self._mnl_type()
-
     def _attr_policy(self, policy):
         if 'flags-mask' in self.checks or self.is_bitfield:
             if self.is_bitfield:
@@ -387,10 +375,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         return [f'{self.type_name} {self.c_name}{self.byte_order_comment}']
 
     def attr_put(self, ri, var):
-        self._attr_put_simple(ri, var, self.mnl_type())
+        self._attr_put_simple(ri, var, self.type)
 
     def _attr_get(self, ri, var):
-        return f"{var}->{self.c_name} = mnl_attr_get_{self.mnl_type()}(attr);", None, None
+        return f"{var}->{self.c_name} = ynl_attr_get_{self.type}(attr);", None, None
 
     def _setter_lines(self, ri, member, presence):
         return [f"{member} = {self.c_name};"]
@@ -404,7 +392,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         return '.type = YNL_PT_FLAG, '
 
     def attr_put(self, ri, var):
-        self._attr_put_line(ri, var, f"mnl_attr_put(nlh, {self.enum_name}, 0, NULL)")
+        self._attr_put_line(ri, var, f"ynl_attr_put(nlh, {self.enum_name}, NULL, 0)")
 
     def _attr_get(self, ri, var):
         return [], None, None
@@ -452,9 +440,9 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         len_mem = var + '->_present.' + self.c_name + '_len'
         return [f"{len_mem} = len;",
                 f"{var}->{self.c_name} = malloc(len + 1);",
-                f"memcpy({var}->{self.c_name}, mnl_attr_get_str(attr), len);",
+                f"memcpy({var}->{self.c_name}, ynl_attr_get_str(attr), len);",
                 f"{var}->{self.c_name}[len] = 0;"], \
-               ['len = strnlen(mnl_attr_get_str(attr), mnl_attr_get_payload_len(attr));'], \
+               ['len = strnlen(ynl_attr_get_str(attr), ynl_attr_data_len(attr));'], \
                ['unsigned int len;']
 
     def _setter_lines(self, ri, member, presence):
@@ -493,15 +481,15 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         return mem
 
     def attr_put(self, ri, var):
-        self._attr_put_line(ri, var, f"mnl_attr_put(nlh, {self.enum_name}, " +
-                            f"{var}->_present.{self.c_name}_len, {var}->{self.c_name})")
+        self._attr_put_line(ri, var, f"ynl_attr_put(nlh, {self.enum_name}, " +
+                            f"{var}->{self.c_name}, {var}->_present.{self.c_name}_len)")
 
     def _attr_get(self, ri, var):
         len_mem = var + '->_present.' + self.c_name + '_len'
         return [f"{len_mem} = len;",
                 f"{var}->{self.c_name} = malloc(len);",
-                f"memcpy({var}->{self.c_name}, mnl_attr_get_payload(attr), len);"], \
-               ['len = mnl_attr_get_payload_len(attr);'], \
+                f"memcpy({var}->{self.c_name}, ynl_attr_data(attr), len);"], \
+               ['len = ynl_attr_data_len(attr);'], \
                ['unsigned int len;']
 
     def _setter_lines(self, ri, member, presence):
@@ -526,11 +514,11 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         return f"NLA_POLICY_BITFIELD32({mask})"
 
     def attr_put(self, ri, var):
-        line = f"mnl_attr_put(nlh, {self.enum_name}, sizeof(struct nla_bitfield32), &{var}->{self.c_name})"
+        line = f"ynl_attr_put(nlh, {self.enum_name}, &{var}->{self.c_name}, sizeof(struct nla_bitfield32))"
         self._attr_put_line(ri, var, line)
 
     def _attr_get(self, ri, var):
-        return f"memcpy(&{var}->{self.c_name}, mnl_attr_get_payload(attr), sizeof(struct nla_bitfield32));", None, None
+        return f"memcpy(&{var}->{self.c_name}, ynl_attr_data(attr), sizeof(struct nla_bitfield32));", None, None
 
     def _setter_lines(self, ri, member, presence):
         return [f"memcpy(&{member}, {self.c_name}, sizeof(struct nla_bitfield32));"]
@@ -589,9 +577,6 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def presence_type(self):
         return 'count'
 
-    def mnl_type(self):
-        return self._mnl_type()
-
     def _complex_member_type(self, ri):
         if 'type' not in self.attr or self.attr['type'] == 'nest':
             return self.nested_struct_type
@@ -625,9 +610,9 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
 
     def attr_put(self, ri, var):
         if self.attr['type'] in scalars:
-            put_type = self.mnl_type()
+            put_type = self.type
             ri.cw.p(f"for (unsigned int i = 0; i < {var}->n_{self.c_name}; i++)")
-            ri.cw.p(f"mnl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name}[i]);")
+            ri.cw.p(f"ynl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name}[i]);")
         elif 'type' not in self.attr or self.attr['type'] == 'nest':
             ri.cw.p(f"for (unsigned int i = 0; i < {var}->n_{self.c_name}; i++)")
             self._attr_put_line(ri, var, f"{self.nested_render_name}_put(nlh, " +
@@ -690,8 +675,8 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
             local_vars += [f'__u32 {", ".join(tv_names)};']
             for level in self.attr["type-value"]:
                 level = c_lower(level)
-                get_lines += [f'attr_{level} = mnl_attr_get_payload({prev});']
-                get_lines += [f'{level} = mnl_attr_get_type(attr_{level});']
+                get_lines += [f'attr_{level} = ynl_attr_data({prev});']
+                get_lines += [f'{level} = ynl_attr_type(attr_{level});']
                 prev = 'attr_' + level
 
             tv_args = f", {', '.join(tv_names)}"
@@ -1612,12 +1597,12 @@ _C_KW = {
     ri.cw.block_start()
     ri.cw.write_func_lvar('struct nlattr *nest;')
 
-    ri.cw.p("nest = mnl_attr_nest_start(nlh, attr_type);")
+    ri.cw.p("nest = ynl_attr_nest_start(nlh, attr_type);")
 
     for _, arg in struct.member_list():
         arg.attr_put(ri, "obj")
 
-    ri.cw.p("mnl_attr_nest_end(nlh, nest);")
+    ri.cw.p("ynl_attr_nest_end(nlh, nest);")
 
     ri.cw.nl()
     ri.cw.p('return 0;')
@@ -1674,7 +1659,7 @@ _C_KW = {
 
     ri.cw.nl()
     ri.cw.block_start(line=iter_line)
-    ri.cw.p('unsigned int type = mnl_attr_get_type(attr);')
+    ri.cw.p('unsigned int type = ynl_attr_type(attr);')
     ri.cw.nl()
 
     first = True
@@ -1696,7 +1681,7 @@ _C_KW = {
         ri.cw.p(f"parg.rsp_policy = &{aspec.nested_render_name}_nest;")
         ri.cw.block_start(line=f"mnl_attr_for_each_nested(attr, attr_{aspec.c_name})")
         ri.cw.p(f"parg.data = &dst->{aspec.c_name}[i];")
-        ri.cw.p(f"if ({aspec.nested_render_name}_parse(&parg, attr, mnl_attr_get_type(attr)))")
+        ri.cw.p(f"if ({aspec.nested_render_name}_parse(&parg, attr, ynl_attr_type(attr)))")
         ri.cw.p('return MNL_CB_ERROR;')
         ri.cw.p('i++;')
         ri.cw.block_end()
@@ -1712,13 +1697,13 @@ _C_KW = {
         if 'nested-attributes' in aspec:
             ri.cw.p(f"parg.rsp_policy = &{aspec.nested_render_name}_nest;")
         ri.cw.block_start(line=iter_line)
-        ri.cw.block_start(line=f"if (mnl_attr_get_type(attr) == {aspec.enum_name})")
+        ri.cw.block_start(line=f"if (ynl_attr_type(attr) == {aspec.enum_name})")
         if 'nested-attributes' in aspec:
             ri.cw.p(f"parg.data = &dst->{aspec.c_name}[i];")
             ri.cw.p(f"if ({aspec.nested_render_name}_parse(&parg, attr))")
             ri.cw.p('return MNL_CB_ERROR;')
         elif aspec.type in scalars:
-            ri.cw.p(f"dst->{aspec.c_name}[i] = mnl_attr_get_{aspec.mnl_type()}(attr);")
+            ri.cw.p(f"dst->{aspec.c_name}[i] = ynl_attr_get_{aspec.type}(attr);")
         else:
             raise Exception('Nest parsing type not supported yet')
         ri.cw.p('i++;')
-- 
2.43.2

Re: [PATCH net-next 02/15] tools: ynl: create local attribute helpers

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> Don't use mnl attr helpers, we're trying to remove the libmnl
> dependency. Create both signed and unsigned helpers, libmnl
> had unsigned helpers, so code generator no longer needs
> the mnl_type() hack.
> 
> The new helpers are written from first principles, but are
> hopefully not too buggy.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/net/ynl/lib/ynl-priv.h | 215 ++++++++++++++++++++++++++++++++---
>  tools/net/ynl/lib/ynl.c      |  44 +++----
>  tools/net/ynl/ynl-gen-c.py   |  59 ++++------
>  3 files changed, 245 insertions(+), 73 deletions(-)
> 
> diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
> index eaa0d432366c..b5f99521fd8d 100644
> --- a/tools/net/ynl/lib/ynl-priv.h
> +++ b/tools/net/ynl/lib/ynl-priv.h
> @@ -127,45 +127,232 @@ int ynl_error_parse(struct ynl_parse_arg *yarg, const char *msg);
>  
>  /* Attribute helpers */
>  
> -static inline __u64 mnl_attr_get_uint(const struct nlattr *attr)
> +static inline void *ynl_nlmsg_end_addr(const struct nlmsghdr *nlh)
>  {
> -	switch (mnl_attr_get_payload_len(attr)) {
> +	return (char *)nlh + nlh->nlmsg_len;
> +}
> +
> +static inline unsigned int ynl_attr_type(const struct nlattr *attr)
> +{
> +	return attr->nla_type & NLA_TYPE_MASK;
> +}
> +
> +static inline unsigned int ynl_attr_data_len(const struct nlattr *attr)
> +{
> +	return attr->nla_len - NLA_ALIGN(sizeof(struct nlattr));
nit: NLA_HDRLEN ?

> +}
> +
> +static inline void *ynl_attr_data(const struct nlattr *attr)
> +{
> +	return (unsigned char *)attr + NLA_ALIGN(sizeof(struct nlattr));
Same.

> +}
> +
> +static inline struct nlattr *
> +ynl_attr_nest_start(struct nlmsghdr *nlh, unsigned int attr_type)
> +{
> +	struct nlattr *attr;
> +
> +	attr = ynl_nlmsg_end_addr(nlh);
> +	attr->nla_type = attr_type | NLA_F_NESTED;
> +	nlh->nlmsg_len += NLMSG_ALIGN(sizeof(struct nlattr));
Same.

> +
> +	return attr;
> +}
> +
> +static inline void
> +ynl_attr_nest_end(struct nlmsghdr *nlh, struct nlattr *attr)
> +{
> +	attr->nla_len = (char *)ynl_nlmsg_end_addr(nlh) - (char *)attr;
> +}
> +
> +static inline void
> +ynl_attr_put(struct nlmsghdr *nlh, unsigned int attr_type,
> +	     const void *value, size_t size)
> +{
> +	struct nlattr *attr;
> +
> +	attr = ynl_nlmsg_end_addr(nlh);
> +	attr->nla_type = attr_type;
> +	attr->nla_len = NLA_ALIGN(sizeof(struct nlattr)) + size;
Same.

> +
> +	memcpy(ynl_attr_data(attr), value, size);
> +
> +	nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);
> +}
> +
> +static inline void
> +ynl_attr_put_strz(struct nlmsghdr *nlh, unsigned int attr_type, const char *str)
ynl_attr_put_str() ?

> +{
> +	struct nlattr *attr;
> +	const char *end;
> +
> +	attr = ynl_nlmsg_end_addr(nlh);
> +	attr->nla_type = attr_type;
> +
> +	end = stpcpy(ynl_attr_data(attr), str);
> +	attr->nla_len =
> +		NLA_ALIGN(sizeof(struct nlattr)) +
NLA_HDRLEN

> +		NLA_ALIGN(end - (char *)ynl_attr_data(attr));
> +
> +	nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);
> +}
> +
> +static inline const char *ynl_attr_get_str(const struct nlattr *attr)
> +{
> +	return (const char *)(attr + 1);
It's the same, but I tend to prefer:
return (const char *)ynl_attr_data(attr);
Same below.

> +}
> +
> +static inline __s8 ynl_attr_get_s8(const struct nlattr *attr)
> +{
> +	__s8 tmp;
> +
> +	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
Why a memcpy instead of a cast?
return *(__s8 *)ynl_attr_data(attr); ?

> +	return tmp;
> +}
> +
> +static inline __s16 ynl_attr_get_s16(const struct nlattr *attr)
> +{
> +	__s16 tmp;
> +
> +	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
> +	return tmp;
> +}
> +
> +static inline __s32 ynl_attr_get_s32(const struct nlattr *attr)
> +{
> +	__s32 tmp;
> +
> +	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
> +	return tmp;
> +}
> +
> +static inline __s64 ynl_attr_get_s64(const struct nlattr *attr)
> +{
> +	__s64 tmp;
> +
> +	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
> +	return tmp;
> +}
> +
> +static inline __u8 ynl_attr_get_u8(const struct nlattr *attr)
> +{
> +	__u8 tmp;
> +
> +	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
> +	return tmp;
> +}
> +
> +static inline __u16 ynl_attr_get_u16(const struct nlattr *attr)
> +{
> +	__u16 tmp;
> +
> +	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
> +	return tmp;
> +}
> +
> +static inline __u32 ynl_attr_get_u32(const struct nlattr *attr)
> +{
> +	__u32 tmp;
> +
> +	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
> +	return tmp;
> +}
> +
> +static inline __u64 ynl_attr_get_u64(const struct nlattr *attr)
> +{
> +	__u64 tmp;
> +
> +	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));
> +	return tmp;
> +}
> +
> +static inline void
> +ynl_attr_put_s8(struct nlmsghdr *nlh, unsigned int attr_type, __s8 value)
> +{
> +	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
> +}
> +
> +static inline void
> +ynl_attr_put_s16(struct nlmsghdr *nlh, unsigned int attr_type, __s16 value)
> +{
> +	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
> +}
> +
> +static inline void
> +ynl_attr_put_s32(struct nlmsghdr *nlh, unsigned int attr_type, __s32 value)
> +{
> +	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
> +}
> +
> +static inline void
> +ynl_attr_put_s64(struct nlmsghdr *nlh, unsigned int attr_type, __s64 value)
> +{
> +	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
> +}
> +
> +static inline void
> +ynl_attr_put_u8(struct nlmsghdr *nlh, unsigned int attr_type, __u8 value)
> +{
> +	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
> +}
> +
> +static inline void
> +ynl_attr_put_u16(struct nlmsghdr *nlh, unsigned int attr_type, __u16 value)
> +{
> +	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
> +}
> +
> +static inline void
> +ynl_attr_put_u32(struct nlmsghdr *nlh, unsigned int attr_type, __u32 value)
> +{
> +	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
> +}
> +
> +static inline void
> +ynl_attr_put_u64(struct nlmsghdr *nlh, unsigned int attr_type, __u64 value)
> +{
> +	ynl_attr_put(nlh, attr_type, &value, sizeof(value));
> +}
> +
> +static inline __u64 ynl_attr_get_uint(const struct nlattr *attr)
> +{
> +	switch (ynl_attr_data_len(attr)) {
>  	case 4:
> -		return mnl_attr_get_u32(attr);
> +		return ynl_attr_get_u32(attr);
>  	case 8:
> -		return mnl_attr_get_u64(attr);
> +		return ynl_attr_get_u64(attr);
>  	default:
>  		return 0;
>  	}
>  }
>  
> -static inline __s64 mnl_attr_get_sint(const struct nlattr *attr)
> +static inline __s64 ynl_attr_get_sint(const struct nlattr *attr)
>  {
> -	switch (mnl_attr_get_payload_len(attr)) {
> +	switch (ynl_attr_data_len(attr)) {
>  	case 4:
> -		return mnl_attr_get_u32(attr);
> +		return ynl_attr_get_u32(attr);
ynl_attr_get_s32() ?

>  	case 8:
> -		return mnl_attr_get_u64(attr);
> +		return ynl_attr_get_u64(attr);
>  	default:
>  		return 0;
>  	}
>  }
>  
>  static inline void
> -mnl_attr_put_uint(struct nlmsghdr *nlh, __u16 type, __u64 data)
> +ynl_attr_put_uint(struct nlmsghdr *nlh, __u16 type, __u64 data)
>  {
>  	if ((__u32)data == (__u64)data)
> -		mnl_attr_put_u32(nlh, type, data);
> +		ynl_attr_put_u32(nlh, type, data);
>  	else
> -		mnl_attr_put_u64(nlh, type, data);
> +		ynl_attr_put_u64(nlh, type, data);
>  }
>  
>  static inline void
> -mnl_attr_put_sint(struct nlmsghdr *nlh, __u16 type, __s64 data)
> +ynl_attr_put_sint(struct nlmsghdr *nlh, __u16 type, __s64 data)
>  {
>  	if ((__s32)data == (__s64)data)
> -		mnl_attr_put_u32(nlh, type, data);
> +		ynl_attr_put_u32(nlh, type, data);
ynl_attr_put_s32() ?

>  	else
> -		mnl_attr_put_u64(nlh, type, data);
> +		ynl_attr_put_u64(nlh, type, data);
>  }
>  #endif
> diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> index 6e6d474c8366..f16297713c0e 100644
> --- a/tools/net/ynl/lib/ynl.c
> +++ b/tools/net/ynl/lib/ynl.c
> @@ -94,7 +94,7 @@ ynl_err_walk(struct ynl_sock *ys, void *start, void *end, unsigned int off,
>  
>  	mnl_attr_for_each_payload(start, data_len) {
>  		astart_off = (char *)attr - (char *)start;
> -		aend_off = astart_off + mnl_attr_get_payload_len(attr);
> +		aend_off = astart_off + ynl_attr_data_len(attr);
>  		if (aend_off <= off)
>  			continue;
>  
> @@ -106,7 +106,7 @@ ynl_err_walk(struct ynl_sock *ys, void *start, void *end, unsigned int off,
>  
>  	off -= astart_off;
>  
> -	type = mnl_attr_get_type(attr);
> +	type = ynl_attr_type(attr);
>  
>  	if (ynl_err_walk_report_one(policy, type, str, str_sz, &n))
>  		return n;
> @@ -124,8 +124,8 @@ ynl_err_walk(struct ynl_sock *ys, void *start, void *end, unsigned int off,
>  	}
>  
>  	off -= sizeof(struct nlattr);
> -	start =  mnl_attr_get_payload(attr);
> -	end = start + mnl_attr_get_payload_len(attr);
> +	start =  ynl_attr_data(attr);
> +	end = start + ynl_attr_data_len(attr);
>  
>  	return n + ynl_err_walk(ys, start, end, off, policy->table[type].nest,
>  				&str[n], str_sz - n, nest_pol);
> @@ -153,8 +153,8 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
>  	mnl_attr_for_each(attr, nlh, hlen) {
>  		unsigned int len, type;
>  
> -		len = mnl_attr_get_payload_len(attr);
> -		type = mnl_attr_get_type(attr);
> +		len = ynl_attr_data_len(attr);
> +		type = ynl_attr_type(attr);
>  
>  		if (type > NLMSGERR_ATTR_MAX)
>  			continue;
> @@ -169,7 +169,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
>  				return MNL_CB_ERROR;
>  			break;
>  		case NLMSGERR_ATTR_MSG:
> -			str = mnl_attr_get_payload(attr);
> +			str = ynl_attr_data(attr);
ynl_attr_get_str() ?

>  			if (str[len - 1])
>  				return MNL_CB_ERROR;
>  			break;
> @@ -185,7 +185,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
>  		unsigned int n, off;
>  		void *start, *end;
>  
> -		ys->err.attr_offs = mnl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]);
> +		ys->err.attr_offs = ynl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]);
>  
>  		n = snprintf(bad_attr, sizeof(bad_attr), "%sbad attribute: ",
>  			     str ? " (" : "");
> @@ -211,7 +211,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
>  		void *start, *end;
>  		int n2;
>  
> -		type = mnl_attr_get_u32(tb[NLMSGERR_ATTR_MISS_TYPE]);
> +		type = ynl_attr_get_u32(tb[NLMSGERR_ATTR_MISS_TYPE]);
>  
>  		n = snprintf(miss_attr, sizeof(miss_attr), "%smissing attribute: ",
>  			     bad_attr[0] ? ", " : (str ? " (" : ""));
> @@ -222,7 +222,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
>  
>  		nest_pol = ys->req_policy;
>  		if (tb[NLMSGERR_ATTR_MISS_NEST]) {
> -			off = mnl_attr_get_u32(tb[NLMSGERR_ATTR_MISS_NEST]);
> +			off = ynl_attr_get_u32(tb[NLMSGERR_ATTR_MISS_NEST]);
>  			off -= sizeof(struct nlmsghdr);
>  			off -= ys->family->hdr_len;
>  
> @@ -314,9 +314,9 @@ int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
>  	unsigned int type, len;
>  	unsigned char *data;
>  
> -	data = mnl_attr_get_payload(attr);
> -	len = mnl_attr_get_payload_len(attr);
> -	type = mnl_attr_get_type(attr);
> +	data = ynl_attr_data(attr);
> +	len = ynl_attr_data_len(attr);
> +	type = ynl_attr_type(attr);
>  	if (type > yarg->rsp_policy->max_attr) {
>  		yerr(yarg->ys, YNL_ERROR_INTERNAL,
>  		     "Internal error, validating unknown attribute");
> @@ -514,11 +514,11 @@ ynl_get_family_info_mcast(struct ynl_sock *ys, const struct nlattr *mcasts)
>  	i = 0;
>  	mnl_attr_for_each_nested(entry, mcasts) {
>  		mnl_attr_for_each_nested(attr, entry) {
> -			if (mnl_attr_get_type(attr) == CTRL_ATTR_MCAST_GRP_ID)
> -				ys->mcast_groups[i].id = mnl_attr_get_u32(attr);
> -			if (mnl_attr_get_type(attr) == CTRL_ATTR_MCAST_GRP_NAME) {
> +			if (ynl_attr_type(attr) == CTRL_ATTR_MCAST_GRP_ID)
> +				ys->mcast_groups[i].id = ynl_attr_get_u32(attr);
> +			if (ynl_attr_type(attr) == CTRL_ATTR_MCAST_GRP_NAME) {
>  				strncpy(ys->mcast_groups[i].name,
> -					mnl_attr_get_str(attr),
> +					ynl_attr_get_str(attr),
>  					GENL_NAMSIZ - 1);
>  				ys->mcast_groups[i].name[GENL_NAMSIZ - 1] = 0;
>  			}
> @@ -536,19 +536,19 @@ static int ynl_get_family_info_cb(const struct nlmsghdr *nlh, void *data)
>  	bool found_id = true;
>  
>  	mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
> -		if (mnl_attr_get_type(attr) == CTRL_ATTR_MCAST_GROUPS)
> +		if (ynl_attr_type(attr) == CTRL_ATTR_MCAST_GROUPS)
>  			if (ynl_get_family_info_mcast(ys, attr))
>  				return MNL_CB_ERROR;
>  
> -		if (mnl_attr_get_type(attr) != CTRL_ATTR_FAMILY_ID)
> +		if (ynl_attr_type(attr) != CTRL_ATTR_FAMILY_ID)
>  			continue;
>  
> -		if (mnl_attr_get_payload_len(attr) != sizeof(__u16)) {
> +		if (ynl_attr_data_len(attr) != sizeof(__u16)) {
>  			yerr(ys, YNL_ERROR_ATTR_INVALID, "Invalid family ID");
>  			return MNL_CB_ERROR;
>  		}
>  
> -		ys->family_id = mnl_attr_get_u16(attr);
> +		ys->family_id = ynl_attr_get_u16(attr);
>  		found_id = true;
>  	}
>  
> @@ -566,7 +566,7 @@ static int ynl_sock_read_family(struct ynl_sock *ys, const char *family_name)
>  	int err;
>  
>  	nlh = ynl_gemsg_start_req(ys, GENL_ID_CTRL, CTRL_CMD_GETFAMILY, 1);
> -	mnl_attr_put_strz(nlh, CTRL_ATTR_FAMILY_NAME, family_name);
> +	ynl_attr_put_strz(nlh, CTRL_ATTR_FAMILY_NAME, family_name);
>  
>  	err = mnl_socket_sendto(ys->sock, nlh, nlh->nlmsg_len);
>  	if (err < 0) {
> diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
> index 7fc1aa788f6f..4f19c959ee7e 100755
> --- a/tools/net/ynl/ynl-gen-c.py
> +++ b/tools/net/ynl/ynl-gen-c.py
> @@ -168,15 +168,6 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>          spec = self._attr_policy(policy)
>          cw.p(f"\t[{self.enum_name}] = {spec},")
>  
> -    def _mnl_type(self):
> -        # mnl does not have helpers for signed integer types
> -        # turn signed type into unsigned
> -        # this only makes sense for scalar types
> -        t = self.type
> -        if t[0] == 's':
> -            t = 'u' + t[1:]
> -        return t
> -
>      def _attr_typol(self):
>          raise Exception(f"Type policy not implemented for class type {self.type}")
>  
> @@ -192,7 +183,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>          ri.cw.p(f"{line};")
>  
>      def _attr_put_simple(self, ri, var, put_type):
> -        line = f"mnl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name})"
> +        line = f"ynl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name})"
>          self._attr_put_line(ri, var, line)
>  
>      def attr_put(self, ri, var):
> @@ -357,9 +348,6 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>          else:
>              self.type_name = '__' + self.type
>  
> -    def mnl_type(self):
> -        return self._mnl_type()
> -
>      def _attr_policy(self, policy):
>          if 'flags-mask' in self.checks or self.is_bitfield:
>              if self.is_bitfield:
> @@ -387,10 +375,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>          return [f'{self.type_name} {self.c_name}{self.byte_order_comment}']
>  
>      def attr_put(self, ri, var):
> -        self._attr_put_simple(ri, var, self.mnl_type())
> +        self._attr_put_simple(ri, var, self.type)
>  
>      def _attr_get(self, ri, var):
> -        return f"{var}->{self.c_name} = mnl_attr_get_{self.mnl_type()}(attr);", None, None
> +        return f"{var}->{self.c_name} = ynl_attr_get_{self.type}(attr);", None, None
>  
>      def _setter_lines(self, ri, member, presence):
>          return [f"{member} = {self.c_name};"]
> @@ -404,7 +392,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>          return '.type = YNL_PT_FLAG, '
>  
>      def attr_put(self, ri, var):
> -        self._attr_put_line(ri, var, f"mnl_attr_put(nlh, {self.enum_name}, 0, NULL)")
> +        self._attr_put_line(ri, var, f"ynl_attr_put(nlh, {self.enum_name}, NULL, 0)")
>  
>      def _attr_get(self, ri, var):
>          return [], None, None
> @@ -452,9 +440,9 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>          len_mem = var + '->_present.' + self.c_name + '_len'
>          return [f"{len_mem} = len;",
>                  f"{var}->{self.c_name} = malloc(len + 1);",
> -                f"memcpy({var}->{self.c_name}, mnl_attr_get_str(attr), len);",
> +                f"memcpy({var}->{self.c_name}, ynl_attr_get_str(attr), len);",
>                  f"{var}->{self.c_name}[len] = 0;"], \
> -               ['len = strnlen(mnl_attr_get_str(attr), mnl_attr_get_payload_len(attr));'], \
> +               ['len = strnlen(ynl_attr_get_str(attr), ynl_attr_data_len(attr));'], \
>                 ['unsigned int len;']
>  
>      def _setter_lines(self, ri, member, presence):
> @@ -493,15 +481,15 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>          return mem
>  
>      def attr_put(self, ri, var):
> -        self._attr_put_line(ri, var, f"mnl_attr_put(nlh, {self.enum_name}, " +
> -                            f"{var}->_present.{self.c_name}_len, {var}->{self.c_name})")
> +        self._attr_put_line(ri, var, f"ynl_attr_put(nlh, {self.enum_name}, " +
> +                            f"{var}->{self.c_name}, {var}->_present.{self.c_name}_len)")
>  
>      def _attr_get(self, ri, var):
>          len_mem = var + '->_present.' + self.c_name + '_len'
>          return [f"{len_mem} = len;",
>                  f"{var}->{self.c_name} = malloc(len);",
> -                f"memcpy({var}->{self.c_name}, mnl_attr_get_payload(attr), len);"], \
> -               ['len = mnl_attr_get_payload_len(attr);'], \
> +                f"memcpy({var}->{self.c_name}, ynl_attr_data(attr), len);"], \
> +               ['len = ynl_attr_data_len(attr);'], \
>                 ['unsigned int len;']
>  
>      def _setter_lines(self, ri, member, presence):
> @@ -526,11 +514,11 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>          return f"NLA_POLICY_BITFIELD32({mask})"
>  
>      def attr_put(self, ri, var):
> -        line = f"mnl_attr_put(nlh, {self.enum_name}, sizeof(struct nla_bitfield32), &{var}->{self.c_name})"
> +        line = f"ynl_attr_put(nlh, {self.enum_name}, &{var}->{self.c_name}, sizeof(struct nla_bitfield32))"
>          self._attr_put_line(ri, var, line)
>  
>      def _attr_get(self, ri, var):
> -        return f"memcpy(&{var}->{self.c_name}, mnl_attr_get_payload(attr), sizeof(struct nla_bitfield32));", None, None
> +        return f"memcpy(&{var}->{self.c_name}, ynl_attr_data(attr), sizeof(struct nla_bitfield32));", None, None
>  
>      def _setter_lines(self, ri, member, presence):
>          return [f"memcpy(&{member}, {self.c_name}, sizeof(struct nla_bitfield32));"]
> @@ -589,9 +577,6 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>      def presence_type(self):
>          return 'count'
>  
> -    def mnl_type(self):
> -        return self._mnl_type()
> -
>      def _complex_member_type(self, ri):
>          if 'type' not in self.attr or self.attr['type'] == 'nest':
>              return self.nested_struct_type
> @@ -625,9 +610,9 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>  
>      def attr_put(self, ri, var):
>          if self.attr['type'] in scalars:
> -            put_type = self.mnl_type()
> +            put_type = self.type
>              ri.cw.p(f"for (unsigned int i = 0; i < {var}->n_{self.c_name}; i++)")
> -            ri.cw.p(f"mnl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name}[i]);")
> +            ri.cw.p(f"ynl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name}[i]);")
>          elif 'type' not in self.attr or self.attr['type'] == 'nest':
>              ri.cw.p(f"for (unsigned int i = 0; i < {var}->n_{self.c_name}; i++)")
>              self._attr_put_line(ri, var, f"{self.nested_render_name}_put(nlh, " +
> @@ -690,8 +675,8 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>              local_vars += [f'__u32 {", ".join(tv_names)};']
>              for level in self.attr["type-value"]:
>                  level = c_lower(level)
> -                get_lines += [f'attr_{level} = mnl_attr_get_payload({prev});']
> -                get_lines += [f'{level} = mnl_attr_get_type(attr_{level});']
> +                get_lines += [f'attr_{level} = ynl_attr_data({prev});']
> +                get_lines += [f'{level} = ynl_attr_type(attr_{level});']
>                  prev = 'attr_' + level
>  
>              tv_args = f", {', '.join(tv_names)}"
> @@ -1612,12 +1597,12 @@ _C_KW = {
>      ri.cw.block_start()
>      ri.cw.write_func_lvar('struct nlattr *nest;')
>  
> -    ri.cw.p("nest = mnl_attr_nest_start(nlh, attr_type);")
> +    ri.cw.p("nest = ynl_attr_nest_start(nlh, attr_type);")
>  
>      for _, arg in struct.member_list():
>          arg.attr_put(ri, "obj")
>  
> -    ri.cw.p("mnl_attr_nest_end(nlh, nest);")
> +    ri.cw.p("ynl_attr_nest_end(nlh, nest);")
>  
>      ri.cw.nl()
>      ri.cw.p('return 0;')
> @@ -1674,7 +1659,7 @@ _C_KW = {
>  
>      ri.cw.nl()
>      ri.cw.block_start(line=iter_line)
> -    ri.cw.p('unsigned int type = mnl_attr_get_type(attr);')
> +    ri.cw.p('unsigned int type = ynl_attr_type(attr);')
>      ri.cw.nl()
>  
>      first = True
> @@ -1696,7 +1681,7 @@ _C_KW = {
>          ri.cw.p(f"parg.rsp_policy = &{aspec.nested_render_name}_nest;")
>          ri.cw.block_start(line=f"mnl_attr_for_each_nested(attr, attr_{aspec.c_name})")
>          ri.cw.p(f"parg.data = &dst->{aspec.c_name}[i];")
> -        ri.cw.p(f"if ({aspec.nested_render_name}_parse(&parg, attr, mnl_attr_get_type(attr)))")
> +        ri.cw.p(f"if ({aspec.nested_render_name}_parse(&parg, attr, ynl_attr_type(attr)))")
>          ri.cw.p('return MNL_CB_ERROR;')
>          ri.cw.p('i++;')
>          ri.cw.block_end()
> @@ -1712,13 +1697,13 @@ _C_KW = {
>          if 'nested-attributes' in aspec:
>              ri.cw.p(f"parg.rsp_policy = &{aspec.nested_render_name}_nest;")
>          ri.cw.block_start(line=iter_line)
> -        ri.cw.block_start(line=f"if (mnl_attr_get_type(attr) == {aspec.enum_name})")
> +        ri.cw.block_start(line=f"if (ynl_attr_type(attr) == {aspec.enum_name})")
>          if 'nested-attributes' in aspec:
>              ri.cw.p(f"parg.data = &dst->{aspec.c_name}[i];")
>              ri.cw.p(f"if ({aspec.nested_render_name}_parse(&parg, attr))")
>              ri.cw.p('return MNL_CB_ERROR;')
>          elif aspec.type in scalars:
> -            ri.cw.p(f"dst->{aspec.c_name}[i] = mnl_attr_get_{aspec.mnl_type()}(attr);")
> +            ri.cw.p(f"dst->{aspec.c_name}[i] = ynl_attr_get_{aspec.type}(attr);")
>          else:
>              raise Exception('Nest parsing type not supported yet')
>          ri.cw.p('i++;')

Re: [PATCH net-next 02/15] tools: ynl: create local attribute helpers

[Jakub Kicinski]

On Fri, 23 Feb 2024 15:03:49 +0100 Nicolas Dichtel wrote:
> > +static inline unsigned int ynl_attr_data_len(const struct nlattr *attr)
> > +{
> > +	return attr->nla_len - NLA_ALIGN(sizeof(struct nlattr));  
> nit: NLA_HDRLEN ?

IIRC I did that because I kept looking at the definition of 
NLA_HDRLEN to check if it's already ALIGNed or not :( The name of 
the define doesn't say. Not that it matters at all given the len is
multiple of 4. If you think NLA_HDRLEN is more idiomatic I'll switch.

> > +		NLA_ALIGN(end - (char *)ynl_attr_data(attr));
> > +
> > +	nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);
> > +}
> > +
> > +static inline const char *ynl_attr_get_str(const struct nlattr *attr)
> > +{
> > +	return (const char *)(attr + 1);  
> It's the same, but I tend to prefer:
> return (const char *)ynl_attr_data(attr);
> Same below.

SG.

> > +}
> > +
> > +static inline __s8 ynl_attr_get_s8(const struct nlattr *attr)
> > +{
> > +	__s8 tmp;
> > +
> > +	memcpy(&tmp, (unsigned char *)(attr + 1), sizeof(tmp));  
> Why a memcpy instead of a cast?
> return *(__s8 *)ynl_attr_data(attr); ?

Sure.

> > -static inline __s64 mnl_attr_get_sint(const struct nlattr *attr)
> > +static inline __s64 ynl_attr_get_sint(const struct nlattr *attr)
> >  {
> > -	switch (mnl_attr_get_payload_len(attr)) {
> > +	switch (ynl_attr_data_len(attr)) {
> >  	case 4:
> > -		return mnl_attr_get_u32(attr);
> > +		return ynl_attr_get_u32(attr);  
> ynl_attr_get_s32() ?

Ah, good catch!

> >  		case NLMSGERR_ATTR_MSG:
> > -			str = mnl_attr_get_payload(attr);
> > +			str = ynl_attr_data(attr);  
> ynl_attr_get_str() ?

ditto.

Thanks!

Re: [PATCH net-next 02/15] tools: ynl: create local attribute helpers

[Nicolas Dichtel]

Le 23/02/2024 à 15:46, Jakub Kicinski a écrit :
> On Fri, 23 Feb 2024 15:03:49 +0100 Nicolas Dichtel wrote:
>>> +static inline unsigned int ynl_attr_data_len(const struct nlattr *attr)
>>> +{
>>> +	return attr->nla_len - NLA_ALIGN(sizeof(struct nlattr));  
>> nit: NLA_HDRLEN ?
> 
> IIRC I did that because I kept looking at the definition of 
> NLA_HDRLEN to check if it's already ALIGNed or not :( The name of 
> the define doesn't say. Not that it matters at all given the len is
I need to check the define every time :D

> multiple of 4. If you think NLA_HDRLEN is more idiomatic I'll switch.
Yes, if you don't mind, I prefer. It better describes the code I think.

[PATCH net-next 03/15] tools: ynl: create local for_each helpers

[Jakub Kicinski]

Create ynl_attr_for_each*() iteration helpers.
Use them instead of the mnl ones.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h | 47 ++++++++++++++++++++++++++++++++++++
 tools/net/ynl/lib/ynl.c      | 12 ++++-----
 tools/net/ynl/ynl-gen-c.py   |  8 +++---
 3 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index b5f99521fd8d..1d658897cceb 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -147,6 +147,53 @@ static inline void *ynl_attr_data(const struct nlattr *attr)
 	return (unsigned char *)attr + NLA_ALIGN(sizeof(struct nlattr));
 }
 
+static inline void *ynl_attr_data_end(const struct nlattr *attr)
+{
+	return ynl_attr_data(attr) + ynl_attr_data_len(attr);
+}
+
+#define ynl_attr_for_each(attr, nlh, fixed_hdr_sz)			\
+	for ((attr) = ynl_attr_first(nlh, (nlh)->nlmsg_len,		\
+				     NLMSG_HDRLEN + fixed_hdr_sz); attr; \
+	     (attr) = ynl_attr_next(ynl_nlmsg_end_addr(nlh), attr))
+
+#define ynl_attr_for_each_nested(attr, outer)				\
+	for ((attr) = ynl_attr_first(outer, outer->nla_len,		\
+				     sizeof(struct nlattr)); attr;	\
+	     (attr) = ynl_attr_next(ynl_attr_data_end(outer), attr))
+
+#define ynl_attr_for_each_payload(start, len, attr)			\
+	for ((attr) = ynl_attr_first(start, len, 0); attr;		\
+	     (attr) = ynl_attr_next(start + len, attr))
+
+static inline struct nlattr *
+ynl_attr_if_good(const void *end, struct nlattr *attr)
+{
+	if (attr + 1 > (const struct nlattr *)end)
+		return NULL;
+	if (ynl_attr_data_end(attr) > end)
+		return NULL;
+	return attr;
+}
+
+static inline struct nlattr *
+ynl_attr_next(const void *end, const struct nlattr *prev)
+{
+	struct nlattr *attr;
+
+	attr = (void *)((char *)prev + NLA_ALIGN(prev->nla_len));
+	return ynl_attr_if_good(end, attr);
+}
+
+static inline struct nlattr *
+ynl_attr_first(const void *start, size_t len, size_t skip)
+{
+	struct nlattr *attr;
+
+	attr = (void *)((char *)start + NLMSG_ALIGN(skip));
+	return ynl_attr_if_good(start + len, attr);
+}
+
 static inline struct nlattr *
 ynl_attr_nest_start(struct nlmsghdr *nlh, unsigned int attr_type)
 {
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index f16297713c0e..614aa3736d84 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -92,7 +92,7 @@ ynl_err_walk(struct ynl_sock *ys, void *start, void *end, unsigned int off,
 
 	data_len = end - start;
 
-	mnl_attr_for_each_payload(start, data_len) {
+	ynl_attr_for_each_payload(start, data_len, attr) {
 		astart_off = (char *)attr - (char *)start;
 		aend_off = astart_off + ynl_attr_data_len(attr);
 		if (aend_off <= off)
@@ -150,7 +150,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 		return MNL_CB_OK;
 	}
 
-	mnl_attr_for_each(attr, nlh, hlen) {
+	ynl_attr_for_each(attr, nlh, hlen) {
 		unsigned int len, type;
 
 		len = ynl_attr_data_len(attr);
@@ -500,7 +500,7 @@ ynl_get_family_info_mcast(struct ynl_sock *ys, const struct nlattr *mcasts)
 	const struct nlattr *entry, *attr;
 	unsigned int i;
 
-	mnl_attr_for_each_nested(attr, mcasts)
+	ynl_attr_for_each_nested(attr, mcasts)
 		ys->n_mcast_groups++;
 
 	if (!ys->n_mcast_groups)
@@ -512,8 +512,8 @@ ynl_get_family_info_mcast(struct ynl_sock *ys, const struct nlattr *mcasts)
 		return MNL_CB_ERROR;
 
 	i = 0;
-	mnl_attr_for_each_nested(entry, mcasts) {
-		mnl_attr_for_each_nested(attr, entry) {
+	ynl_attr_for_each_nested(entry, mcasts) {
+		ynl_attr_for_each_nested(attr, entry) {
 			if (ynl_attr_type(attr) == CTRL_ATTR_MCAST_GRP_ID)
 				ys->mcast_groups[i].id = ynl_attr_get_u32(attr);
 			if (ynl_attr_type(attr) == CTRL_ATTR_MCAST_GRP_NAME) {
@@ -535,7 +535,7 @@ static int ynl_get_family_info_cb(const struct nlmsghdr *nlh, void *data)
 	const struct nlattr *attr;
 	bool found_id = true;
 
-	mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
+	ynl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
 		if (ynl_attr_type(attr) == CTRL_ATTR_MCAST_GROUPS)
 			if (ynl_get_family_info_mcast(ys, attr))
 				return MNL_CB_ERROR;
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 4f19c959ee7e..01380e6d2881 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -650,7 +650,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def _attr_get(self, ri, var):
         local_vars = ['const struct nlattr *attr2;']
         get_lines = [f'attr_{self.c_name} = attr;',
-                     'mnl_attr_for_each_nested(attr2, attr)',
+                     'ynl_attr_for_each_nested(attr2, attr)',
                      f'\t{var}->n_{self.c_name}++;']
         return get_lines, None, local_vars
 
@@ -1612,11 +1612,11 @@ _C_KW = {
 
 def _multi_parse(ri, struct, init_lines, local_vars):
     if struct.nested:
-        iter_line = "mnl_attr_for_each_nested(attr, nested)"
+        iter_line = "ynl_attr_for_each_nested(attr, nested)"
     else:
         if ri.fixed_hdr:
             local_vars += ['void *hdr;']
-        iter_line = "mnl_attr_for_each(attr, nlh, yarg->ys->family->hdr_len)"
+        iter_line = "ynl_attr_for_each(attr, nlh, yarg->ys->family->hdr_len)"
 
     array_nests = set()
     multi_attrs = set()
@@ -1679,7 +1679,7 @@ _C_KW = {
         ri.cw.p(f"dst->n_{aspec.c_name} = n_{aspec.c_name};")
         ri.cw.p('i = 0;')
         ri.cw.p(f"parg.rsp_policy = &{aspec.nested_render_name}_nest;")
-        ri.cw.block_start(line=f"mnl_attr_for_each_nested(attr, attr_{aspec.c_name})")
+        ri.cw.block_start(line=f"ynl_attr_for_each_nested(attr, attr_{aspec.c_name})")
         ri.cw.p(f"parg.data = &dst->{aspec.c_name}[i];")
         ri.cw.p(f"if ({aspec.nested_render_name}_parse(&parg, attr, ynl_attr_type(attr)))")
         ri.cw.p('return MNL_CB_ERROR;')
-- 
2.43.2

Re: [PATCH net-next 03/15] tools: ynl: create local for_each helpers

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> Create ynl_attr_for_each*() iteration helpers.
> Use them instead of the mnl ones.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 04/15] tools: ynl: create local nlmsg access helpers

[Jakub Kicinski]

Create helpers for accessing payloads of struct nlmsg.
Use them instead of the libmnl ones.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h | 39 +++++++++++++++++++++++++++++++++++-
 tools/net/ynl/lib/ynl.c      | 24 ++++++++++------------
 tools/net/ynl/ynl-gen-c.py   |  6 +++---
 3 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 1d658897cceb..654d1e29c482 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -125,13 +125,50 @@ int ynl_exec_dump(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 void ynl_error_unknown_notification(struct ynl_sock *ys, __u8 cmd);
 int ynl_error_parse(struct ynl_parse_arg *yarg, const char *msg);
 
-/* Attribute helpers */
+/* Netlink message handling helpers */
+
+static inline struct nlmsghdr *ynl_nlmsg_put_header(void *buf)
+{
+	struct nlmsghdr *nlh = buf;
+
+	memset(nlh, 0, sizeof(*nlh));
+	nlh->nlmsg_len = NLMSG_HDRLEN;
+
+	return nlh;
+}
+
+static inline unsigned int ynl_nlmsg_data_len(const struct nlmsghdr *nlh)
+{
+	return nlh->nlmsg_len - NLMSG_HDRLEN;
+}
+
+static inline void *ynl_nlmsg_data(const struct nlmsghdr *nlh)
+{
+	return (unsigned char *)nlh + NLMSG_HDRLEN;
+}
+
+static inline void *
+ynl_nlmsg_data_offset(const struct nlmsghdr *nlh, unsigned int offset)
+{
+	return (unsigned char *)nlh + NLMSG_HDRLEN + offset;
+}
 
 static inline void *ynl_nlmsg_end_addr(const struct nlmsghdr *nlh)
 {
 	return (char *)nlh + nlh->nlmsg_len;
 }
 
+static inline void *
+ynl_nlmsg_put_extra_header(struct nlmsghdr *nlh, unsigned int size)
+{
+	void *tail = ynl_nlmsg_end_addr(nlh);
+
+	nlh->nlmsg_len += NLMSG_ALIGN(size);
+	return tail;
+}
+
+/* Netlink attribute helpers */
+
 static inline unsigned int ynl_attr_type(const struct nlattr *attr)
 {
 	return attr->nla_type & NLA_TYPE_MASK;
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 614aa3736d84..39f7c7b23443 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -190,9 +190,8 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 		n = snprintf(bad_attr, sizeof(bad_attr), "%sbad attribute: ",
 			     str ? " (" : "");
 
-		start = mnl_nlmsg_get_payload_offset(ys->nlh,
-						     ys->family->hdr_len);
-		end = mnl_nlmsg_get_payload_tail(ys->nlh);
+		start = ynl_nlmsg_data_offset(ys->nlh, ys->family->hdr_len);
+		end = ynl_nlmsg_end_addr(ys->nlh);
 
 		off = ys->err.attr_offs;
 		off -= sizeof(struct nlmsghdr);
@@ -216,9 +215,8 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 		n = snprintf(miss_attr, sizeof(miss_attr), "%smissing attribute: ",
 			     bad_attr[0] ? ", " : (str ? " (" : ""));
 
-		start = mnl_nlmsg_get_payload_offset(ys->nlh,
-						     ys->family->hdr_len);
-		end = mnl_nlmsg_get_payload_tail(ys->nlh);
+		start = ynl_nlmsg_data_offset(ys->nlh, ys->family->hdr_len);
+		end = ynl_nlmsg_end_addr(ys->nlh);
 
 		nest_pol = ys->req_policy;
 		if (tb[NLMSGERR_ATTR_MISS_NEST]) {
@@ -259,7 +257,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 
 static int ynl_cb_error(const struct nlmsghdr *nlh, void *data)
 {
-	const struct nlmsgerr *err = mnl_nlmsg_get_payload(nlh);
+	const struct nlmsgerr *err = ynl_nlmsg_data(nlh);
 	struct ynl_parse_arg *yarg = data;
 	unsigned int hlen;
 	int code;
@@ -270,7 +268,7 @@ static int ynl_cb_error(const struct nlmsghdr *nlh, void *data)
 
 	hlen = sizeof(*err);
 	if (!(nlh->nlmsg_flags & NLM_F_CAPPED))
-		hlen += mnl_nlmsg_get_payload_len(&err->msg);
+		hlen += ynl_nlmsg_data_len(&err->msg);
 
 	ynl_ext_ack_check(yarg->ys, nlh, hlen);
 
@@ -413,7 +411,7 @@ struct nlmsghdr *ynl_msg_start(struct ynl_sock *ys, __u32 id, __u16 flags)
 
 	ynl_err_reset(ys);
 
-	nlh = ys->nlh = mnl_nlmsg_put_header(ys->tx_buf);
+	nlh = ys->nlh = ynl_nlmsg_put_header(ys->tx_buf);
 	nlh->nlmsg_type	= id;
 	nlh->nlmsg_flags = flags;
 	nlh->nlmsg_seq = ++ys->seq;
@@ -435,7 +433,7 @@ ynl_gemsg_start(struct ynl_sock *ys, __u32 id, __u16 flags,
 	gehdr.cmd = cmd;
 	gehdr.version = version;
 
-	data = mnl_nlmsg_put_extra_header(nlh, sizeof(gehdr));
+	data = ynl_nlmsg_put_extra_header(nlh, sizeof(gehdr));
 	memcpy(data, &gehdr, sizeof(gehdr));
 
 	return nlh;
@@ -718,7 +716,7 @@ static int ynl_ntf_parse(struct ynl_sock *ys, const struct nlmsghdr *nlh)
 	struct genlmsghdr *gehdr;
 	int ret;
 
-	gehdr = mnl_nlmsg_get_payload(nlh);
+	gehdr = ynl_nlmsg_data(nlh);
 	if (gehdr->cmd >= ys->family->ntf_info_size)
 		return MNL_CB_ERROR;
 	info = &ys->family->ntf_info[gehdr->cmd];
@@ -808,13 +806,13 @@ ynl_check_alien(struct ynl_sock *ys, const struct nlmsghdr *nlh, __u32 rsp_cmd)
 {
 	struct genlmsghdr *gehdr;
 
-	if (mnl_nlmsg_get_payload_len(nlh) < sizeof(*gehdr)) {
+	if (ynl_nlmsg_data_len(nlh) < sizeof(*gehdr)) {
 		yerr(ys, YNL_ERROR_INV_RESP,
 		     "Kernel responded with truncated message");
 		return -1;
 	}
 
-	gehdr = mnl_nlmsg_get_payload(nlh);
+	gehdr = ynl_nlmsg_data(nlh);
 	if (gehdr->cmd != rsp_cmd)
 		return ynl_ntf_parse(ys, nlh);
 
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 01380e6d2881..7b5635bf4c04 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -1650,7 +1650,7 @@ _C_KW = {
         ri.cw.p(f'dst->{arg} = {arg};')
 
     if ri.fixed_hdr:
-        ri.cw.p('hdr = mnl_nlmsg_get_payload_offset(nlh, sizeof(struct genlmsghdr));')
+        ri.cw.p('hdr = ynl_nlmsg_data_offset(nlh, sizeof(struct genlmsghdr));')
         ri.cw.p(f"memcpy(&dst->_hdr, hdr, sizeof({ri.fixed_hdr}));")
     for anest in sorted(all_multi):
         aspec = struct[anest]
@@ -1794,7 +1794,7 @@ _C_KW = {
 
     if ri.fixed_hdr:
         ri.cw.p("hdr_len = sizeof(req->_hdr);")
-        ri.cw.p("hdr = mnl_nlmsg_put_extra_header(nlh, hdr_len);")
+        ri.cw.p("hdr = ynl_nlmsg_put_extra_header(nlh, hdr_len);")
         ri.cw.p("memcpy(hdr, &req->_hdr, hdr_len);")
         ri.cw.nl()
 
@@ -1857,7 +1857,7 @@ _C_KW = {
 
     if ri.fixed_hdr:
         ri.cw.p("hdr_len = sizeof(req->_hdr);")
-        ri.cw.p("hdr = mnl_nlmsg_put_extra_header(nlh, hdr_len);")
+        ri.cw.p("hdr = ynl_nlmsg_put_extra_header(nlh, hdr_len);")
         ri.cw.p("memcpy(hdr, &req->_hdr, hdr_len);")
         ri.cw.nl()
 
-- 
2.43.2

Re: [PATCH net-next 04/15] tools: ynl: create local nlmsg access helpers

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> Create helpers for accessing payloads of struct nlmsg.
> Use them instead of the libmnl ones.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 05/15] tools: ynl: create local ARRAY_SIZE() helper

[Jakub Kicinski]

libc doesn't have an ARRAY_SIZE() create one locally.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h | 3 +++
 tools/net/ynl/ynl-gen-c.py   | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 654d1e29c482..0df0edd1af9a 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -27,6 +27,9 @@ enum ynl_policy_type {
 	YNL_PT_BITFIELD32,
 };
 
+#define YNL_ARRAY_SIZE(array)	(sizeof(array) ?			\
+				 sizeof(array) / sizeof(array[0]) : 0)
+
 struct ynl_policy_attr {
 	enum ynl_policy_type type;
 	unsigned int len;
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 7b5635bf4c04..6366f69e210c 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -1535,7 +1535,7 @@ _C_KW = {
     cw.block_start()
     if enum and enum.type == 'flags':
         cw.p(f'{arg_name} = ffs({arg_name}) - 1;')
-    cw.p(f'if ({arg_name} < 0 || {arg_name} >= (int)MNL_ARRAY_SIZE({map_name}))')
+    cw.p(f'if ({arg_name} < 0 || {arg_name} >= (int)YNL_ARRAY_SIZE({map_name}))')
     cw.p('return NULL;')
     cw.p(f'return {map_name}[{arg_name}];')
     cw.block_end()
@@ -2569,7 +2569,7 @@ _C_KW = {
         cw.p('.hdr_len\t= sizeof(struct genlmsghdr),')
     if family.ntfs:
         cw.p(f".ntf_info\t= {family['name']}_ntf_info,")
-        cw.p(f".ntf_info_size\t= MNL_ARRAY_SIZE({family['name']}_ntf_info),")
+        cw.p(f".ntf_info_size\t= YNL_ARRAY_SIZE({family['name']}_ntf_info),")
     cw.block_end(line=';')
 
 
-- 
2.43.2

Re: [PATCH net-next 05/15] tools: ynl: create local ARRAY_SIZE() helper

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> libc doesn't have an ARRAY_SIZE() create one locally.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 06/15] tools: ynl: make yarg the first member of struct ynl_dump_state

[Jakub Kicinski]

All YNL parsing code expects a pointer to struct ynl_parse_arg AKA yarg.
For dump was pass in struct ynl_dump_state, which works fine, because
struct ynl_dump_state and struct ynl_parse_arg have identical layout
for the members that matter.. but it's a bit hacky.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h | 3 +--
 tools/net/ynl/lib/ynl.c      | 5 ++---
 tools/net/ynl/ynl-gen-c.py   | 5 +++--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 0df0edd1af9a..eb109170102d 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -104,8 +104,7 @@ struct ynl_req_state {
 };
 
 struct ynl_dump_state {
-	struct ynl_sock *ys;
-	struct ynl_policy_nest *rsp_policy;
+	struct ynl_parse_arg yarg;
 	void *first;
 	struct ynl_dump_list_type *last;
 	size_t alloc_sz;
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 39f7c7b23443..ec3bc7baadd1 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -864,7 +864,7 @@ static int ynl_dump_trampoline(const struct nlmsghdr *nlh, void *data)
 	struct ynl_parse_arg yarg = {};
 	int ret;
 
-	ret = ynl_check_alien(ds->ys, nlh, ds->rsp_cmd);
+	ret = ynl_check_alien(ds->yarg.ys, nlh, ds->rsp_cmd);
 	if (ret)
 		return ret < 0 ? MNL_CB_ERROR : MNL_CB_OK;
 
@@ -878,8 +878,7 @@ static int ynl_dump_trampoline(const struct nlmsghdr *nlh, void *data)
 		ds->last->next = obj;
 	ds->last = obj;
 
-	yarg.ys = ds->ys;
-	yarg.rsp_policy = ds->rsp_policy;
+	yarg = ds->yarg;
 	yarg.data = &obj->data;
 
 	return ds->cb(nlh, &yarg);
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 6366f69e210c..5d174ce67dbc 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -1844,14 +1844,15 @@ _C_KW = {
 
     ri.cw.write_func_lvar(local_vars)
 
-    ri.cw.p('yds.ys = ys;')
+    ri.cw.p('yds.yarg.ys = ys;')
+    ri.cw.p(f"yds.yarg.rsp_policy = &{ri.struct['reply'].render_name}_nest;")
+    ri.cw.p("yds.yarg.data = NULL;")
     ri.cw.p(f"yds.alloc_sz = sizeof({type_name(ri, rdir(direction))});")
     ri.cw.p(f"yds.cb = {op_prefix(ri, 'reply', deref=True)}_parse;")
     if ri.op.value is not None:
         ri.cw.p(f'yds.rsp_cmd = {ri.op.enum_name};')
     else:
         ri.cw.p(f'yds.rsp_cmd = {ri.op.rsp_value};')
-    ri.cw.p(f"yds.rsp_policy = &{ri.struct['reply'].render_name}_nest;")
     ri.cw.nl()
     ri.cw.p(f"nlh = ynl_gemsg_start_dump(ys, {ri.nl.get_family_id()}, {ri.op.enum_name}, 1);")
 
-- 
2.43.2

Re: [PATCH net-next 06/15] tools: ynl: make yarg the first member of struct ynl_dump_state

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> All YNL parsing code expects a pointer to struct ynl_parse_arg AKA yarg.
> For dump was pass in struct ynl_dump_state, which works fine, because
> struct ynl_dump_state and struct ynl_parse_arg have identical layout
> for the members that matter.. but it's a bit hacky.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 07/15] tools: ynl-gen: remove unused parse code

[Jakub Kicinski]

Commit f2ba1e5e2208 ("tools: ynl-gen: stop generating common notification handlers")
removed the last caller of the parse_cb_run() helper.
We no longer need to export ynl_cb_array.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h | 2 --
 tools/net/ynl/lib/ynl.c      | 2 +-
 tools/net/ynl/ynl-gen-c.py   | 8 --------
 3 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index eb109170102d..653119c9f47c 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -83,8 +83,6 @@ struct ynl_ntf_base_type {
 	unsigned char data[] __attribute__((aligned(8)));
 };
 
-extern mnl_cb_t ynl_cb_array[NLMSG_MIN_TYPE];
-
 struct nlmsghdr *
 ynl_gemsg_start_req(struct ynl_sock *ys, __u32 id, __u8 cmd, __u8 version);
 struct nlmsghdr *
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index ec3bc7baadd1..c9790257189c 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -297,7 +297,7 @@ static int ynl_cb_noop(const struct nlmsghdr *nlh, void *data)
 	return MNL_CB_OK;
 }
 
-mnl_cb_t ynl_cb_array[NLMSG_MIN_TYPE] = {
+static mnl_cb_t ynl_cb_array[NLMSG_MIN_TYPE] = {
 	[NLMSG_NOOP]	= ynl_cb_noop,
 	[NLMSG_ERROR]	= ynl_cb_error,
 	[NLMSG_DONE]	= ynl_cb_done,
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 5d174ce67dbc..87f8b9d28734 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -40,14 +40,6 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def get_family_id(self):
         return 'ys->family_id'
 
-    def parse_cb_run(self, cb, data, is_dump=False, indent=1):
-        ind = '\n\t\t' + '\t' * indent + ' '
-        if is_dump:
-            return f"mnl_cb_run2(ys->rx_buf, len, 0, 0, {cb}, {data},{ind}ynl_cb_array, NLMSG_MIN_TYPE)"
-        else:
-            return f"mnl_cb_run2(ys->rx_buf, len, ys->seq, ys->portid,{ind}{cb}, {data},{ind}" + \
-                   "ynl_cb_array, NLMSG_MIN_TYPE)"
-
 
 class Type(SpecAttr):
     def __init__(self, family, attr_set, attr, value):
-- 
2.43.2

Re: [PATCH net-next 07/15] tools: ynl-gen: remove unused parse code

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> Commit f2ba1e5e2208 ("tools: ynl-gen: stop generating common notification handlers")
> removed the last caller of the parse_cb_run() helper.
> We no longer need to export ynl_cb_array.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 08/15] tools: ynl: wrap recv() + mnl_cb_run2() into a single helper

[Jakub Kicinski]

All callers to mnl_cb_run2() call mnl_socket_recvfrom() right before.
Wrap the two in a helper, take typed arguments (struct ynl_parse_arg),
instead of hoping that all callers remember that parser error handling
requires yarg.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl.c | 56 +++++++++++++----------------------------
 1 file changed, 18 insertions(+), 38 deletions(-)

diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index c9790257189c..96a209773ace 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -491,6 +491,19 @@ int ynl_cb_null(const struct nlmsghdr *nlh, void *data)
 	return MNL_CB_ERROR;
 }
 
+static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, mnl_cb_t cb)
+{
+	struct ynl_sock *ys = yarg->ys;
+	ssize_t len;
+
+	len = mnl_socket_recvfrom(ys->sock, ys->rx_buf, MNL_SOCKET_BUFFER_SIZE);
+	if (len < 0)
+		return len;
+
+	return mnl_cb_run2(ys->rx_buf, len, ys->seq, ys->portid,
+			   cb, yarg, ynl_cb_array, NLMSG_MIN_TYPE);
+}
+
 /* Init/fini and genetlink boiler plate */
 static int
 ynl_get_family_info_mcast(struct ynl_sock *ys, const struct nlattr *mcasts)
@@ -572,14 +585,7 @@ static int ynl_sock_read_family(struct ynl_sock *ys, const char *family_name)
 		return err;
 	}
 
-	err = mnl_socket_recvfrom(ys->sock, ys->rx_buf, MNL_SOCKET_BUFFER_SIZE);
-	if (err <= 0) {
-		perr(ys, "failed to receive the socket family info");
-		return err;
-	}
-	err = mnl_cb_run2(ys->rx_buf, err, ys->seq, ys->portid,
-			  ynl_get_family_info_cb, &yarg,
-			  ynl_cb_array, ARRAY_SIZE(ynl_cb_array));
+	err = ynl_sock_read_msgs(&yarg, ynl_get_family_info_cb);
 	if (err < 0) {
 		free(ys->mcast_groups);
 		perr(ys, "failed to receive the socket family info - no such family?");
@@ -755,7 +761,6 @@ static int ynl_ntf_trampoline(const struct nlmsghdr *nlh, void *data)
 int ynl_ntf_check(struct ynl_sock *ys)
 {
 	struct ynl_parse_arg yarg = { .ys = ys, };
-	ssize_t len;
 	int err;
 
 	do {
@@ -770,14 +775,7 @@ int ynl_ntf_check(struct ynl_sock *ys)
 		if (err < 1)
 			return err;
 
-		len = mnl_socket_recvfrom(ys->sock, ys->rx_buf,
-					  MNL_SOCKET_BUFFER_SIZE);
-		if (len < 0)
-			return len;
-
-		err = mnl_cb_run2(ys->rx_buf, len, ys->seq, ys->portid,
-				  ynl_ntf_trampoline, &yarg,
-				  ynl_cb_array, NLMSG_MIN_TYPE);
+		err = ynl_sock_read_msgs(&yarg, ynl_ntf_trampoline);
 		if (err < 0)
 			return err;
 	} while (err > 0);
@@ -834,7 +832,6 @@ static int ynl_req_trampoline(const struct nlmsghdr *nlh, void *data)
 int ynl_exec(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 	     struct ynl_req_state *yrs)
 {
-	ssize_t len;
 	int err;
 
 	err = mnl_socket_sendto(ys->sock, req_nlh, req_nlh->nlmsg_len);
@@ -842,19 +839,10 @@ int ynl_exec(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 		return err;
 
 	do {
-		len = mnl_socket_recvfrom(ys->sock, ys->rx_buf,
-					  MNL_SOCKET_BUFFER_SIZE);
-		if (len < 0)
-			return len;
-
-		err = mnl_cb_run2(ys->rx_buf, len, ys->seq, ys->portid,
-				  ynl_req_trampoline, yrs,
-				  ynl_cb_array, NLMSG_MIN_TYPE);
-		if (err < 0)
-			return err;
+		err = ynl_sock_read_msgs(&yrs->yarg, ynl_req_trampoline);
 	} while (err > 0);
 
-	return 0;
+	return err;
 }
 
 static int ynl_dump_trampoline(const struct nlmsghdr *nlh, void *data)
@@ -896,7 +884,6 @@ static void *ynl_dump_end(struct ynl_dump_state *ds)
 int ynl_exec_dump(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 		  struct ynl_dump_state *yds)
 {
-	ssize_t len;
 	int err;
 
 	err = mnl_socket_sendto(ys->sock, req_nlh, req_nlh->nlmsg_len);
@@ -904,14 +891,7 @@ int ynl_exec_dump(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 		return err;
 
 	do {
-		len = mnl_socket_recvfrom(ys->sock, ys->rx_buf,
-					  MNL_SOCKET_BUFFER_SIZE);
-		if (len < 0)
-			goto err_close_list;
-
-		err = mnl_cb_run2(ys->rx_buf, len, ys->seq, ys->portid,
-				  ynl_dump_trampoline, yds,
-				  ynl_cb_array, NLMSG_MIN_TYPE);
+		err = ynl_sock_read_msgs(&yds->yarg, ynl_dump_trampoline);
 		if (err < 0)
 			goto err_close_list;
 	} while (err > 0);
-- 
2.43.2

Re: [PATCH net-next 08/15] tools: ynl: wrap recv() + mnl_cb_run2() into a single helper

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> All callers to mnl_cb_run2() call mnl_socket_recvfrom() right before.
> Wrap the two in a helper, take typed arguments (struct ynl_parse_arg),
> instead of hoping that all callers remember that parser error handling
> requires yarg.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/net/ynl/lib/ynl.c | 56 +++++++++++++----------------------------
>  1 file changed, 18 insertions(+), 38 deletions(-)
> 
> diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> index c9790257189c..96a209773ace 100644
> --- a/tools/net/ynl/lib/ynl.c
> +++ b/tools/net/ynl/lib/ynl.c
> @@ -491,6 +491,19 @@ int ynl_cb_null(const struct nlmsghdr *nlh, void *data)
>  	return MNL_CB_ERROR;
>  }
>  
> +static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, mnl_cb_t cb)
> +{
> +	struct ynl_sock *ys = yarg->ys;
> +	ssize_t len;
> +
> +	len = mnl_socket_recvfrom(ys->sock, ys->rx_buf, MNL_SOCKET_BUFFER_SIZE);
> +	if (len < 0)
It was '<=' before your patch. Not sure it's worth running mnl_cb_run2() with no
data, but this doesn't seem wrong.
Maybe a sentence in the commit description will be good to tell it's done on
purpose?

Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 09/15] tools: ynl: use ynl_sock_read_msgs() for ACK handling

[Jakub Kicinski]

ynl_recv_ack() is simple and it's the only user of mnl_cb_run().
Now that ynl_sock_read_msgs() exists it's actually less code
to use ynl_sock_read_msgs() instead of being special.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h |  3 ---
 tools/net/ynl/lib/ynl.c      | 34 ++++++++++++++--------------------
 2 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 653119c9f47c..65880f879447 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -90,9 +90,6 @@ ynl_gemsg_start_dump(struct ynl_sock *ys, __u32 id, __u8 cmd, __u8 version);
 
 int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr);
 
-int ynl_recv_ack(struct ynl_sock *ys, int ret);
-int ynl_cb_null(const struct nlmsghdr *nlh, void *data);
-
 /* YNL specific helpers used by the auto-generated code */
 
 struct ynl_req_state {
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 96a209773ace..c01a971c251e 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -462,26 +462,7 @@ ynl_gemsg_start_dump(struct ynl_sock *ys, __u32 id, __u8 cmd, __u8 version)
 			       cmd, version);
 }
 
-int ynl_recv_ack(struct ynl_sock *ys, int ret)
-{
-	struct ynl_parse_arg yarg = { .ys = ys, };
-
-	if (!ret) {
-		yerr(ys, YNL_ERROR_EXPECT_ACK,
-		     "Expecting an ACK but nothing received");
-		return -1;
-	}
-
-	ret = mnl_socket_recvfrom(ys->sock, ys->rx_buf, MNL_SOCKET_BUFFER_SIZE);
-	if (ret < 0) {
-		perr(ys, "Socket receive failed");
-		return ret;
-	}
-	return mnl_cb_run(ys->rx_buf, ret, ys->seq, ys->portid,
-			  ynl_cb_null, &yarg);
-}
-
-int ynl_cb_null(const struct nlmsghdr *nlh, void *data)
+static int ynl_cb_null(const struct nlmsghdr *nlh, void *data)
 {
 	struct ynl_parse_arg *yarg = data;
 
@@ -504,6 +485,19 @@ static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, mnl_cb_t cb)
 			   cb, yarg, ynl_cb_array, NLMSG_MIN_TYPE);
 }
 
+static int ynl_recv_ack(struct ynl_sock *ys, int ret)
+{
+	struct ynl_parse_arg yarg = { .ys = ys, };
+
+	if (!ret) {
+		yerr(ys, YNL_ERROR_EXPECT_ACK,
+		     "Expecting an ACK but nothing received");
+		return -1;
+	}
+
+	return ynl_sock_read_msgs(&yarg, ynl_cb_null);
+}
+
 /* Init/fini and genetlink boiler plate */
 static int
 ynl_get_family_info_mcast(struct ynl_sock *ys, const struct nlattr *mcasts)
-- 
2.43.2

Re: [PATCH net-next 09/15] tools: ynl: use ynl_sock_read_msgs() for ACK handling

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> ynl_recv_ack() is simple and it's the only user of mnl_cb_run().
> Now that ynl_sock_read_msgs() exists it's actually less code
> to use ynl_sock_read_msgs() instead of being special.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 10/15] tools: ynl: stop using mnl_cb_run2()

[Jakub Kicinski]

There's only one set of callbacks in YNL, for netlink control
messages, and most of them are trivial. So implement the message
walking directly without depending on mnl_cb_run2().

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl.c | 66 +++++++++++++++++++++++++++++------------
 tools/net/ynl/lib/ynl.h |  1 +
 2 files changed, 48 insertions(+), 19 deletions(-)

diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index c01a971c251e..0f96ce948f75 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -255,10 +255,10 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 	return MNL_CB_OK;
 }
 
-static int ynl_cb_error(const struct nlmsghdr *nlh, void *data)
+static int
+ynl_cb_error(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 {
 	const struct nlmsgerr *err = ynl_nlmsg_data(nlh);
-	struct ynl_parse_arg *yarg = data;
 	unsigned int hlen;
 	int code;
 
@@ -275,9 +275,8 @@ static int ynl_cb_error(const struct nlmsghdr *nlh, void *data)
 	return code ? MNL_CB_ERROR : MNL_CB_STOP;
 }
 
-static int ynl_cb_done(const struct nlmsghdr *nlh, void *data)
+static int ynl_cb_done(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 {
-	struct ynl_parse_arg *yarg = data;
 	int err;
 
 	err = *(int *)NLMSG_DATA(nlh);
@@ -292,18 +291,6 @@ static int ynl_cb_done(const struct nlmsghdr *nlh, void *data)
 	return MNL_CB_STOP;
 }
 
-static int ynl_cb_noop(const struct nlmsghdr *nlh, void *data)
-{
-	return MNL_CB_OK;
-}
-
-static mnl_cb_t ynl_cb_array[NLMSG_MIN_TYPE] = {
-	[NLMSG_NOOP]	= ynl_cb_noop,
-	[NLMSG_ERROR]	= ynl_cb_error,
-	[NLMSG_DONE]	= ynl_cb_done,
-	[NLMSG_OVERRUN]	= ynl_cb_noop,
-};
-
 /* Attribute validation */
 
 int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
@@ -475,14 +462,55 @@ static int ynl_cb_null(const struct nlmsghdr *nlh, void *data)
 static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, mnl_cb_t cb)
 {
 	struct ynl_sock *ys = yarg->ys;
-	ssize_t len;
+	ssize_t len, rem;
+	int ret;
 
 	len = mnl_socket_recvfrom(ys->sock, ys->rx_buf, MNL_SOCKET_BUFFER_SIZE);
 	if (len < 0)
 		return len;
 
-	return mnl_cb_run2(ys->rx_buf, len, ys->seq, ys->portid,
-			   cb, yarg, ynl_cb_array, NLMSG_MIN_TYPE);
+	ret = MNL_CB_STOP;
+	for (rem = len; rem > 0;) {
+		const struct nlmsghdr *nlh;
+
+		nlh = (struct nlmsghdr *)&ys->rx_buf[len - rem];
+		if (!NLMSG_OK(nlh, rem)) {
+			yerr(yarg->ys, YNL_ERROR_INV_RESP,
+			     "Invalid message or trailing data in the response.");
+			return MNL_CB_ERROR;
+		}
+
+		if (nlh->nlmsg_flags & NLM_F_DUMP_INTR) {
+			/* TODO: handle this better */
+			yerr(yarg->ys, YNL_ERROR_DUMP_INTER,
+			     "Dump interrupted / inconsistent, please retry.");
+			return MNL_CB_ERROR;
+		}
+
+		switch (nlh->nlmsg_type) {
+		case 0:
+			yerr(yarg->ys, YNL_ERROR_INV_RESP,
+			     "Invalid message type in the response.");
+			return MNL_CB_ERROR;
+		case NLMSG_NOOP:
+		case NLMSG_OVERRUN ... NLMSG_MIN_TYPE - 1:
+			ret = MNL_CB_OK;
+			break;
+		case NLMSG_ERROR:
+			ret = ynl_cb_error(nlh, yarg);
+			break;
+		case NLMSG_DONE:
+			ret = ynl_cb_done(nlh, yarg);
+			break;
+		default:
+			ret = cb(nlh, yarg);
+			break;
+		}
+
+		NLMSG_NEXT(nlh, rem);
+	}
+
+	return ret;
 }
 
 static int ynl_recv_ack(struct ynl_sock *ys, int ret)
diff --git a/tools/net/ynl/lib/ynl.h b/tools/net/ynl/lib/ynl.h
index ce77a6d76ce0..4849c142fce0 100644
--- a/tools/net/ynl/lib/ynl.h
+++ b/tools/net/ynl/lib/ynl.h
@@ -12,6 +12,7 @@ enum ynl_error_code {
 	YNL_ERROR_NONE = 0,
 	__YNL_ERRNO_END = 4096,
 	YNL_ERROR_INTERNAL,
+	YNL_ERROR_DUMP_INTER,
 	YNL_ERROR_EXPECT_ACK,
 	YNL_ERROR_EXPECT_MSG,
 	YNL_ERROR_UNEXPECT_MSG,
-- 
2.43.2

Re: [PATCH net-next 10/15] tools: ynl: stop using mnl_cb_run2()

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> There's only one set of callbacks in YNL, for netlink control
> messages, and most of them are trivial. So implement the message
> walking directly without depending on mnl_cb_run2().
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/net/ynl/lib/ynl.c | 66 +++++++++++++++++++++++++++++------------
>  tools/net/ynl/lib/ynl.h |  1 +
>  2 files changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> index c01a971c251e..0f96ce948f75 100644
> --- a/tools/net/ynl/lib/ynl.c
> +++ b/tools/net/ynl/lib/ynl.c
> @@ -255,10 +255,10 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
>  	return MNL_CB_OK;
>  }
>  
> -static int ynl_cb_error(const struct nlmsghdr *nlh, void *data)
> +static int
> +ynl_cb_error(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
>  {
>  	const struct nlmsgerr *err = ynl_nlmsg_data(nlh);
> -	struct ynl_parse_arg *yarg = data;
>  	unsigned int hlen;
>  	int code;
>  
> @@ -275,9 +275,8 @@ static int ynl_cb_error(const struct nlmsghdr *nlh, void *data)
>  	return code ? MNL_CB_ERROR : MNL_CB_STOP;
>  }
>  
> -static int ynl_cb_done(const struct nlmsghdr *nlh, void *data)
> +static int ynl_cb_done(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
>  {
> -	struct ynl_parse_arg *yarg = data;
>  	int err;
>  
>  	err = *(int *)NLMSG_DATA(nlh);
> @@ -292,18 +291,6 @@ static int ynl_cb_done(const struct nlmsghdr *nlh, void *data)
>  	return MNL_CB_STOP;
>  }
>  
> -static int ynl_cb_noop(const struct nlmsghdr *nlh, void *data)
> -{
> -	return MNL_CB_OK;
> -}
> -
> -static mnl_cb_t ynl_cb_array[NLMSG_MIN_TYPE] = {
> -	[NLMSG_NOOP]	= ynl_cb_noop,
> -	[NLMSG_ERROR]	= ynl_cb_error,
> -	[NLMSG_DONE]	= ynl_cb_done,
> -	[NLMSG_OVERRUN]	= ynl_cb_noop,
> -};
> -
>  /* Attribute validation */
>  
>  int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr)
> @@ -475,14 +462,55 @@ static int ynl_cb_null(const struct nlmsghdr *nlh, void *data)
>  static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, mnl_cb_t cb)
>  {
>  	struct ynl_sock *ys = yarg->ys;
> -	ssize_t len;
> +	ssize_t len, rem;
> +	int ret;
>  
>  	len = mnl_socket_recvfrom(ys->sock, ys->rx_buf, MNL_SOCKET_BUFFER_SIZE);
>  	if (len < 0)
>  		return len;
>  
> -	return mnl_cb_run2(ys->rx_buf, len, ys->seq, ys->portid,
> -			   cb, yarg, ynl_cb_array, NLMSG_MIN_TYPE);
> +	ret = MNL_CB_STOP;
> +	for (rem = len; rem > 0;) {
Maybe this (if the nlh declaration is put above)?
for (rem = len; rem > 0; NLMSG_NEXT(nlh, rem)) {

> +		const struct nlmsghdr *nlh;
> +
> +		nlh = (struct nlmsghdr *)&ys->rx_buf[len - rem];
> +		if (!NLMSG_OK(nlh, rem)) {
> +			yerr(yarg->ys, YNL_ERROR_INV_RESP,
> +			     "Invalid message or trailing data in the response.");
> +			return MNL_CB_ERROR;
> +		}
> +
> +		if (nlh->nlmsg_flags & NLM_F_DUMP_INTR) {
> +			/* TODO: handle this better */
> +			yerr(yarg->ys, YNL_ERROR_DUMP_INTER,
> +			     "Dump interrupted / inconsistent, please retry.");
> +			return MNL_CB_ERROR;
> +		}
> +
> +		switch (nlh->nlmsg_type) {
> +		case 0:
> +			yerr(yarg->ys, YNL_ERROR_INV_RESP,
> +			     "Invalid message type in the response.");
> +			return MNL_CB_ERROR;
> +		case NLMSG_NOOP:
> +		case NLMSG_OVERRUN ... NLMSG_MIN_TYPE - 1:
I didn't know this was possible in C :D


> +			ret = MNL_CB_OK;
> +			break;
> +		case NLMSG_ERROR:
> +			ret = ynl_cb_error(nlh, yarg);
> +			break;
> +		case NLMSG_DONE:
> +			ret = ynl_cb_done(nlh, yarg);
> +			break;
> +		default:
> +			ret = cb(nlh, yarg);
> +			break;
> +		}
> +
> +		NLMSG_NEXT(nlh, rem);
> +	}
> +
> +	return ret;
>  }
>  
>  static int ynl_recv_ack(struct ynl_sock *ys, int ret)
> diff --git a/tools/net/ynl/lib/ynl.h b/tools/net/ynl/lib/ynl.h
> index ce77a6d76ce0..4849c142fce0 100644
> --- a/tools/net/ynl/lib/ynl.h
> +++ b/tools/net/ynl/lib/ynl.h
> @@ -12,6 +12,7 @@ enum ynl_error_code {
>  	YNL_ERROR_NONE = 0,
>  	__YNL_ERRNO_END = 4096,
>  	YNL_ERROR_INTERNAL,
> +	YNL_ERROR_DUMP_INTER,
>  	YNL_ERROR_EXPECT_ACK,
>  	YNL_ERROR_EXPECT_MSG,
>  	YNL_ERROR_UNEXPECT_MSG,

[PATCH net-next 11/15] tools: ynl: switch away from mnl_cb_t

[Jakub Kicinski]

All YNL parsing callbacks take struct ynl_parse_arg as the argument.
Make that official by using a local callback type instead of mnl_cb_t.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h | 11 ++++++++---
 tools/net/ynl/lib/ynl.c      | 25 ++++++++++++-------------
 tools/net/ynl/ynl-gen-c.py   |  3 +--
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 65880f879447..36523f3115c0 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -6,6 +6,8 @@
 #include <libmnl/libmnl.h>
 #include <linux/types.h>
 
+struct ynl_parse_arg;
+
 /*
  * YNL internals / low level stuff
  */
@@ -30,6 +32,9 @@ enum ynl_policy_type {
 #define YNL_ARRAY_SIZE(array)	(sizeof(array) ?			\
 				 sizeof(array) / sizeof(array[0]) : 0)
 
+typedef int (*ynl_parse_cb_t)(const struct nlmsghdr *nlh,
+			      struct ynl_parse_arg *yarg);
+
 struct ynl_policy_attr {
 	enum ynl_policy_type type;
 	unsigned int len;
@@ -94,7 +99,7 @@ int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr);
 
 struct ynl_req_state {
 	struct ynl_parse_arg yarg;
-	mnl_cb_t cb;
+	ynl_parse_cb_t cb;
 	__u32 rsp_cmd;
 };
 
@@ -103,13 +108,13 @@ struct ynl_dump_state {
 	void *first;
 	struct ynl_dump_list_type *last;
 	size_t alloc_sz;
-	mnl_cb_t cb;
+	ynl_parse_cb_t cb;
 	__u32 rsp_cmd;
 };
 
 struct ynl_ntf_info {
 	struct ynl_policy_nest *policy;
-	mnl_cb_t cb;
+	ynl_parse_cb_t cb;
 	size_t alloc_sz;
 	void (*free)(struct ynl_ntf_base_type *ntf);
 };
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 0f96ce948f75..5548cdc775e5 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -449,17 +449,15 @@ ynl_gemsg_start_dump(struct ynl_sock *ys, __u32 id, __u8 cmd, __u8 version)
 			       cmd, version);
 }
 
-static int ynl_cb_null(const struct nlmsghdr *nlh, void *data)
+static int ynl_cb_null(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 {
-	struct ynl_parse_arg *yarg = data;
-
 	yerr(yarg->ys, YNL_ERROR_UNEXPECT_MSG,
 	     "Received a message when none were expected");
 
 	return MNL_CB_ERROR;
 }
 
-static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, mnl_cb_t cb)
+static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, ynl_parse_cb_t cb)
 {
 	struct ynl_sock *ys = yarg->ys;
 	ssize_t len, rem;
@@ -561,9 +559,9 @@ ynl_get_family_info_mcast(struct ynl_sock *ys, const struct nlattr *mcasts)
 	return 0;
 }
 
-static int ynl_get_family_info_cb(const struct nlmsghdr *nlh, void *data)
+static int
+ynl_get_family_info_cb(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 {
-	struct ynl_parse_arg *yarg = data;
 	struct ynl_sock *ys = yarg->ys;
 	const struct nlattr *attr;
 	bool found_id = true;
@@ -773,10 +771,9 @@ static int ynl_ntf_parse(struct ynl_sock *ys, const struct nlmsghdr *nlh)
 	return MNL_CB_ERROR;
 }
 
-static int ynl_ntf_trampoline(const struct nlmsghdr *nlh, void *data)
+static int
+ynl_ntf_trampoline(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 {
-	struct ynl_parse_arg *yarg = data;
-
 	return ynl_ntf_parse(yarg->ys, nlh);
 }
 
@@ -839,9 +836,10 @@ ynl_check_alien(struct ynl_sock *ys, const struct nlmsghdr *nlh, __u32 rsp_cmd)
 	return 0;
 }
 
-static int ynl_req_trampoline(const struct nlmsghdr *nlh, void *data)
+static
+int ynl_req_trampoline(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 {
-	struct ynl_req_state *yrs = data;
+	struct ynl_req_state *yrs = (void *)yarg;
 	int ret;
 
 	ret = ynl_check_alien(yrs->yarg.ys, nlh, yrs->rsp_cmd);
@@ -867,9 +865,10 @@ int ynl_exec(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 	return err;
 }
 
-static int ynl_dump_trampoline(const struct nlmsghdr *nlh, void *data)
+static int
+ynl_dump_trampoline(const struct nlmsghdr *nlh, struct ynl_parse_arg *data)
 {
-	struct ynl_dump_state *ds = data;
+	struct ynl_dump_state *ds = (void *)data;
 	struct ynl_dump_list_type *obj;
 	struct ynl_parse_arg yarg = {};
 	int ret;
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 87f8b9d28734..7cc2b859d8de 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -1737,10 +1737,9 @@ _C_KW = {
         return
 
     func_args = ['const struct nlmsghdr *nlh',
-                 'void *data']
+                 'struct ynl_parse_arg *yarg']
 
     local_vars = [f'{type_name(ri, "reply", deref=deref)} *dst;',
-                  'struct ynl_parse_arg *yarg = data;',
                   'const struct nlattr *attr;']
     init_lines = ['dst = yarg->data;']
 
-- 
2.43.2

Re: [PATCH net-next 11/15] tools: ynl: switch away from mnl_cb_t

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> All YNL parsing callbacks take struct ynl_parse_arg as the argument.
> Make that official by using a local callback type instead of mnl_cb_t.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 12/15] tools: ynl: switch away from MNL_CB_*

[Jakub Kicinski]

Create a local version of the MNL_CB_* parser control values.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h |  6 ++++
 tools/net/ynl/lib/ynl.c      | 54 ++++++++++++++++++------------------
 tools/net/ynl/ynl-gen-c.py   | 14 +++++-----
 3 files changed, 40 insertions(+), 34 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 36523f3115c0..658768243d2f 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -29,6 +29,12 @@ enum ynl_policy_type {
 	YNL_PT_BITFIELD32,
 };
 
+enum ynl_parse_result {
+	YNL_PARSE_CB_ERROR = -1,
+	YNL_PARSE_CB_STOP = 0,
+	YNL_PARSE_CB_OK = 1,
+};
+
 #define YNL_ARRAY_SIZE(array)	(sizeof(array) ?			\
 				 sizeof(array) / sizeof(array[0]) : 0)
 
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 5548cdc775e5..cc0d9701b145 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -147,7 +147,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 
 	if (!(nlh->nlmsg_flags & NLM_F_ACK_TLVS)) {
 		yerr_msg(ys, "%s", strerror(ys->err.code));
-		return MNL_CB_OK;
+		return YNL_PARSE_CB_OK;
 	}
 
 	ynl_attr_for_each(attr, nlh, hlen) {
@@ -166,12 +166,12 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 		case NLMSGERR_ATTR_MISS_TYPE:
 		case NLMSGERR_ATTR_MISS_NEST:
 			if (len != sizeof(__u32))
-				return MNL_CB_ERROR;
+				return YNL_PARSE_CB_ERROR;
 			break;
 		case NLMSGERR_ATTR_MSG:
 			str = ynl_attr_data(attr);
 			if (str[len - 1])
-				return MNL_CB_ERROR;
+				return YNL_PARSE_CB_ERROR;
 			break;
 		default:
 			break;
@@ -252,7 +252,7 @@ ynl_ext_ack_check(struct ynl_sock *ys, const struct nlmsghdr *nlh,
 	else
 		yerr_msg(ys, "%s", strerror(ys->err.code));
 
-	return MNL_CB_OK;
+	return YNL_PARSE_CB_OK;
 }
 
 static int
@@ -272,7 +272,7 @@ ynl_cb_error(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 
 	ynl_ext_ack_check(yarg->ys, nlh, hlen);
 
-	return code ? MNL_CB_ERROR : MNL_CB_STOP;
+	return code ? YNL_PARSE_CB_ERROR : YNL_PARSE_CB_STOP;
 }
 
 static int ynl_cb_done(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
@@ -286,9 +286,9 @@ static int ynl_cb_done(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 
 		ynl_ext_ack_check(yarg->ys, nlh, sizeof(int));
 
-		return MNL_CB_ERROR;
+		return YNL_PARSE_CB_ERROR;
 	}
-	return MNL_CB_STOP;
+	return YNL_PARSE_CB_STOP;
 }
 
 /* Attribute validation */
@@ -454,7 +454,7 @@ static int ynl_cb_null(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 	yerr(yarg->ys, YNL_ERROR_UNEXPECT_MSG,
 	     "Received a message when none were expected");
 
-	return MNL_CB_ERROR;
+	return YNL_PARSE_CB_ERROR;
 }
 
 static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, ynl_parse_cb_t cb)
@@ -467,7 +467,7 @@ static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, ynl_parse_cb_t cb)
 	if (len < 0)
 		return len;
 
-	ret = MNL_CB_STOP;
+	ret = YNL_PARSE_CB_STOP;
 	for (rem = len; rem > 0;) {
 		const struct nlmsghdr *nlh;
 
@@ -475,24 +475,24 @@ static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, ynl_parse_cb_t cb)
 		if (!NLMSG_OK(nlh, rem)) {
 			yerr(yarg->ys, YNL_ERROR_INV_RESP,
 			     "Invalid message or trailing data in the response.");
-			return MNL_CB_ERROR;
+			return YNL_PARSE_CB_ERROR;
 		}
 
 		if (nlh->nlmsg_flags & NLM_F_DUMP_INTR) {
 			/* TODO: handle this better */
 			yerr(yarg->ys, YNL_ERROR_DUMP_INTER,
 			     "Dump interrupted / inconsistent, please retry.");
-			return MNL_CB_ERROR;
+			return YNL_PARSE_CB_ERROR;
 		}
 
 		switch (nlh->nlmsg_type) {
 		case 0:
 			yerr(yarg->ys, YNL_ERROR_INV_RESP,
 			     "Invalid message type in the response.");
-			return MNL_CB_ERROR;
+			return YNL_PARSE_CB_ERROR;
 		case NLMSG_NOOP:
 		case NLMSG_OVERRUN ... NLMSG_MIN_TYPE - 1:
-			ret = MNL_CB_OK;
+			ret = YNL_PARSE_CB_OK;
 			break;
 		case NLMSG_ERROR:
 			ret = ynl_cb_error(nlh, yarg);
@@ -540,7 +540,7 @@ ynl_get_family_info_mcast(struct ynl_sock *ys, const struct nlattr *mcasts)
 	ys->mcast_groups = calloc(ys->n_mcast_groups,
 				  sizeof(*ys->mcast_groups));
 	if (!ys->mcast_groups)
-		return MNL_CB_ERROR;
+		return YNL_PARSE_CB_ERROR;
 
 	i = 0;
 	ynl_attr_for_each_nested(entry, mcasts) {
@@ -569,14 +569,14 @@ ynl_get_family_info_cb(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 	ynl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
 		if (ynl_attr_type(attr) == CTRL_ATTR_MCAST_GROUPS)
 			if (ynl_get_family_info_mcast(ys, attr))
-				return MNL_CB_ERROR;
+				return YNL_PARSE_CB_ERROR;
 
 		if (ynl_attr_type(attr) != CTRL_ATTR_FAMILY_ID)
 			continue;
 
 		if (ynl_attr_data_len(attr) != sizeof(__u16)) {
 			yerr(ys, YNL_ERROR_ATTR_INVALID, "Invalid family ID");
-			return MNL_CB_ERROR;
+			return YNL_PARSE_CB_ERROR;
 		}
 
 		ys->family_id = ynl_attr_get_u16(attr);
@@ -585,9 +585,9 @@ ynl_get_family_info_cb(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 
 	if (!found_id) {
 		yerr(ys, YNL_ERROR_ATTR_MISSING, "Family ID missing");
-		return MNL_CB_ERROR;
+		return YNL_PARSE_CB_ERROR;
 	}
-	return MNL_CB_OK;
+	return YNL_PARSE_CB_OK;
 }
 
 static int ynl_sock_read_family(struct ynl_sock *ys, const char *family_name)
@@ -744,10 +744,10 @@ static int ynl_ntf_parse(struct ynl_sock *ys, const struct nlmsghdr *nlh)
 
 	gehdr = ynl_nlmsg_data(nlh);
 	if (gehdr->cmd >= ys->family->ntf_info_size)
-		return MNL_CB_ERROR;
+		return YNL_PARSE_CB_ERROR;
 	info = &ys->family->ntf_info[gehdr->cmd];
 	if (!info->cb)
-		return MNL_CB_ERROR;
+		return YNL_PARSE_CB_ERROR;
 
 	rsp = calloc(1, info->alloc_sz);
 	rsp->free = info->free;
@@ -755,7 +755,7 @@ static int ynl_ntf_parse(struct ynl_sock *ys, const struct nlmsghdr *nlh)
 	yarg.rsp_policy = info->policy;
 
 	ret = info->cb(nlh, &yarg);
-	if (ret <= MNL_CB_STOP)
+	if (ret <= YNL_PARSE_CB_STOP)
 		goto err_free;
 
 	rsp->family = nlh->nlmsg_type;
@@ -764,11 +764,11 @@ static int ynl_ntf_parse(struct ynl_sock *ys, const struct nlmsghdr *nlh)
 	*ys->ntf_last_next = rsp;
 	ys->ntf_last_next = &rsp->next;
 
-	return MNL_CB_OK;
+	return YNL_PARSE_CB_OK;
 
 err_free:
 	info->free(rsp);
-	return MNL_CB_ERROR;
+	return YNL_PARSE_CB_ERROR;
 }
 
 static int
@@ -815,7 +815,7 @@ void ynl_error_unknown_notification(struct ynl_sock *ys, __u8 cmd)
 int ynl_error_parse(struct ynl_parse_arg *yarg, const char *msg)
 {
 	yerr(yarg->ys, YNL_ERROR_INV_RESP, "Error parsing response: %s", msg);
-	return MNL_CB_ERROR;
+	return YNL_PARSE_CB_ERROR;
 }
 
 static int
@@ -844,7 +844,7 @@ int ynl_req_trampoline(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 
 	ret = ynl_check_alien(yrs->yarg.ys, nlh, yrs->rsp_cmd);
 	if (ret)
-		return ret < 0 ? MNL_CB_ERROR : MNL_CB_OK;
+		return ret < 0 ? YNL_PARSE_CB_ERROR : YNL_PARSE_CB_OK;
 
 	return yrs->cb(nlh, &yrs->yarg);
 }
@@ -875,11 +875,11 @@ ynl_dump_trampoline(const struct nlmsghdr *nlh, struct ynl_parse_arg *data)
 
 	ret = ynl_check_alien(ds->yarg.ys, nlh, ds->rsp_cmd);
 	if (ret)
-		return ret < 0 ? MNL_CB_ERROR : MNL_CB_OK;
+		return ret < 0 ? YNL_PARSE_CB_ERROR : YNL_PARSE_CB_OK;
 
 	obj = calloc(1, ds->alloc_sz);
 	if (!obj)
-		return MNL_CB_ERROR;
+		return YNL_PARSE_CB_ERROR;
 
 	if (!ds->first)
 		ds->first = obj;
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 7cc2b859d8de..f12e03d8753f 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -200,7 +200,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
 
         if not self.is_multi_val():
             ri.cw.p("if (ynl_attr_validate(yarg, attr))")
-            ri.cw.p("return MNL_CB_ERROR;")
+            ri.cw.p("return YNL_PARSE_CB_ERROR;")
             if self.presence_type() == 'bit':
                 ri.cw.p(f"{var}->_present.{self.c_name} = 1;")
 
@@ -247,7 +247,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         return []
 
     def _attr_get(self, ri, var):
-        return ['return MNL_CB_ERROR;'], None, None
+        return ['return YNL_PARSE_CB_ERROR;'], None, None
 
     def _attr_typol(self):
         return '.type = YNL_PT_REJECT, '
@@ -543,7 +543,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
 
     def _attr_get(self, ri, var):
         get_lines = [f"if ({self.nested_render_name}_parse(&parg, attr))",
-                     "return MNL_CB_ERROR;"]
+                     "return YNL_PARSE_CB_ERROR;"]
         init_lines = [f"parg.rsp_policy = &{self.nested_render_name}_nest;",
                       f"parg.data = &{var}->{self.c_name};"]
         return get_lines, init_lines, None
@@ -1674,7 +1674,7 @@ _C_KW = {
         ri.cw.block_start(line=f"ynl_attr_for_each_nested(attr, attr_{aspec.c_name})")
         ri.cw.p(f"parg.data = &dst->{aspec.c_name}[i];")
         ri.cw.p(f"if ({aspec.nested_render_name}_parse(&parg, attr, ynl_attr_type(attr)))")
-        ri.cw.p('return MNL_CB_ERROR;')
+        ri.cw.p('return YNL_PARSE_CB_ERROR;')
         ri.cw.p('i++;')
         ri.cw.block_end()
         ri.cw.block_end()
@@ -1693,7 +1693,7 @@ _C_KW = {
         if 'nested-attributes' in aspec:
             ri.cw.p(f"parg.data = &dst->{aspec.c_name}[i];")
             ri.cw.p(f"if ({aspec.nested_render_name}_parse(&parg, attr))")
-            ri.cw.p('return MNL_CB_ERROR;')
+            ri.cw.p('return YNL_PARSE_CB_ERROR;')
         elif aspec.type in scalars:
             ri.cw.p(f"dst->{aspec.c_name}[i] = ynl_attr_get_{aspec.type}(attr);")
         else:
@@ -1707,7 +1707,7 @@ _C_KW = {
     if struct.nested:
         ri.cw.p('return 0;')
     else:
-        ri.cw.p('return MNL_CB_OK;')
+        ri.cw.p('return YNL_PARSE_CB_OK;')
     ri.cw.block_end()
     ri.cw.nl()
 
@@ -1750,7 +1750,7 @@ _C_KW = {
     else:
         # Empty reply
         ri.cw.block_start()
-        ri.cw.p('return MNL_CB_OK;')
+        ri.cw.p('return YNL_PARSE_CB_OK;')
         ri.cw.block_end()
         ri.cw.nl()
 
-- 
2.43.2

Re: [PATCH net-next 12/15] tools: ynl: switch away from MNL_CB_*

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> Create a local version of the MNL_CB_* parser control values.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 13/15] tools: ynl: stop using mnl socket helpers

[Jakub Kicinski]

Most libmnl socket helpers can be replaced by direct calls to
the underlying libc API. We need portid, the netlink manpage
suggests we bind() address of zero.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h |  2 ++
 tools/net/ynl/lib/ynl.c      | 60 +++++++++++++++++++++++-------------
 tools/net/ynl/lib/ynl.h      |  2 +-
 3 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 658768243d2f..37720e505a11 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -35,6 +35,8 @@ enum ynl_parse_result {
 	YNL_PARSE_CB_OK = 1,
 };
 
+#define YNL_SOCKET_BUFFER_SIZE		(1 << 17)
+
 #define YNL_ARRAY_SIZE(array)	(sizeof(array) ?			\
 				 sizeof(array) / sizeof(array[0]) : 0)
 
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index cc0d9701b145..febb7581062d 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -3,10 +3,12 @@
 #include <poll.h>
 #include <string.h>
 #include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
 #include <linux/types.h>
-
 #include <libmnl/libmnl.h>
 #include <linux/genetlink.h>
+#include <sys/socket.h>
 
 #include "ynl.h"
 
@@ -463,7 +465,7 @@ static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, ynl_parse_cb_t cb)
 	ssize_t len, rem;
 	int ret;
 
-	len = mnl_socket_recvfrom(ys->sock, ys->rx_buf, MNL_SOCKET_BUFFER_SIZE);
+	len = recv(ys->socket, ys->rx_buf, YNL_SOCKET_BUFFER_SIZE, 0);
 	if (len < 0)
 		return len;
 
@@ -599,7 +601,7 @@ static int ynl_sock_read_family(struct ynl_sock *ys, const char *family_name)
 	nlh = ynl_gemsg_start_req(ys, GENL_ID_CTRL, CTRL_CMD_GETFAMILY, 1);
 	ynl_attr_put_strz(nlh, CTRL_ATTR_FAMILY_NAME, family_name);
 
-	err = mnl_socket_sendto(ys->sock, nlh, nlh->nlmsg_len);
+	err = send(ys->socket, nlh, nlh->nlmsg_len, 0);
 	if (err < 0) {
 		perr(ys, "failed to request socket family info");
 		return err;
@@ -624,38 +626,54 @@ static int ynl_sock_read_family(struct ynl_sock *ys, const char *family_name)
 struct ynl_sock *
 ynl_sock_create(const struct ynl_family *yf, struct ynl_error *yse)
 {
+	struct sockaddr_nl addr;
 	struct ynl_sock *ys;
+	socklen_t addrlen;
 	int one = 1;
 
-	ys = malloc(sizeof(*ys) + 2 * MNL_SOCKET_BUFFER_SIZE);
+	ys = malloc(sizeof(*ys) + 2 * YNL_SOCKET_BUFFER_SIZE);
 	if (!ys)
 		return NULL;
 	memset(ys, 0, sizeof(*ys));
 
 	ys->family = yf;
 	ys->tx_buf = &ys->raw_buf[0];
-	ys->rx_buf = &ys->raw_buf[MNL_SOCKET_BUFFER_SIZE];
+	ys->rx_buf = &ys->raw_buf[YNL_SOCKET_BUFFER_SIZE];
 	ys->ntf_last_next = &ys->ntf_first;
 
-	ys->sock = mnl_socket_open(NETLINK_GENERIC);
-	if (!ys->sock) {
+	ys->socket = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
+	if (ys->socket < 0) {
 		__perr(yse, "failed to create a netlink socket");
 		goto err_free_sock;
 	}
 
-	if (mnl_socket_setsockopt(ys->sock, NETLINK_CAP_ACK,
-				  &one, sizeof(one))) {
+	if (setsockopt(ys->socket, SOL_NETLINK, NETLINK_CAP_ACK,
+		       &one, sizeof(one))) {
 		__perr(yse, "failed to enable netlink ACK");
 		goto err_close_sock;
 	}
-	if (mnl_socket_setsockopt(ys->sock, NETLINK_EXT_ACK,
-				  &one, sizeof(one))) {
+	if (setsockopt(ys->socket, SOL_NETLINK, NETLINK_EXT_ACK,
+		       &one, sizeof(one))) {
 		__perr(yse, "failed to enable netlink ext ACK");
 		goto err_close_sock;
 	}
 
+	memset(&addr, 0, sizeof(addr));
+	addr.nl_family = AF_NETLINK;
+	if (bind(ys->socket, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
+		__perr(yse, "unable to bind to a socket address");
+		goto err_close_sock;;
+	}
+
+	memset(&addr, 0, sizeof(addr));
+	addrlen = sizeof(addr);
+	if (getsockname(ys->socket, (struct sockaddr *)&addr, &addrlen) < 0) {
+		__perr(yse, "unable to read socket address");
+		goto err_close_sock;;
+	}
+	ys->portid = addr.nl_pid;
 	ys->seq = random();
-	ys->portid = mnl_socket_get_portid(ys->sock);
+
 
 	if (ynl_sock_read_family(ys, yf->name)) {
 		if (yse)
@@ -666,7 +684,7 @@ ynl_sock_create(const struct ynl_family *yf, struct ynl_error *yse)
 	return ys;
 
 err_close_sock:
-	mnl_socket_close(ys->sock);
+	close(ys->socket);
 err_free_sock:
 	free(ys);
 	return NULL;
@@ -676,7 +694,7 @@ void ynl_sock_destroy(struct ynl_sock *ys)
 {
 	struct ynl_ntf_base_type *ntf;
 
-	mnl_socket_close(ys->sock);
+	close(ys->socket);
 	while ((ntf = ynl_ntf_dequeue(ys)))
 		ynl_ntf_free(ntf);
 	free(ys->mcast_groups);
@@ -703,9 +721,9 @@ int ynl_subscribe(struct ynl_sock *ys, const char *grp_name)
 		return -1;
 	}
 
-	err = mnl_socket_setsockopt(ys->sock, NETLINK_ADD_MEMBERSHIP,
-				    &ys->mcast_groups[i].id,
-				    sizeof(ys->mcast_groups[i].id));
+	err = setsockopt(ys->socket, SOL_NETLINK, NETLINK_ADD_MEMBERSHIP,
+			 &ys->mcast_groups[i].id,
+			 sizeof(ys->mcast_groups[i].id));
 	if (err < 0) {
 		perr(ys, "Subscribing to multicast group failed");
 		return -1;
@@ -716,7 +734,7 @@ int ynl_subscribe(struct ynl_sock *ys, const char *grp_name)
 
 int ynl_socket_get_fd(struct ynl_sock *ys)
 {
-	return mnl_socket_get_fd(ys->sock);
+	return ys->socket;
 }
 
 struct ynl_ntf_base_type *ynl_ntf_dequeue(struct ynl_sock *ys)
@@ -788,7 +806,7 @@ int ynl_ntf_check(struct ynl_sock *ys)
 		 */
 		struct pollfd pfd = { };
 
-		pfd.fd = mnl_socket_get_fd(ys->sock);
+		pfd.fd = ys->socket;
 		pfd.events = POLLIN;
 		err = poll(&pfd, 1, 1);
 		if (err < 1)
@@ -854,7 +872,7 @@ int ynl_exec(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 {
 	int err;
 
-	err = mnl_socket_sendto(ys->sock, req_nlh, req_nlh->nlmsg_len);
+	err = send(ys->socket, req_nlh, req_nlh->nlmsg_len, 0);
 	if (err < 0)
 		return err;
 
@@ -907,7 +925,7 @@ int ynl_exec_dump(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 {
 	int err;
 
-	err = mnl_socket_sendto(ys->sock, req_nlh, req_nlh->nlmsg_len);
+	err = send(ys->socket, req_nlh, req_nlh->nlmsg_len, 0);
 	if (err < 0)
 		return err;
 
diff --git a/tools/net/ynl/lib/ynl.h b/tools/net/ynl/lib/ynl.h
index 4849c142fce0..dbeeef8ce91a 100644
--- a/tools/net/ynl/lib/ynl.h
+++ b/tools/net/ynl/lib/ynl.h
@@ -59,7 +59,7 @@ struct ynl_sock {
 
 /* private: */
 	const struct ynl_family *family;
-	struct mnl_socket *sock;
+	int socket;
 	__u32 seq;
 	__u32 portid;
 	__u16 family_id;
-- 
2.43.2

Re: [PATCH net-next 13/15] tools: ynl: stop using mnl socket helpers

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> Most libmnl socket helpers can be replaced by direct calls to
> the underlying libc API. We need portid, the netlink manpage
> suggests we bind() address of zero.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 14/15] tools: ynl: remove the libmnl dependency

[Jakub Kicinski]

We don't use libmnl any more.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl-priv.h   | 4 +---
 tools/net/ynl/lib/ynl.c        | 1 -
 tools/net/ynl/samples/Makefile | 2 +-
 tools/net/ynl/ynl-gen-c.py     | 1 -
 4 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 37720e505a11..42b8d88ae587 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -2,8 +2,8 @@
 #ifndef __YNL_C_PRIV_H
 #define __YNL_C_PRIV_H 1
 
+#include <stdbool.h>
 #include <stddef.h>
-#include <libmnl/libmnl.h>
 #include <linux/types.h>
 
 struct ynl_parse_arg;
@@ -12,8 +12,6 @@ struct ynl_parse_arg;
  * YNL internals / low level stuff
  */
 
-/* Generic mnl helper code */
-
 enum ynl_policy_type {
 	YNL_PT_REJECT = 1,
 	YNL_PT_IGNORE,
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index febb7581062d..e9631226cd8d 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -6,7 +6,6 @@
 #include <stdio.h>
 #include <unistd.h>
 #include <linux/types.h>
-#include <libmnl/libmnl.h>
 #include <linux/genetlink.h>
 #include <sys/socket.h>
 
diff --git a/tools/net/ynl/samples/Makefile b/tools/net/ynl/samples/Makefile
index 28bdb1557a54..1d33e98e3ffe 100644
--- a/tools/net/ynl/samples/Makefile
+++ b/tools/net/ynl/samples/Makefile
@@ -9,7 +9,7 @@ ifeq ("$(DEBUG)","1")
   CFLAGS += -g -fsanitize=address -fsanitize=leak -static-libasan
 endif
 
-LDLIBS=-lmnl ../lib/ynl.a ../generated/protos.a
+LDLIBS=../lib/ynl.a ../generated/protos.a
 
 SRCS=$(wildcard *.c)
 BINS=$(patsubst %.c,%,${SRCS})
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index f12e03d8753f..9a58dba2ef1d 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -2677,7 +2677,6 @@ _C_KW = {
 
     if args.mode == "user":
         if not args.header:
-            cw.p("#include <libmnl/libmnl.h>")
             cw.p("#include <linux/genetlink.h>")
             cw.nl()
             for one in args.user_header:
-- 
2.43.2

Re: [PATCH net-next 14/15] tools: ynl: remove the libmnl dependency

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> We don't use libmnl any more.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH net-next 15/15] tools: ynl: use MSG_DONTWAIT for getting notifications

[Jakub Kicinski]

To stick to libmnl wrappers in the past we had to use poll()
to check if there are any outstanding notifications on the socket.
This is no longer necessary, we can use MSG_DONTWAIT.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/lib/ynl.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index e9631226cd8d..1739db19a37c 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -458,15 +458,19 @@ static int ynl_cb_null(const struct nlmsghdr *nlh, struct ynl_parse_arg *yarg)
 	return YNL_PARSE_CB_ERROR;
 }
 
-static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, ynl_parse_cb_t cb)
+static int
+__ynl_sock_read_msgs(struct ynl_parse_arg *yarg, ynl_parse_cb_t cb, int flags)
 {
 	struct ynl_sock *ys = yarg->ys;
 	ssize_t len, rem;
 	int ret;
 
-	len = recv(ys->socket, ys->rx_buf, YNL_SOCKET_BUFFER_SIZE, 0);
-	if (len < 0)
+	len = recv(ys->socket, ys->rx_buf, YNL_SOCKET_BUFFER_SIZE, flags);
+	if (len < 0) {
+		if (flags & MSG_DONTWAIT && errno == EAGAIN)
+			return YNL_PARSE_CB_STOP;
 		return len;
+	}
 
 	ret = YNL_PARSE_CB_STOP;
 	for (rem = len; rem > 0;) {
@@ -512,6 +516,11 @@ static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, ynl_parse_cb_t cb)
 	return ret;
 }
 
+static int ynl_sock_read_msgs(struct ynl_parse_arg *yarg, ynl_parse_cb_t cb)
+{
+	return __ynl_sock_read_msgs(yarg, cb, 0);
+}
+
 static int ynl_recv_ack(struct ynl_sock *ys, int ret)
 {
 	struct ynl_parse_arg yarg = { .ys = ys, };
@@ -800,18 +809,8 @@ int ynl_ntf_check(struct ynl_sock *ys)
 	int err;
 
 	do {
-		/* libmnl doesn't let us pass flags to the recv to make
-		 * it non-blocking so we need to poll() or peek() :|
-		 */
-		struct pollfd pfd = { };
-
-		pfd.fd = ys->socket;
-		pfd.events = POLLIN;
-		err = poll(&pfd, 1, 1);
-		if (err < 1)
-			return err;
-
-		err = ynl_sock_read_msgs(&yarg, ynl_ntf_trampoline);
+		err = __ynl_sock_read_msgs(&yarg, ynl_ntf_trampoline,
+					   MSG_DONTWAIT);
 		if (err < 0)
 			return err;
 	} while (err > 0);
-- 
2.43.2

Re: [PATCH net-next 15/15] tools: ynl: use MSG_DONTWAIT for getting notifications

[Nicolas Dichtel]

Le 23/02/2024 à 00:56, Jakub Kicinski a écrit :
> To stick to libmnl wrappers in the past we had to use poll()
> to check if there are any outstanding notifications on the socket.
> This is no longer necessary, we can use MSG_DONTWAIT.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Re: [PATCH net-next 00/15] tools: ynl: stop using libmnl

[Donald Hunter]

On Thu, 22 Feb 2024 at 23:56, Jakub Kicinski <kuba@kernel.org> wrote:
>
> There is no strong reason to stop using libmnl in ynl but there
> are a few small ones which add up.
>
> First, we do much more advanced netlink level parsing than libmnl
> in ynl so it's hard to say that libmnl abstracts much from us.
> The fact that this series, removing the libmnl dependency, only
> adds <300 LoC shows that code savings aren't huge.
> OTOH when new types are added (e.g. auto-int) we need to add
> compatibility to deal with older version of libmnl (in fact,
> even tho patches have been sent months ago, auto-ints are still
> not supported in libmnl.git).
>
> Second, the dependency makes ynl less self contained, and harder
> to vendor in. Whether vendoring libraries into projects is a good
> idea is a separate discussion, nonetheless, people want to do it.
>
> Third, there are small annoyances with the libmnl APIs which
> are hard to fix in backward-compatible ways.
>
> All in all, libmnl is a great library, but with all the code
> generation and structured parsing, ynl is better served by going
> its own way.

Is the absence of buffer bounds checking intentional, i.e. relying on libasan?

Re: [PATCH net-next 00/15] tools: ynl: stop using libmnl

[Jakub Kicinski]

On Fri, 23 Feb 2024 16:26:33 +0000 Donald Hunter wrote:
> Is the absence of buffer bounds checking intentional, i.e. relying on libasan?

In ynl.c or the generated code?

Re: [PATCH net-next 00/15] tools: ynl: stop using libmnl

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Fri, 23 Feb 2024 16:26:33 +0000 Donald Hunter wrote:
>> Is the absence of buffer bounds checking intentional, i.e. relying on libasan?
>
> In ynl.c or the generated code?

I'm looking at ynl_attr_nest_start() and ynl_attr_put*() in ynl-priv.h
and there's no checks for buffer overrun. It is admittedly a big
buffer, with rx following tx, but still.

Re: [PATCH net-next 00/15] tools: ynl: stop using libmnl

[Jakub Kicinski]

On Mon, 26 Feb 2024 09:04:12 +0000 Donald Hunter wrote:
> > On Fri, 23 Feb 2024 16:26:33 +0000 Donald Hunter wrote:  
> >> Is the absence of buffer bounds checking intentional, i.e. relying on libasan?  
> >
> > In ynl.c or the generated code?  
> 
> I'm looking at ynl_attr_nest_start() and ynl_attr_put*() in ynl-priv.h
> and there's no checks for buffer overrun. It is admittedly a big
> buffer, with rx following tx, but still.

You're right. But this series isn't making it worse, AFAIU.
We weren't checking before, we aren't checking now.

I don't want to have to add another arg to all put() calls.
How about we sash the max len on nlmsg_pid?

Something like:

diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
index 6361318e5c4c..d4ffe18b00f9 100644
--- a/tools/net/ynl/lib/ynl-priv.h
+++ b/tools/net/ynl/lib/ynl-priv.h
@@ -135,6 +135,8 @@ int ynl_error_parse(struct ynl_parse_arg *yarg, const char *msg);
 
 /* Netlink message handling helpers */
 
+#define YNL_MSG_OVERFLOW	1
+
 static inline struct nlmsghdr *ynl_nlmsg_put_header(void *buf)
 {
 	struct nlmsghdr *nlh = buf;
@@ -239,11 +241,26 @@ ynl_attr_first(const void *start, size_t len, size_t skip)
 	return ynl_attr_if_good(start + len, attr);
 }
 
+static inline bool
+__ynl_attr_put_overflow(struct nlmsghdr *nlh, size_t size)
+{
+	bool o;
+
+	/* We stash buffer length on nlmsg_pid. */
+	o = nlh->nlmsg_len + NLA_HDRLEN + NLMSG_ALIGN(size) > nlh->nlmsg_pid;
+	if (o)
+		nlh->nlmsg_pid = YNL_MSG_OVERFLOW;
+	return o;
+}
+
 static inline struct nlattr *
 ynl_attr_nest_start(struct nlmsghdr *nlh, unsigned int attr_type)
 {
 	struct nlattr *attr;
 
+	if (__ynl_attr_put_overflow(nlh, 0))
+		return ynl_nlmsg_end_addr(nlh) - NLA_HDRLEN;
+
 	attr = ynl_nlmsg_end_addr(nlh);
 	attr->nla_type = attr_type | NLA_F_NESTED;
 	nlh->nlmsg_len += NLMSG_ALIGN(sizeof(struct nlattr));
@@ -263,6 +280,9 @@ ynl_attr_put(struct nlmsghdr *nlh, unsigned int attr_type,
 {
 	struct nlattr *attr;
 
+	if (__ynl_attr_put_overflow(nlh, size))
+		return;
+
 	attr = ynl_nlmsg_end_addr(nlh);
 	attr->nla_type = attr_type;
 	attr->nla_len = NLA_HDRLEN + size;
@@ -276,14 +296,17 @@ static inline void
 ynl_attr_put_str(struct nlmsghdr *nlh, unsigned int attr_type, const char *str)
 {
 	struct nlattr *attr;
-	const char *end;
+	size_t len;
+
+	len = strlen(str);
+	if (__ynl_attr_put_overflow(nlh, len))
+		return;
 
 	attr = ynl_nlmsg_end_addr(nlh);
 	attr->nla_type = attr_type;
 
-	end = stpcpy(ynl_attr_data(attr), str);
-	attr->nla_len =
-		NLA_HDRLEN + NLA_ALIGN(end - (char *)ynl_attr_data(attr));
+	strcpy(ynl_attr_data(attr), str);
+	attr->nla_len = NLA_HDRLEN + NLA_ALIGN(len);
 
 	nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);
 }
diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
index 86729119e1ef..c2ba72f68028 100644
--- a/tools/net/ynl/lib/ynl.c
+++ b/tools/net/ynl/lib/ynl.c
@@ -404,9 +404,33 @@ struct nlmsghdr *ynl_msg_start(struct ynl_sock *ys, __u32 id, __u16 flags)
 	nlh->nlmsg_flags = flags;
 	nlh->nlmsg_seq = ++ys->seq;
 
+	/* This is a local YNL hack for length checking, we put the buffer
+	 * length in nlmsg_pid, since messages sent to the kernel always use
+	 * PID 0. Message needs to be terminated with ynl_msg_end().
+	 */
+	nlh->nlmsg_pid = YNL_SOCKET_BUFFER_SIZE;
+
 	return nlh;
 }
 
+static int ynl_msg_end(struct ynl_sock *ys, struct nlmsghdr *nlh)
+{
+	/* We stash buffer length on nlmsg_pid */
+	if (nlh->nlmsg_pid == 0) {
+		yerr(ys, YNL_ERROR_INPUT_INVALID,
+		     "Unknwon input buffer lenght");
+		return -EINVAL;
+	}
+	if (nlh->nlmsg_pid == YNL_MSG_OVERFLOW) {
+		yerr(ys, YNL_ERROR_INPUT_TOO_BIG,
+		     "Constructred message longer than internal buffer");
+		return -EMSGSIZE;
+	}
+
+	nlh->nlmsg_pid = 0;
+	return 0;
+}
+
 struct nlmsghdr *
 ynl_gemsg_start(struct ynl_sock *ys, __u32 id, __u16 flags,
 		__u8 cmd, __u8 version)
@@ -606,6 +630,10 @@ static int ynl_sock_read_family(struct ynl_sock *ys, const char *family_name)
 	nlh = ynl_gemsg_start_req(ys, GENL_ID_CTRL, CTRL_CMD_GETFAMILY, 1);
 	ynl_attr_put_str(nlh, CTRL_ATTR_FAMILY_NAME, family_name);
 
+	err = ynl_msg_end(ys, nlh);
+	if (err < 0)
+		return err;
+
 	err = send(ys->socket, nlh, nlh->nlmsg_len, 0);
 	if (err < 0) {
 		perr(ys, "failed to request socket family info");
@@ -867,6 +895,10 @@ int ynl_exec(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 {
 	int err;
 
+	err = ynl_msg_end(ys, req_nlh);
+	if (err < 0)
+		return err;
+
 	err = send(ys->socket, req_nlh, req_nlh->nlmsg_len, 0);
 	if (err < 0)
 		return err;
@@ -920,6 +952,10 @@ int ynl_exec_dump(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
 {
 	int err;
 
+	err = ynl_msg_end(ys, req_nlh);
+	if (err < 0)
+		return err;
+
 	err = send(ys->socket, req_nlh, req_nlh->nlmsg_len, 0);
 	if (err < 0)
 		return err;
diff --git a/tools/net/ynl/lib/ynl.h b/tools/net/ynl/lib/ynl.h
index dbeeef8ce91a..9842e85a8c57 100644
--- a/tools/net/ynl/lib/ynl.h
+++ b/tools/net/ynl/lib/ynl.h
@@ -20,6 +20,8 @@ enum ynl_error_code {
 	YNL_ERROR_ATTR_INVALID,
 	YNL_ERROR_UNKNOWN_NTF,
 	YNL_ERROR_INV_RESP,
+	YNL_ERROR_INPUT_INVALID,
+	YNL_ERROR_INPUT_TOO_BIG,
 };
 
 /**
-- 
2.43.2

Re: [PATCH net-next 00/15] tools: ynl: stop using libmnl

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Mon, 26 Feb 2024 09:04:12 +0000 Donald Hunter wrote:
>> > On Fri, 23 Feb 2024 16:26:33 +0000 Donald Hunter wrote:  
>> >> Is the absence of buffer bounds checking intentional, i.e. relying on libasan?  
>> >
>> > In ynl.c or the generated code?  
>> 
>> I'm looking at ynl_attr_nest_start() and ynl_attr_put*() in ynl-priv.h
>> and there's no checks for buffer overrun. It is admittedly a big
>> buffer, with rx following tx, but still.
>
> You're right. But this series isn't making it worse, AFAIU.
> We weren't checking before, we aren't checking now.

Agreed, libmnl had the same issue.

> I don't want to have to add another arg to all put() calls.
> How about we sash the max len on nlmsg_pid?

Seems reasonable. Minor comments below.

> Something like:
>
> diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
> index 6361318e5c4c..d4ffe18b00f9 100644
> --- a/tools/net/ynl/lib/ynl-priv.h
> +++ b/tools/net/ynl/lib/ynl-priv.h
> @@ -135,6 +135,8 @@ int ynl_error_parse(struct ynl_parse_arg *yarg, const char *msg);
>  
>  /* Netlink message handling helpers */
>  
> +#define YNL_MSG_OVERFLOW	1
> +
>  static inline struct nlmsghdr *ynl_nlmsg_put_header(void *buf)
>  {
>  	struct nlmsghdr *nlh = buf;
> @@ -239,11 +241,26 @@ ynl_attr_first(const void *start, size_t len, size_t skip)
>  	return ynl_attr_if_good(start + len, attr);
>  }
>  
> +static inline bool
> +__ynl_attr_put_overflow(struct nlmsghdr *nlh, size_t size)
> +{
> +	bool o;
> +
> +	/* We stash buffer length on nlmsg_pid. */
> +	o = nlh->nlmsg_len + NLA_HDRLEN + NLMSG_ALIGN(size) > nlh->nlmsg_pid;

The comment confused me here. How about "We compare against stashed buffer
length in nlmsg_pid".

> +	if (o)
> +		nlh->nlmsg_pid = YNL_MSG_OVERFLOW;

It took me a moment to realise that this behaves like a very short
buffer length for subsequent calls to __ynl_attr_put_overflow(). Is it
worth extending the comment in ynl_msg_start() to say "buffer length or
overflow status"?

> +	return o;
> +}
> +
>  static inline struct nlattr *
>  ynl_attr_nest_start(struct nlmsghdr *nlh, unsigned int attr_type)
>  {
>  	struct nlattr *attr;
>  
> +	if (__ynl_attr_put_overflow(nlh, 0))
> +		return ynl_nlmsg_end_addr(nlh) - NLA_HDRLEN;

Is the idea here to return a struct nlattr * that is safe to use?
Shouldn't we zero the values in the buffer first?

> +
>  	attr = ynl_nlmsg_end_addr(nlh);
>  	attr->nla_type = attr_type | NLA_F_NESTED;
>  	nlh->nlmsg_len += NLMSG_ALIGN(sizeof(struct nlattr));
> @@ -263,6 +280,9 @@ ynl_attr_put(struct nlmsghdr *nlh, unsigned int attr_type,
>  {
>  	struct nlattr *attr;
>  
> +	if (__ynl_attr_put_overflow(nlh, size))
> +		return;
> +
>  	attr = ynl_nlmsg_end_addr(nlh);
>  	attr->nla_type = attr_type;
>  	attr->nla_len = NLA_HDRLEN + size;
> @@ -276,14 +296,17 @@ static inline void
>  ynl_attr_put_str(struct nlmsghdr *nlh, unsigned int attr_type, const char *str)
>  {
>  	struct nlattr *attr;
> -	const char *end;
> +	size_t len;
> +
> +	len = strlen(str);
> +	if (__ynl_attr_put_overflow(nlh, len))
> +		return;
>  
>  	attr = ynl_nlmsg_end_addr(nlh);
>  	attr->nla_type = attr_type;
>  
> -	end = stpcpy(ynl_attr_data(attr), str);
> -	attr->nla_len =
> -		NLA_HDRLEN + NLA_ALIGN(end - (char *)ynl_attr_data(attr));
> +	strcpy(ynl_attr_data(attr), str);
> +	attr->nla_len = NLA_HDRLEN + NLA_ALIGN(len);
>  
>  	nlh->nlmsg_len += NLMSG_ALIGN(attr->nla_len);
>  }
> diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c
> index 86729119e1ef..c2ba72f68028 100644
> --- a/tools/net/ynl/lib/ynl.c
> +++ b/tools/net/ynl/lib/ynl.c
> @@ -404,9 +404,33 @@ struct nlmsghdr *ynl_msg_start(struct ynl_sock *ys, __u32 id, __u16 flags)
>  	nlh->nlmsg_flags = flags;
>  	nlh->nlmsg_seq = ++ys->seq;
>  
> +	/* This is a local YNL hack for length checking, we put the buffer
> +	 * length in nlmsg_pid, since messages sent to the kernel always use
> +	 * PID 0. Message needs to be terminated with ynl_msg_end().
> +	 */
> +	nlh->nlmsg_pid = YNL_SOCKET_BUFFER_SIZE;
> +
>  	return nlh;
>  }
>  
> +static int ynl_msg_end(struct ynl_sock *ys, struct nlmsghdr *nlh)
> +{
> +	/* We stash buffer length on nlmsg_pid */
> +	if (nlh->nlmsg_pid == 0) {
> +		yerr(ys, YNL_ERROR_INPUT_INVALID,
> +		     "Unknwon input buffer lenght");

Typo: lenght -> length

> +		return -EINVAL;
> +	}
> +	if (nlh->nlmsg_pid == YNL_MSG_OVERFLOW) {
> +		yerr(ys, YNL_ERROR_INPUT_TOO_BIG,
> +		     "Constructred message longer than internal buffer");
> +		return -EMSGSIZE;
> +	}
> +
> +	nlh->nlmsg_pid = 0;
> +	return 0;
> +}
> +
>  struct nlmsghdr *
>  ynl_gemsg_start(struct ynl_sock *ys, __u32 id, __u16 flags,
>  		__u8 cmd, __u8 version)
> @@ -606,6 +630,10 @@ static int ynl_sock_read_family(struct ynl_sock *ys, const char *family_name)
>  	nlh = ynl_gemsg_start_req(ys, GENL_ID_CTRL, CTRL_CMD_GETFAMILY, 1);
>  	ynl_attr_put_str(nlh, CTRL_ATTR_FAMILY_NAME, family_name);
>  
> +	err = ynl_msg_end(ys, nlh);
> +	if (err < 0)
> +		return err;
> +
>  	err = send(ys->socket, nlh, nlh->nlmsg_len, 0);
>  	if (err < 0) {
>  		perr(ys, "failed to request socket family info");
> @@ -867,6 +895,10 @@ int ynl_exec(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
>  {
>  	int err;
>  
> +	err = ynl_msg_end(ys, req_nlh);
> +	if (err < 0)
> +		return err;
> +
>  	err = send(ys->socket, req_nlh, req_nlh->nlmsg_len, 0);
>  	if (err < 0)
>  		return err;
> @@ -920,6 +952,10 @@ int ynl_exec_dump(struct ynl_sock *ys, struct nlmsghdr *req_nlh,
>  {
>  	int err;
>  
> +	err = ynl_msg_end(ys, req_nlh);
> +	if (err < 0)
> +		return err;
> +
>  	err = send(ys->socket, req_nlh, req_nlh->nlmsg_len, 0);
>  	if (err < 0)
>  		return err;
> diff --git a/tools/net/ynl/lib/ynl.h b/tools/net/ynl/lib/ynl.h
> index dbeeef8ce91a..9842e85a8c57 100644
> --- a/tools/net/ynl/lib/ynl.h
> +++ b/tools/net/ynl/lib/ynl.h
> @@ -20,6 +20,8 @@ enum ynl_error_code {
>  	YNL_ERROR_ATTR_INVALID,
>  	YNL_ERROR_UNKNOWN_NTF,
>  	YNL_ERROR_INV_RESP,
> +	YNL_ERROR_INPUT_INVALID,
> +	YNL_ERROR_INPUT_TOO_BIG,
>  };
>  
>  /**

Re: [PATCH net-next 00/15] tools: ynl: stop using libmnl

[Jakub Kicinski]

On Tue, 27 Feb 2024 10:49:42 +0000 Donald Hunter wrote:
> > +static inline bool
> > +__ynl_attr_put_overflow(struct nlmsghdr *nlh, size_t size)
> > +{
> > +	bool o;
> > +
> > +	/* We stash buffer length on nlmsg_pid. */
> > +	o = nlh->nlmsg_len + NLA_HDRLEN + NLMSG_ALIGN(size) > nlh->nlmsg_pid;  
> 
> The comment confused me here. How about "We compare against stashed buffer
> length in nlmsg_pid".

The comment should give context, rather than describe the code so how
about:

	/* ynl_msg_start() stashed buffer length in nlmsg_pid. */

> > +	if (o)
> > +		nlh->nlmsg_pid = YNL_MSG_OVERFLOW;  
> 
> It took me a moment to realise that this behaves like a very short
> buffer length for subsequent calls to __ynl_attr_put_overflow(). Is it
> worth extending the comment in ynl_msg_start() to say "buffer length or
> overflow status"?

Added:
		/* YNL_MSG_OVERFLOW is < NLMSG_HDRLEN, all subsequent checks
		 * are guaranteed to fail.
		 */
SG?

> > +	return o;
> > +}
> > +
> >  static inline struct nlattr *
> >  ynl_attr_nest_start(struct nlmsghdr *nlh, unsigned int attr_type)
> >  {
> >  	struct nlattr *attr;
> >  
> > +	if (__ynl_attr_put_overflow(nlh, 0))
> > +		return ynl_nlmsg_end_addr(nlh) - NLA_HDRLEN;  
> 
> Is the idea here to return a struct nlattr * that is safe to use?
> Shouldn't we zero the values in the buffer first?

The only thing that the attr is used for is to call ynl_attr_nest_end().
so I think zero init won't make any difference.

Re: [PATCH net-next 00/15] tools: ynl: stop using libmnl

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 27 Feb 2024 10:49:42 +0000 Donald Hunter wrote:
>> > +static inline bool
>> > +__ynl_attr_put_overflow(struct nlmsghdr *nlh, size_t size)
>> > +{
>> > +	bool o;
>> > +
>> > +	/* We stash buffer length on nlmsg_pid. */
>> > +	o = nlh->nlmsg_len + NLA_HDRLEN + NLMSG_ALIGN(size) > nlh->nlmsg_pid;  
>> 
>> The comment confused me here. How about "We compare against stashed buffer
>> length in nlmsg_pid".
>
> The comment should give context, rather than describe the code so how
> about:
>
> 	/* ynl_msg_start() stashed buffer length in nlmsg_pid. */

LGTM.

>> > +	if (o)
>> > +		nlh->nlmsg_pid = YNL_MSG_OVERFLOW;  
>> 
>> It took me a moment to realise that this behaves like a very short
>> buffer length for subsequent calls to __ynl_attr_put_overflow(). Is it
>> worth extending the comment in ynl_msg_start() to say "buffer length or
>> overflow status"?
>
> Added:
> 		/* YNL_MSG_OVERFLOW is < NLMSG_HDRLEN, all subsequent checks
> 		 * are guaranteed to fail.
> 		 */
> SG?

Perfect.

>> > +	return o;
>> > +}
>> > +
>> >  static inline struct nlattr *
>> >  ynl_attr_nest_start(struct nlmsghdr *nlh, unsigned int attr_type)
>> >  {
>> >  	struct nlattr *attr;
>> >  
>> > +	if (__ynl_attr_put_overflow(nlh, 0))
>> > +		return ynl_nlmsg_end_addr(nlh) - NLA_HDRLEN;  
>> 
>> Is the idea here to return a struct nlattr * that is safe to use?
>> Shouldn't we zero the values in the buffer first?
>
> The only thing that the attr is used for is to call ynl_attr_nest_end().
> so I think zero init won't make any difference.

I guess it is fine for generated code.