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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240223064650.7e7b5975@kernel.org>
Date: Fri, 23 Feb 2024 06:46:50 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, jiri@...nulli.us, sdf@...gle.com,
 donald.hunter@...il.com
Subject: Re: [PATCH net-next 02/15] tools: ynl: create local attribute
 helpers

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!