Re: [PATCH v5 6/8] nbd: factor out a nbd_genl_foreach_sock

["yu kuai" <yukuai@fygo.io>]

Hi,

在 2026/7/30 16:20, Yang Erkun 写道:
> The NBD_ATTR_SOCKETS walk is duplicated in nbd_genl_connect (add sockets)
> and nbd_genl_reconfigure (reconnect).  Factor out a single helper that
> walks the list and calls a callback per fd; with a NULL callback it is a
> pure counter, used by a later patch to learn nr_hw_queues before the
> device exists.  Returns the number of fds walked (>= 0) or a negative
> errno; a callback >0 stops early as success (reconnect's -ENOSPC).
>
> Signed-off-by: Yang Erkun <yangerkun@huawei.com>
> ---
>   drivers/block/nbd.c | 137 +++++++++++++++++++++++---------------------
>   1 file changed, 73 insertions(+), 64 deletions(-)

This patch LGTM, two nits below.

>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 3b7363b11d0b..34b84fc1c61f 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -2108,6 +2108,58 @@ static int nbd_genl_size_set(struct genl_info *info, struct nbd_device *nbd)
>   	return 0;
>   }
>   
> +/*
> + * Walk the NBD_ATTR_SOCKETS nested list can call @cb for each socket fd.
> + *
> + * Return the number of fds walked, or a negative errno.
> + */
> +static int nbd_genl_foreach_sock(struct genl_info *info,
> +		int (*cb)(struct nbd_device *nbd, int fd),
> +		struct nbd_device *nbd)
> +{
> +	struct nlattr *attr;
> +	int rem, count = 0;
> +
> +	if (!info->attrs[NBD_ATTR_SOCKETS])
> +		return 0;
> +
> +	nla_for_each_nested(attr, info->attrs[NBD_ATTR_SOCKETS], rem) {
> +		struct nlattr *socks[NBD_SOCK_MAX + 1];
> +		int ret;
> +
> +		if (nla_type(attr) != NBD_SOCK_ITEM) {
> +			pr_err("socks must be embedded in a SOCK_ITEM attr\n");
> +			return -EINVAL;
> +		}
> +
> +		if (nla_parse_nested_deprecated(socks, NBD_SOCK_MAX,
> +						attr,
> +						nbd_sock_policy,
> +						info->extack)) {
> +			pr_err("error processing sock list\n");
> +			return -EINVAL;
> +		}
> +
> +		if (!socks[NBD_SOCK_FD])
> +			continue;
> +
> +		count++;
> +		if (cb) {
> +			ret = cb(nbd, (int)nla_get_u32(socks[NBD_SOCK_FD]));
> +			if (ret > 0)
> +				return count;
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +	return count;
> +}
> +
> +static int nbd_genl_connect_sock_cb(struct nbd_device *nbd, int fd)
> +{
> +	return nbd_add_socket(nbd, fd, true);
> +}
> +
>   static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
>   {
>   	struct nbd_device *nbd;
> @@ -2227,36 +2279,9 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
>   		}
>   	}
>   
> -	if (info->attrs[NBD_ATTR_SOCKETS]) {
> -		struct nlattr *attr;
> -		int rem, fd;
> -
> -		nla_for_each_nested(attr, info->attrs[NBD_ATTR_SOCKETS],
> -				    rem) {
> -			struct nlattr *socks[NBD_SOCK_MAX+1];
> -
> -			if (nla_type(attr) != NBD_SOCK_ITEM) {
> -				pr_err("socks must be embedded in a SOCK_ITEM attr\n");
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			ret = nla_parse_nested_deprecated(socks, NBD_SOCK_MAX,
> -							  attr,
> -							  nbd_sock_policy,
> -							  info->extack);
> -			if (ret != 0) {
> -				pr_err("error processing sock list\n");
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			if (!socks[NBD_SOCK_FD])
> -				continue;
> -			fd = (int)nla_get_u32(socks[NBD_SOCK_FD]);
> -			ret = nbd_add_socket(nbd, fd, true);
> -			if (ret)
> -				goto out;
> -		}
> -	}
> +	ret = nbd_genl_foreach_sock(info, nbd_genl_connect_sock_cb, nbd);
> +	if (ret < 0)
> +		goto out;
>   
>   	if (info->attrs[NBD_ATTR_BACKEND_IDENTIFIER]) {
>   		nbd->backend = nla_strdup(info->attrs[NBD_ATTR_BACKEND_IDENTIFIER],
> @@ -2345,6 +2370,20 @@ static int nbd_genl_disconnect(struct sk_buff *skb, struct genl_info *info)
>   	return 0;
>   }
>   
> +static int nbd_genl_reconnect_sock_cb(struct nbd_device *nbd, int fd)
> +{
> +	int ret = nbd_reconnect_socket(nbd, fd);
> +
> +	if (!ret) {
> +		dev_info(nbd_to_dev(nbd), "reconnected socket\n");
> +		return 0;
> +	}
> +
> +	if (ret == -ENOSPC)
> +		return 1;
> +	return ret;
> +}

Since there is only one caller for nbd_reconnect_socket(), you might as well just fold above
changes into nbd_reconnect_socket() directly.

> +
>   static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
>   {
>   	struct nbd_device *nbd = NULL;
> @@ -2441,40 +2480,10 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
>   		}
>   	}
>   
> -	if (info->attrs[NBD_ATTR_SOCKETS]) {
> -		struct nlattr *attr;
> -		int rem, fd;
> -
> -		nla_for_each_nested(attr, info->attrs[NBD_ATTR_SOCKETS],
> -				    rem) {
> -			struct nlattr *socks[NBD_SOCK_MAX+1];
> -
> -			if (nla_type(attr) != NBD_SOCK_ITEM) {
> -				pr_err("socks must be embedded in a SOCK_ITEM attr\n");
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			ret = nla_parse_nested_deprecated(socks, NBD_SOCK_MAX,
> -							  attr,
> -							  nbd_sock_policy,
> -							  info->extack);
> -			if (ret != 0) {
> -				pr_err("error processing sock list\n");
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -			if (!socks[NBD_SOCK_FD])
> -				continue;
> -			fd = (int)nla_get_u32(socks[NBD_SOCK_FD]);
> -			ret = nbd_reconnect_socket(nbd, fd);
> -			if (ret) {
> -				if (ret == -ENOSPC)
> -					ret = 0;
> -				goto out;
> -			}
> -			dev_info(nbd_to_dev(nbd), "reconnected socket\n");
> -		}
> -	}
> +	ret = nbd_genl_foreach_sock(info, nbd_genl_reconnect_sock_cb, nbd);
> +	/* foreach_sock returns a positive count on success; doit must return 0 */
> +	if (ret >= 0)
> +		ret = 0;
>   out:
>   	mutex_unlock(&nbd->config_lock);
>   	nbd_config_put(nbd);

-- 
Thanks,
Kuai