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

[Jakub Kicinski <kuba@kernel.org>]

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!