Re: [PATCH 01/17] netlink: Split up functionality if nl_link()

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 3 Aug 2023 14:29:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Aug 03, 2023 at 12:09:16PM +1000, David Gibson wrote:
> > On Thu, Aug 03, 2023 at 12:47:29AM +0200, Stefano Brivio wrote:  
> [snip]
> > > > -void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
> > > > +void nl_link_get_mac(int ns, unsigned int ifi, void *mac)
> > > >  {
> > > > -	int change = !MAC_IS_ZERO(mac) || up || mtu;
> > > >  	struct req_t {
> > > >  		struct nlmsghdr nlh;
> > > >  		struct ifinfomsg ifm;
> > > > -		struct rtattr rta;
> > > > -		union {
> > > > -			unsigned char mac[ETH_ALEN];
> > > > -			struct {
> > > > -				unsigned int mtu;
> > > > -			} mtu;
> > > > -		} set;
> > > >  	} req = {
> > > > -		.nlh.nlmsg_type   = change ? RTM_NEWLINK : RTM_GETLINK,
> > > > -		.nlh.nlmsg_len    = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
> > > > -		.nlh.nlmsg_flags  = NLM_F_REQUEST | (change ? NLM_F_ACK : 0),
> > > > +		.nlh.nlmsg_type	  = RTM_GETLINK,
> > > > +		.nlh.nlmsg_len	  = sizeof(req),  
> > > 
> > > I don't think there's a practical issue with this, but there were two
> > > reasons why I used NLMSG_LENGTH(sizeof(struct ifinfomsg)) instead:
> > > 
> > > - NLMSG_LENGTH() aligns to 4 bytes, not to whatever
> > >   architecture-dependent alignment we might have: the message might
> > >   actually be smaller  
> > 
> > Oof... so.  On the one hand, I see the issue; if these are different,
> > I'm not sure what the effect will be.  On the other hand, if we use
> > NLMSG_LENGTH and it *is* longer than the structure size, we'll be
> > saying that this message is longer than the datagram containing it.
> > I'm not sure what the effect of that will be either.  
> 
> Duh, sorry, I realized I had this backwards.  NLSMSG_LENGTH() is the
> non-aligned length, sizeof() may include alignment.  I'll rework based
> on that understanding.

Right, I was about to write you that... or rather, NLMSG_LENGTH() is
the (presumably) lesser-aligned length. Also, if you check pretty much
any example in iproute2, nlmsg_len is always set like that, using
NLMSG_LENGTH() on the payload.

> > Not really sure what to do about this.
> >   
> > > - I see that this works with gcc and clang, but, strictly
> > >   speaking, is the size of the struct known "before"
> > >   (sequence-point-wise) we're done initialising it? I have a very vague
> > >   memory of this not working with gcc 2.9 or suchlike -- which is not a
> > >   problem, as long as our new friend C11 actually supports this (but
> > >   I'm not entirely sure).  
> > 
> > I'm pretty sure it's ok, regardless of C11 state.  It's not really a
> > question of sequence points: those are about the ordering of run time
> > operations.  Even though the structure is being defined inline,
> > determining it's size and layout will still happen at compile time,
> > whereas the initialization is obviously a runtime event.

Ah, sorry, yes, of course. Still I remember that failing spectacularly
in a distant past. But you just checked with gcc 4-ish I guess, so I
guess it would have been fine anyway.

-- 
Stefano