Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
From: Bart Van Assche <bvanassche@acm.org>
To: Jack Wang <jinpuwang@gmail.com>,
	linux-block@vger.kernel.org, linux-rdma@vger.kernel.org
Cc: axboe@kernel.dk, hch@infradead.org, sagi@grimberg.me,
	jgg@mellanox.com, dledford@redhat.com,
	danil.kipnis@cloud.ionos.com, rpenyaev@suse.de,
	Jack Wang <jinpu.wang@cloud.ionos.com>
Subject: Re: [PATCH v4 18/25] ibnbd: client: sysfs interface functions
Date: Wed, 18 Sep 2019 09:28:37 -0700
Message-ID: <05192413-4b0c-76fd-7459-bc6d430b46a6@acm.org> (raw)
In-Reply-To: <20190620150337.7847-19-jinpuwang@gmail.com>

On 6/20/19 8:03 AM, Jack Wang wrote:
> +#undef pr_fmt
> +#define pr_fmt(fmt) KBUILD_MODNAME " L" __stringify(__LINE__) ": " fmt

Including the line number in all messages is too much information. 
Please don't do this. Additionally, this will make the line number occur 
twice in messages produced by pr_debug().

> +static unsigned int ibnbd_opt_mandatory[] = {
> +	IBNBD_OPT_PATH,
> +	IBNBD_OPT_DEV_PATH,
> +	IBNBD_OPT_SESSNAME,
> +};

Should this array have been declared const?

 > +/* remove new line from string */
 > +static void strip(char *s)
 > +{
 > +	char *p = s;
 > +
 > +	while (*s != '\0') {
 > +		if (*s != '\n')
 > +			*p++ = *s++;
 > +		else
 > +			++s;
 > +	}
 > +	*p = '\0';
 > +}

This function can remove a newline from the middle of a string. Are you 
sure that's what you want?

Is it useful to strip newline characters only and to keep other 
whitespace? Could this function be dropped and can the callers use 
strim() instead?

