Re: [PATCH 17/17] netlink: Propagate errors for "dup" operations

[Stefano Brivio <sbrivio@redhat.com>]

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.

>  		}
>  	}
> +	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.

-- 
Stefano