Re: [PATCH v11 21/26] block/rnbd: server: main functionality

[Bart Van Assche <bvanassche@acm.org>]

On 2020-03-20 05:16, Jack Wang wrote:
> +static int __read_mostly port_nr = RTRS_PORT;

Would uint16_t be sufficient for this kernel module parameter?

Is this kernel module parameter used anywhere in the hot path? If not,
should __read_mostly perhaps be left out?

> +module_param_named(port_nr, port_nr, int, 0444);
> +MODULE_PARM_DESC(port_nr,
> +		 "The port number server is listening on (default: "
                                ^^^
                                the?
> +		 __stringify(RTRS_PORT)")");
> +
> +#define DEFAULT_DEV_SEARCH_PATH "/"

> +static void destroy_device(struct rnbd_srv_dev *dev)
> +{
> +	WARN(!list_empty(&dev->sess_dev_list),
> +	     "Device %s is being destroyed but still in use!\n",
> +	     dev->id);

Has it been considered to change WARN() into WARN_ONCE()?

> +static int rnbd_srv_rdma_ev(struct rtrs_srv *rtrs, void *priv,
> +			     struct rtrs_srv_op *id, int dir,
> +			     void *data, size_t datalen, const void *usr,
> +			     size_t usrlen)
> +{
> +	struct rnbd_srv_session *srv_sess = priv;
> +	const struct rnbd_msg_hdr *hdr = usr;
> +	int ret = 0;
> +	u16 type;
> +
> +	if (WARN_ON(!srv_sess))
> +		return -ENODEV;

Same question here: how about using WARN_ON_ONCE() instead of WARN_ON()?

> +static char *rnbd_srv_get_full_path(struct rnbd_srv_session *srv_sess,
> +				     const char *dev_name)
> +{
> +	char *full_path;
> +	char *a, *b;
> +
> +	full_path = kmalloc(PATH_MAX, GFP_KERNEL);
> +	if (!full_path)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/*
> +	 * Replace %SESSNAME% with a real session name in order to
> +	 * create device namespace.
> +	 */
> +	a = strnstr(dev_search_path, "%SESSNAME%", sizeof(dev_search_path));
> +	if (a) {
> +		int len = a - dev_search_path;
> +
> +		len = snprintf(full_path, PATH_MAX, "%.*s/%s/%s", len,
> +			       dev_search_path, srv_sess->sessname, dev_name);
> +		if (len >= PATH_MAX) {
> +			pr_err("Tooooo looong path: %s, %s, %s\n",
> +			       dev_search_path, srv_sess->sessname, dev_name);
> +			kfree(full_path);
> +			return ERR_PTR(-EINVAL);
> +		}
> +	} else {
> +		snprintf(full_path, PATH_MAX, "%s/%s",
> +			 dev_search_path, dev_name);
> +	}

Has it been considered to use kasprintf() instead of kmalloc() + snprintf()?

Otherwise this patch looks fine to me.

Thanks,

Bart.