On Thu, Aug 03, 2023 at 12:48:20AM +0200, Stefano Brivio wrote:
> On Mon, 24 Jul 2023 16:09:36 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > We now detect errors on netlink "set" operations while configuring the
> > pasta namespace with --config-net.  However in many cases rather than
> > a simple "set" we use a more complex "dup" function to copy
> > configuration from the host to the namespace.  We're not yet properly
> > detecting and reporting netlink errors for that case.
> > 
> > Change the "dup" operations to propagate netlink errors to their
> > caller, pasta_ns_conf() and report them there.
> > 
> > Link: https://bugs.passt.top/show_bug.cgi?id=60
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  netlink.c | 40 ++++++++++++++++++++++++++++------------
> >  netlink.h |  8 ++++----
> >  pasta.c   | 15 ++++++++-------
> >  3 files changed, 40 insertions(+), 23 deletions(-)
> > 
> > diff --git a/netlink.c b/netlink.c
> > index 9e72b16..cdc18c0 100644
> > --- a/netlink.c
> > +++ b/netlink.c
> > @@ -413,9 +413,11 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, void *gw)
> >   * @s_dst:	Netlink socket in destination namespace
> >   * @ifi_dst:	Interface index in destination namespace
> >   * @af:		Address family
> > + *
> > + * Return: 0 on success, negative error code on failure
> >   */
> > -void nl_route_dup(int s_src, unsigned int ifi_src,
> > -		  int s_dst, unsigned int ifi_dst, sa_family_t af)
> > +int nl_route_dup(int s_src, unsigned int ifi_src,
> > +		 int s_dst, unsigned int ifi_dst, sa_family_t af)
> >  {
> >  	struct req_t {
> >  		struct nlmsghdr nlh;
> > @@ -477,9 +479,11 @@ void nl_route_dup(int s_src, unsigned int ifi_src,
> >  
> >  		if (extra) {
> >  			err("netlink: Too many routes to duplicate");
> > -			return;
> > +			return -E2BIG;
> 
> This is "Argument list too long", and... I don't have much better
> ideas. I would instinctively use ENOSPC or ENOMEM in this case, but
> both are slightly misleading in different ways, too.

Right.  My main reason for picking this is that it's an obscure error
that won't be generated by the underlying netlink operations so it's
unambiguous.  I'm open to better ideas.

> >  		}
> >  	}
> > +	if (status < 0)
> > +		return status;
> >  
> >  	/* Routes might have dependencies between each other, and the
> >  	 * kernel processes RTM_NEWROUTE messages sequentially. For n
> > @@ -494,15 +498,20 @@ void nl_route_dup(int s_src, unsigned int ifi_src,
> >  		     NLMSG_OK(nh, status);
> >  		     nh = NLMSG_NEXT(nh, status)) {
> >  			uint16_t flags = nh->nlmsg_flags;
> > +			int rc;
> >  
> >  			if (nh->nlmsg_type != RTM_NEWROUTE)
> >  				continue;
> >  
> > -			nl_do(s_dst, nh, RTM_NEWROUTE,
> > -			       (flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
> > -			       nh->nlmsg_len);
> > +			rc = nl_do(s_dst, nh, RTM_NEWROUTE,
> > +				   (flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
> > +				   nh->nlmsg_len);
> > +			if (rc < 0 && rc != -ENETUNREACH && rc != -EEXIST)
> > +				return rc;
> >  		}
> >  	}
> > +
> > +	return 0;
> >  }
> >  
> >  /**
> > @@ -635,9 +644,11 @@ int nl_addr_set(int s, unsigned int ifi, sa_family_t af,
> >   * @s_dst:	Netlink socket in destination network namespace
> >   * @ifi_dst:	Interface index in destination namespace
> >   * @af:		Address family
> > + *
> > + * Return: 0 on success, negative error code on failure
> >   */
> > -void nl_addr_dup(int s_src, unsigned int ifi_src,
> > -		 int s_dst, unsigned int ifi_dst, sa_family_t af)
> > +int nl_addr_dup(int s_src, unsigned int ifi_src,
> > +		int s_dst, unsigned int ifi_dst, sa_family_t af)
> >  {
> >  	struct req_t {
> >  		struct nlmsghdr nlh;
> > @@ -651,6 +662,7 @@ void nl_addr_dup(int s_src, unsigned int ifi_src,
> >  	struct nlmsghdr *nh;
> >  	ssize_t status;
> >  	uint16_t seq;
> > +	int rc= 0;
> 
> Missing whitespace.

Fixed.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson