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

[yangerkun <yangerkun@huawei.com>]

在 2026/8/2 20:14, yu kuai 写道:
> 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.

OK, will do it next version!

> 
>> +
>>    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);
>