On 3/21/19 2:18 PM, Igor Konopko wrote:
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
I'm a user-space freak. Let's fix it up in nvme-cli.
On 18.03.2019 15:41, Hans Holmberg wrote:
On Mon, Mar 18, 2019 at 2:18 PM Igor Konopko
<igor.j.konopko@xxxxxxxxx> wrote:
On 18.03.2019 11:28, Hans Holmberg wrote:
On Thu, Mar 14, 2019 at 5:11 PM Igor Konopko
<igor.j.konopko@xxxxxxxxx> wrote:
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 <igor.j.konopko@xxxxxxxxx>
---
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
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
