Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
From: Jinpu Wang <jinpu.wang@cloud.ionos.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Jack Wang <jinpuwang@gmail.com>,
	linux-block@vger.kernel.org, linux-rdma@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	Sagi Grimberg <sagi@grimberg.me>,
	Jason Gunthorpe <jgg@mellanox.com>,
	Doug Ledford <dledford@redhat.com>,
	Danil Kipnis <danil.kipnis@cloud.ionos.com>,
	rpenyaev@suse.de
Subject: Re: [PATCH v4 18/25] ibnbd: client: sysfs interface functions
Date: Thu, 19 Sep 2019 17:55:14 +0200
Message-ID: <CAMGffEn09RiLjkHZjt1GFkeLjk+vprupLFdPfLJ2PetArc5nHw@mail.gmail.com> (raw)
In-Reply-To: <05192413-4b0c-76fd-7459-bc6d430b46a6@acm.org>

On Wed, Sep 18, 2019 at 6:28 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> 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().
We feel it's quite handy for debugging to have line number, I checked
in mainline,
some driver even include __func__ and __LINE__.
Also did a test, the line number occur only once from 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?
Sounds good.
>
>  > +/* 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?
Yes, we want a strip all newline in the string, when print with
> 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?
 We strstrip/strim afterwards to remove the whitespace.
>
> > +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?
Yes, it only removes the whitespace
>
> > +     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.
We can change sessname to be dynamically allocated, but I think the
the NAME_MAX check
is not conflicting, we don't want to have that long sessname anyway.

>
> > +             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.
Ditto
>
> > +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.
You're right, will address both.
>
> > +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.
DEVICE_ATTR_RO doesn't fit here, will use __ATTR_RO, thanks
>
> > +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?
sysfs_streq is only tolerant for trailing newline. we use strstrip to
strip whitespaces, strip for newlines.

>
> > +     /*
> > +      * 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?
That's the first thing we do in ibnbd_destroy_sessions already.
>
> > +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.

The reason might be lost in the dust, I can retest without module_* to
see if our tests don't break

>
> > +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.
I'm not aware of device_create_with_groups, will try it out.

Thanks,
Jinpu

  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   ` [PATCH v4 18/25] ibnbd: client: sysfs interface functions Bart Van Assche
2019-09-19 15:55     ` Jinpu Wang [this message]
     [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=CAMGffEn09RiLjkHZjt1GFkeLjk+vprupLFdPfLJ2PetArc5nHw@mail.gmail.com \
    --to=jinpu.wang@cloud.ionos.com \
    --cc=axboe@kernel.dk \
    --cc=bvanassche@acm.org \
    --cc=danil.kipnis@cloud.ionos.com \
    --cc=dledford@redhat.com \
    --cc=hch@infradead.org \
    --cc=jgg@mellanox.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
	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.git