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

Hi,

=E5=9C=A8 2026/7/30 16:20, Yang Erkun =E5=86=99=E9=81=93:
> 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 (>=3D 0) or a negative
> errno; a callback >0 stops early as success (reconnect's -ENOSPC).
>
> Signed-off-by: Yang Erkun <[email protected]>
> ---
>   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 *inf=
o, struct nbd_device *nbd)
>   	return 0;
>   }
>  =20
> +/*
> + * 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 =3D 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) !=3D 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 =3D 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, s=
truct genl_info *info)
>   		}
>   	}
>  =20
> -	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) !=3D NBD_SOCK_ITEM) {
> -				pr_err("socks must be embedded in a SOCK_ITEM attr\n");
> -				ret =3D -EINVAL;
> -				goto out;
> -			}
> -			ret =3D nla_parse_nested_deprecated(socks, NBD_SOCK_MAX,
> -							  attr,
> -							  nbd_sock_policy,
> -							  info->extack);
> -			if (ret !=3D 0) {
> -				pr_err("error processing sock list\n");
> -				ret =3D -EINVAL;
> -				goto out;
> -			}
> -			if (!socks[NBD_SOCK_FD])
> -				continue;
> -			fd =3D (int)nla_get_u32(socks[NBD_SOCK_FD]);
> -			ret =3D nbd_add_socket(nbd, fd, true);
> -			if (ret)
> -				goto out;
> -		}
> -	}
> +	ret =3D nbd_genl_foreach_sock(info, nbd_genl_connect_sock_cb, nbd);
> +	if (ret < 0)
> +		goto out;
>  =20
>   	if (info->attrs[NBD_ATTR_BACKEND_IDENTIFIER]) {
>   		nbd->backend =3D 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;
>   }
>  =20
> +static int nbd_genl_reconnect_sock_cb(struct nbd_device *nbd, int fd)
> +{
> +	int ret =3D nbd_reconnect_socket(nbd, fd);
> +
> +	if (!ret) {
> +		dev_info(nbd_to_dev(nbd), "reconnected socket\n");
> +		return 0;
> +	}
> +
> +	if (ret =3D=3D -ENOSPC)
> +		return 1;
> +	return ret;
> +}

Since there is only one caller for nbd_reconnect_socket(), you might as wel=
l 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 =3D NULL;
> @@ -2441,40 +2480,10 @@ static int nbd_genl_reconfigure(struct sk_buff *s=
kb, struct genl_info *info)
>   		}
>   	}
>  =20
> -	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) !=3D NBD_SOCK_ITEM) {
> -				pr_err("socks must be embedded in a SOCK_ITEM attr\n");
> -				ret =3D -EINVAL;
> -				goto out;
> -			}
> -			ret =3D nla_parse_nested_deprecated(socks, NBD_SOCK_MAX,
> -							  attr,
> -							  nbd_sock_policy,
> -							  info->extack);
> -			if (ret !=3D 0) {
> -				pr_err("error processing sock list\n");
> -				ret =3D -EINVAL;
> -				goto out;
> -			}
> -			if (!socks[NBD_SOCK_FD])
> -				continue;
> -			fd =3D (int)nla_get_u32(socks[NBD_SOCK_FD]);
> -			ret =3D nbd_reconnect_socket(nbd, fd);
> -			if (ret) {
> -				if (ret =3D=3D -ENOSPC)
> -					ret =3D 0;
> -				goto out;
> -			}
> -			dev_info(nbd_to_dev(nbd), "reconnected socket\n");
> -		}
> -	}
> +	ret =3D 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 >=3D 0)
> +		ret =3D 0;
>   out:
>   	mutex_unlock(&nbd->config_lock);
>   	nbd_config_put(nbd);

--=20
Thanks,
Kuai