> +static int ibnbd_clt_parse_map_options(const char *buf,
> +				       char *sessname,
> +				       struct ibtrs_addr *paths,
> +				       size_t *path_cnt,
> +				       size_t max_path_cnt,
> +				       char *pathname,
> +				       enum ibnbd_access_mode *access_mode,
> +				       enum ibnbd_io_mode *io_mode)
> +{

Please introduce a structure for all the output parameters of this 
function and pass a pointer to that structure to this function. That 
will make it easier to introduce support for new parameters.

> +	char *options, *sep_opt;
> +	char *p;
> +	substring_t args[MAX_OPT_ARGS];
> +	int opt_mask = 0;
> +	int token;
> +	int ret = -EINVAL;
> +	int i;
> +	int p_cnt = 0;
> +
> +	options = kstrdup(buf, GFP_KERNEL);
> +	if (!options)
> +		return -ENOMEM;
> +
> +	sep_opt = strstrip(options);
> +	strip(sep_opt);

Are you sure that strstrip() does not remove trailing newline characters?

> +	while ((p = strsep(&sep_opt, " ")) != NULL) {
> +		if (!*p)
> +			continue;
> +
> +		token = match_token(p, ibnbd_opt_tokens, args);
> +		opt_mask |= token;
> +
> +		switch (token) {
> +		case IBNBD_OPT_SESSNAME:
> +			p = match_strdup(args);
> +			if (!p) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +			if (strlen(p) > NAME_MAX) {
> +				pr_err("map_device: sessname too long\n");
> +				ret = -EINVAL;
> +				kfree(p);
> +				goto out;
> +			}
> +			strlcpy(sessname, p, NAME_MAX);
> +			kfree(p);
> +			break;

Please change sessname from a fixed size buffer into a dynamically 
allocated buffer. That will remove the need to perform a strlcpy() and 
will also allow to remove the NAME_MAX checks.

> +		case IBNBD_OPT_DEV_PATH:
> +			p = match_strdup(args);
> +			if (!p) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +			if (strlen(p) > NAME_MAX) {
> +				pr_err("map_device: Device path too long\n");
> +				ret = -EINVAL;
> +				kfree(p);
> +				goto out;
> +			}
> +			strlcpy(pathname, p, NAME_MAX);
> +			kfree(p);
> +			break;

Same comment here - please change pathname from a fixed-size array into 
a dynamically allocated buffer.

> +static ssize_t ibnbd_clt_state_show(struct kobject *kobj,
> +				    struct kobj_attribute *attr, char *page)
> +{
> +	struct ibnbd_clt_dev *dev;
> +
> +	dev = container_of(kobj, struct ibnbd_clt_dev, kobj);
> +
> +	switch (dev->dev_state) {
> +	case (DEV_STATE_INIT):
> +		return scnprintf(page, PAGE_SIZE, "init\n");
> +	case (DEV_STATE_MAPPED):
> +		/* TODO fix cli tool before changing to proper state */
> +		return scnprintf(page, PAGE_SIZE, "open\n");
> +	case (DEV_STATE_MAPPED_DISCONNECTED):
> +		/* TODO fix cli tool before changing to proper state */
> +		return scnprintf(page, PAGE_SIZE, "closed\n");
> +	case (DEV_STATE_UNMAPPED):
> +		return scnprintf(page, PAGE_SIZE, "unmapped\n");
> +	default:
> +		return scnprintf(page, PAGE_SIZE, "unknown\n");
> +	}
> +}

Please remove the superfluous parentheses from around the DEV_STATE_* 
constants.

Additionally, using scnprintf() here is overkill. snprintf() should be 
sufficient.

> +static struct kobj_attribute ibnbd_clt_state_attr =
> +	__ATTR(state, 0444, ibnbd_clt_state_show, NULL);

Please use DEVICE_ATTR_RO() instead of __ATTR() for all read-only 
attributes.

> +static ssize_t ibnbd_clt_unmap_dev_store(struct kobject *kobj,
> +					 struct kobj_attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	struct ibnbd_clt_dev *dev;
> +	char *opt, *options;
> +	bool force;
> +	int err;
> +
> +	opt = kstrdup(buf, GFP_KERNEL);
> +	if (!opt)
> +		return -ENOMEM;
> +
> +	options = strstrip(opt);
> +	strip(options);
> +
> +	dev = container_of(kobj, struct ibnbd_clt_dev, kobj);
> +
> +	if (sysfs_streq(options, "normal")) {
> +		force = false;
> +	} else if (sysfs_streq(options, "force")) {
> +		force = true;
> +	} else {
> +		ibnbd_err(dev, "unmap_device: Invalid value: %s\n", options);
> +		err = -EINVAL;
> +		goto out;
> +	}

Wasn't sysfs_streq() introduced to avoid having to duplicate and strip 
the input string?

> +	/*
> +	 * We take explicit module reference only for one reason: do not
> +	 * race with lockless ibnbd_destroy_sessions().
> +	 */
> +	if (!try_module_get(THIS_MODULE)) {
> +		err = -ENODEV;
> +		goto out;
> +	}
> +	err = ibnbd_clt_unmap_device(dev, force, &attr->attr);
> +	if (unlikely(err)) {
> +		if (unlikely(err != -EALREADY))
> +			ibnbd_err(dev, "unmap_device: %d\n",  err);
> +		goto module_put;
> +	}
> +
> +	/*
> +	 * Here device can be vanished!
> +	 */
> +
> +	err = count;
> +
> +module_put:
> +	module_put(THIS_MODULE);

I've never before seen a module_get() / module_put() pair inside a sysfs 
  callback function. Can this race be fixed by making 
ibnbd_destroy_sessions() remove this sysfs attribute before it tries to 
destroy any sessions?

> +void ibnbd_clt_remove_dev_symlink(struct ibnbd_clt_dev *dev)
> +{
> +	/*
> +	 * The module_is_live() check is crucial and helps to avoid annoying
> +	 * sysfs warning raised in sysfs_remove_link(), when the whole sysfs
> +	 * path was just removed, see ibnbd_close_sessions().
> +	 */
> +	if (strlen(dev->blk_symlink_name) && module_is_live(THIS_MODULE))
> +		sysfs_remove_link(ibnbd_devs_kobj, dev->blk_symlink_name);
> +}

I haven't been able to find any other sysfs code that calls 
module_is_live()? Please elaborate why that check is needed.

> +int ibnbd_clt_create_sysfs_files(void)
> +{
> +	int err;
> +
> +	ibnbd_dev_class = class_create(THIS_MODULE, "ibnbd-client");
> +	if (unlikely(IS_ERR(ibnbd_dev_class)))
> +		return PTR_ERR(ibnbd_dev_class);
> +
> +	ibnbd_dev = device_create(ibnbd_dev_class, NULL,
> +				  MKDEV(0, 0), NULL, "ctl");
> +	if (unlikely(IS_ERR(ibnbd_dev))) {
> +		err = PTR_ERR(ibnbd_dev);
> +		goto cls_destroy;
> +	}
> +	ibnbd_devs_kobj = kobject_create_and_add("devices", &ibnbd_dev->kobj);
> +	if (unlikely(!ibnbd_devs_kobj)) {
> +		err = -ENOMEM;
> +		goto dev_destroy;
> +	}
> +	err = sysfs_create_group(&ibnbd_dev->kobj, &default_attr_group);
> +	if (unlikely(err))
> +		goto put_devs_kobj;
> +
> +	return 0;
> +
> +put_devs_kobj:
> +	kobject_del(ibnbd_devs_kobj);
> +	kobject_put(ibnbd_devs_kobj);
> +dev_destroy:
> +	device_destroy(ibnbd_dev_class, MKDEV(0, 0));
> +cls_destroy:
> +	class_destroy(ibnbd_dev_class);
> +
> +	return err;
> +}

I think this is the wrong way to create a device node because this 
approach will inform udev about device creation before the sysfs group 
has been created. Please use device_create_with_groups() instead of 
calling device_create() and sysfs_create_group() separately.

Bart.

  parent reply index

Thread overview: 123+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190620150337.7847-1-jinpuwang@gmail.com>
2019-06-20 15:03 ` [PATCH v4 01/25] sysfs: export sysfs_remove_file_self() Jack Wang
2019-09-23 17:21   ` Bart Van Assche
2019-09-25  9:30     ` Danil Kipnis
2019-07-09  9:55 ` [PATCH v4 00/25] InfiniBand Transport (IBTRS) and Network Block Device (IBNBD) Danil Kipnis
2019-07-09 11:00   ` Leon Romanovsky
2019-07-09 11:17     ` Greg KH
2019-07-09 11:57       ` Jinpu Wang
2019-07-09 13:32       ` Leon Romanovsky
2019-07-09 15:39       ` Bart Van Assche
2019-07-09 11:37     ` Jinpu Wang
2019-07-09 12:06       ` Jason Gunthorpe
2019-07-09 13:15         ` Jinpu Wang
2019-07-09 13:19           ` Jason Gunthorpe
2019-07-09 14:17             ` Jinpu Wang
2019-07-09 21:27             ` Sagi Grimberg
2019-07-19 13:12               ` Danil Kipnis
2019-07-10 14:55     ` Danil Kipnis
2019-07-09 12:04   ` Jason Gunthorpe
2019-07-09 19:45   ` Sagi Grimberg
2019-07-10 13:55     ` Jason Gunthorpe
2019-07-10 16:25       ` Sagi Grimberg
2019-07-10 17:25         ` Jason Gunthorpe
2019-07-10 19:11           ` Sagi Grimberg
2019-07-11  7:27             ` Danil Kipnis
2019-07-11  8:54     ` Danil Kipnis
2019-07-12  0:22       ` Sagi Grimberg
2019-07-12  7:57         ` Jinpu Wang
2019-07-12 19:40           ` Sagi Grimberg
2019-07-15 11:21             ` Jinpu Wang
2019-07-12 10:58         ` Danil Kipnis
     [not found] ` <20190620150337.7847-26-jinpuwang@gmail.com>
2019-07-09 15:10   ` [PATCH v4 25/25] MAINTAINERS: Add maintainer for IBNBD/IBTRS modules Leon Romanovsky
2019-07-09 15:18     ` Jinpu Wang
2019-07-09 15:51       ` Leon Romanovsky
2019-09-13 23:56   ` Bart Van Assche
2019-09-19 10:30     ` Jinpu Wang
     [not found] ` <20190620150337.7847-16-jinpuwang@gmail.com>
2019-09-13 22:10   ` [PATCH v4 15/25] ibnbd: private headers with IBNBD protocol structs and helpers Bart Van Assche
2019-09-15 14:30     ` Jinpu Wang
2019-09-16  5:27       ` Leon Romanovsky
2019-09-16 13:45         ` Bart Van Assche
2019-09-17 15:41           ` Leon Romanovsky
2019-09-17 15:52             ` Jinpu Wang
2019-09-16  7:08       ` Danil Kipnis
2019-09-16 14:57       ` Jinpu Wang
2019-09-16 17:25         ` Bart Van Assche
2019-09-17 12:27           ` Jinpu Wang
2019-09-16 15:39       ` Jinpu Wang
2019-09-18 15:26         ` Bart Van Assche
2019-09-18 16:11           ` Jinpu Wang
     [not found] ` <20190620150337.7847-17-jinpuwang@gmail.com>
2019-09-13 22:25   ` [PATCH v4 16/25] ibnbd: client: private header with client structs and functions Bart Van Assche
2019-09-17 16:36     ` Jinpu Wang
2019-09-25 23:43       ` Danil Kipnis
2019-09-26 10:00         ` Jinpu Wang
     [not found] ` <20190620150337.7847-18-jinpuwang@gmail.com>
2019-09-13 23:46   ` [PATCH v4 17/25] ibnbd: client: main functionality Bart Van Assche
2019-09-16 14:17     ` Danil Kipnis
2019-09-16 16:46       ` Bart Van Assche
2019-09-17 11:39         ` Danil Kipnis
2019-09-18  7:14           ` Danil Kipnis
2019-09-18 15:47             ` Bart Van Assche
2019-09-20  8:29               ` Danil Kipnis
2019-09-25 22:26               ` Danil Kipnis
2019-09-26  9:55                 ` Roman Penyaev
2019-09-26 15:01                   ` Bart Van Assche
2019-09-27  8:52                     ` Roman Penyaev
2019-09-27  9:32                       ` Danil Kipnis
2019-09-27 12:18                         ` Danil Kipnis
2019-09-27 16:37                       ` Bart Van Assche
2019-09-27 16:50                         ` Roman Penyaev
2019-09-27 17:16                           ` Bart Van Assche
2019-09-17 13:09     ` Jinpu Wang
2019-09-17 16:46       ` Bart Van Assche
2019-09-18 12:02         ` Jinpu Wang
2019-09-18 16:05     ` Jinpu Wang
2019-09-14  0:00   ` Bart Van Assche
     [not found] ` <20190620150337.7847-25-jinpuwang@gmail.com>
2019-09-13 23:58   ` [PATCH v4 24/25] ibnbd: a bit of documentation Bart Van Assche
2019-09-18 12:22     ` Jinpu Wang
     [not found] ` <20190620150337.7847-19-jinpuwang@gmail.com>
2019-09-18 16:28   ` Bart Van Assche [this message]
2019-09-19 15:55     ` [PATCH v4 18/25] ibnbd: client: sysfs interface functions Jinpu Wang
     [not found] ` <20190620150337.7847-21-jinpuwang@gmail.com>
2019-09-18 17:41   ` [PATCH v4 20/25] ibnbd: server: main functionality Bart Van Assche
2019-09-20  7:36     ` Danil Kipnis
2019-09-20 15:42       ` Bart Van Assche
2019-09-23 15:19         ` Danil Kipnis
     [not found] ` <20190620150337.7847-22-jinpuwang@gmail.com>
2019-09-18 21:46   ` [PATCH v4 21/25] ibnbd: server: functionality for IO submission to file or block dev Bart Van Assche
2019-09-26 14:04     ` Jinpu Wang
2019-09-26 15:11       ` Bart Van Assche
2019-09-26 15:25         ` Danil Kipnis
2019-09-26 15:29           ` Bart Van Assche
2019-09-26 15:38             ` Danil Kipnis
2019-09-26 15:42               ` Jinpu Wang
     [not found] ` <20190620150337.7847-3-jinpuwang@gmail.com>
2019-09-23 17:44   ` [PATCH v4 02/25] ibtrs: public interface header to establish RDMA connections Bart Van Assche
2019-09-25 10:20     ` Danil Kipnis
2019-09-25 15:38       ` Bart Van Assche
     [not found] ` <20190620150337.7847-7-jinpuwang@gmail.com>
2019-09-23 21:51   ` [PATCH v4 06/25] ibtrs: client: main functionality Bart Van Assche
2019-09-25 17:36     ` Danil Kipnis
2019-09-25 18:55       ` Bart Van Assche
2019-09-25 20:50         ` Danil Kipnis
2019-09-25 21:08           ` Bart Van Assche
2019-09-25 21:16             ` Bart Van Assche
2019-09-25 22:53             ` Danil Kipnis
2019-09-25 23:21               ` Bart Van Assche
2019-09-26  9:16                 ` Danil Kipnis
     [not found] ` <20190620150337.7847-4-jinpuwang@gmail.com>
2019-09-23 22:50   ` [PATCH v4 03/25] ibtrs: private headers with IBTRS protocol structs and helpers Bart Van Assche
2019-09-25 21:45     ` Danil Kipnis
2019-09-25 21:57       ` Bart Van Assche
2019-09-27  8:56     ` Jinpu Wang
     [not found] ` <20190620150337.7847-5-jinpuwang@gmail.com>
2019-09-23 23:03   ` [PATCH v4 04/25] ibtrs: core: lib functions shared between client and server modules Bart Van Assche
2019-09-27 10:13     ` Jinpu Wang
     [not found] ` <20190620150337.7847-6-jinpuwang@gmail.com>
2019-09-23 23:05   ` [PATCH v4 05/25] ibtrs: client: private header with client structs and functions Bart Van Assche
2019-09-27 10:18     ` Jinpu Wang
     [not found] ` <20190620150337.7847-8-jinpuwang@gmail.com>
2019-09-23 23:15   ` [PATCH v4 07/25] ibtrs: client: statistics functions Bart Van Assche
2019-09-27 12:00     ` Jinpu Wang
     [not found] ` <20190620150337.7847-10-jinpuwang@gmail.com>
2019-09-23 23:21   ` [PATCH v4 09/25] ibtrs: server: private header with server structs and functions Bart Van Assche
2019-09-27 12:04     ` Jinpu Wang
     [not found] ` <20190620150337.7847-11-jinpuwang@gmail.com>
2019-09-23 23:49   ` [PATCH v4 10/25] ibtrs: server: main functionality Bart Van Assche
2019-09-27 15:03     ` Jinpu Wang
2019-09-27 15:11       ` Bart Van Assche
2019-09-27 15:19         ` Jinpu Wang
     [not found] ` <20190620150337.7847-12-jinpuwang@gmail.com>
2019-09-23 23:56   ` [PATCH v4 11/25] ibtrs: server: statistics functions Bart Van Assche
2019-10-02 15:15     ` Jinpu Wang
2019-10-02 15:42       ` Leon Romanovsky
2019-10-02 15:45         ` Jinpu Wang
2019-10-02 16:00           ` Leon Romanovsky
     [not found] ` <20190620150337.7847-13-jinpuwang@gmail.com>
2019-09-24  0:00   ` [PATCH v4 12/25] ibtrs: server: sysfs interface functions Bart Van Assche
2019-10-02 15:11     ` Jinpu Wang

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=05192413-4b0c-76fd-7459-bc6d430b46a6@acm.org \
    --to=bvanassche@acm.org \
    --cc=axboe@kernel.dk \
    --cc=danil.kipnis@cloud.ionos.com \
    --cc=dledford@redhat.com \
    --cc=hch@infradead.org \
    --cc=jgg@mellanox.com \
    --cc=jinpu.wang@cloud.ionos.com \
    --cc=jinpuwang@gmail.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=rpenyaev@suse.de \
    --cc=sagi@grimberg.me \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org linux-rdma@archiver.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/ public-inbox