Matias,
any opinion from your side, whether you would like to do such a changes 
in userspace tool or in lightnvm core? I can go both ways.
Thanks
Igor

On 18.03.2019 15:41, Hans Holmberg wrote:

On Mon, Mar 18, 2019 at 2:18 PM Igor Konopko [off-list ref] wrote:

quoted

On 18.03.2019 11:28, Hans Holmberg wrote:

quoted

On Thu, Mar 14, 2019 at 5:11 PM Igor Konopko [off-list ref] wrote:

quoted

This patch adds the possibility to provide full device path (like
/dev/nvme0n1) when specifying device on top of which pblk instance
should be created/removed.

This makes creation of targets from nvme-cli (or other ioctl based
tools) more unified with other commands in comparison with current
situation where almost all commands uses full device path with except
of lightnvm creation/removal parameter which uses just 'nvme0n1'
naming convention. After this changes both approach will be valid.

Signed-off-by: Igor Konopko <redacted>
---
   drivers/lightnvm/core.c | 23 ++++++++++++++++++-----
   1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index c01f83b..838c3d8 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c

@@ -1195,6 +1195,21 @@ void nvm_unregister(struct nvm_dev *dev)
   }
   EXPORT_SYMBOL(nvm_unregister);

+#define PREFIX_STR "/dev/"
+static void nvm_normalize_path(char *path)
+{
+       path[DISK_NAME_LEN - 1] = '\0';
+       if (!memcmp(PREFIX_STR, path,
+                               sizeof(char) * strlen(PREFIX_STR))) {
+               /*
+                * User provide name in '/dev/nvme0n1' format,
+                * so we need to skip '/dev/' for comparison
+                */
+               memmove(path, path + sizeof(char) * strlen(PREFIX_STR),
+                       (DISK_NAME_LEN - strlen(PREFIX_STR)) * sizeof(char));
+       }
+}
+

I don't like this. Why add string parsing to the kernel? Can't this
feature be added to the nvme tool?

Since during target creation/removal in kernel, we already operate on
strings multiple times (strcmp calls for target types, nvme device,
target names) my idea was to keep this in the same layer too.

Oh, pardon the terse and rather grumpy review. Let me elaborate:

String parsing is best avoided when possible, and i don't think it's
worth increasing the kernel code size and changing the behavior of the
IOCTL when its fully doable to do this in userspace.

Thanks,
Hans

quoted

quoted

quoted

   static int __nvm_configure_create(struct nvm_ioctl_create *create)
   {
          struct nvm_dev *dev;

@@ -1304,9 +1319,9 @@ static long nvm_ioctl_dev_create(struct file *file, void __user *arg)
                  return -EINVAL;
          }

-       create.dev[DISK_NAME_LEN - 1] = '\0';
+       nvm_normalize_path(create.dev);
+       nvm_normalize_path(create.tgtname);
          create.tgttype[NVM_TTYPE_NAME_MAX - 1] = '\0';
-       create.tgtname[DISK_NAME_LEN - 1] = '\0';

          if (create.flags != 0) {
                  __u32 flags = create.flags;

@@ -1333,7 +1348,7 @@ static long nvm_ioctl_dev_remove(struct file *file, void __user *arg)
          if (copy_from_user(&remove, arg, sizeof(struct nvm_ioctl_remove)))
                  return -EFAULT;

-       remove.tgtname[DISK_NAME_LEN - 1] = '\0';
+       nvm_normalize_path(remove.tgtname);

          if (remove.flags != 0) {
                  pr_err("nvm: no flags supported\n");

@@ -1373,8 +1388,6 @@ static long nvm_ioctl_dev_factory(struct file *file, void __user *arg)
          if (copy_from_user(&fact, arg, sizeof(struct nvm_ioctl_dev_factory)))
                  return -EFAULT;

-       fact.dev[DISK_NAME_LEN - 1] = '\0';
-
          if (fact.flags & ~(NVM_FACTORY_NR_BITS - 1))
                  return -EINVAL;

--

2.9.5