[PATCH] block/blkio: Make driver nvme-io_uring take a "path" instead of a "filename"

[PATCH] block/blkio: Make driver nvme-io_uring take a "path" instead of a "filename"

[Alberto Faria]

The nvme-io_uring driver expects a character special file such as
/dev/ng0n1. Follow the convention of having a "filename" option when a
regular file is expected, and a "path" option otherwise.

This makes io_uring the only libblkio-based driver with a "filename"
option, as it accepts a regular file (even though it can also take a
block special file).

Signed-off-by: Alberto Faria <afaria@redhat.com>
---
 block/blkio.c        | 12 ++++++++----
 qapi/block-core.json |  4 ++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index 82f26eedd2..5f600e5e9e 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -639,12 +639,17 @@ static int blkio_io_uring_open(BlockDriverState *bs, QDict *options, int flags,
 static int blkio_nvme_io_uring(BlockDriverState *bs, QDict *options, int flags,
                                Error **errp)
 {
-    const char *filename = qdict_get_str(options, "filename");
+    const char *path = qdict_get_try_str(options, "path");
     BDRVBlkioState *s = bs->opaque;
     int ret;
 
-    ret = blkio_set_str(s->blkio, "path", filename);
-    qdict_del(options, "filename");
+    if (!path) {
+        error_setg(errp, "missing 'path' option");
+        return -EINVAL;
+    }
+
+    ret = blkio_set_str(s->blkio, "path", path);
+    qdict_del(options, "path");
     if (ret < 0) {
         error_setg_errno(errp, -ret, "failed to set path: %s",
                          blkio_get_error_msg());
@@ -986,7 +991,6 @@ static BlockDriver bdrv_io_uring = BLKIO_DRIVER(
 
 static BlockDriver bdrv_nvme_io_uring = BLKIO_DRIVER(
     DRIVER_NVME_IO_URING,
-    .bdrv_needs_filename = true,
 );
 
 static BlockDriver bdrv_virtio_blk_vhost_user = BLKIO_DRIVER(
diff --git a/qapi/block-core.json b/qapi/block-core.json
index cb5079e645..6d36c0ed8b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3703,12 +3703,12 @@
 #
 # Driver specific block device options for the nvme-io_uring backend.
 #
-# @filename: path to the image file
+# @path: path to the image file
 #
 # Since: 7.2
 ##
 { 'struct': 'BlockdevOptionsNvmeIoUring',
-  'data': { 'filename': 'str' },
+  'data': { 'path': 'str' },
   'if': 'CONFIG_BLKIO' }
 
 ##
-- 
2.38.1

Re: [PATCH] block/blkio: Make driver nvme-io_uring take a "path" instead of a "filename"

[Markus Armbruster]

Alberto Faria <afaria@redhat.com> writes:

> The nvme-io_uring driver expects a character special file such as
> /dev/ng0n1. Follow the convention of having a "filename" option when a
> regular file is expected, and a "path" option otherwise.

I suspect this is by accident, not by design.  Is it desirable?

> This makes io_uring the only libblkio-based driver with a "filename"
> option, as it accepts a regular file (even though it can also take a
> block special file).
>
> Signed-off-by: Alberto Faria <afaria@redhat.com>

Patch does not apply to master (344744e148e).  What's your base?

Re: [PATCH] block/blkio: Make driver nvme-io_uring take a "path" instead of a "filename"

[Alberto Faria]

On Sat, Oct 29, 2022 at 7:05 AM Markus Armbruster <armbru@redhat.com> wrote:
> Alberto Faria <afaria@redhat.com> writes:
>
> > The nvme-io_uring driver expects a character special file such as
> > /dev/ng0n1. Follow the convention of having a "filename" option when a
> > regular file is expected, and a "path" option otherwise.
>
> I suspect this is by accident, not by design.  Is it desirable?

I'm not sure. Naturally I'd be happier if either "filename" or "path"
was used everywhere. Maybe we should settle on a single one for all
the libblkio drivers? Or maybe we should just leave things as is?

> > This makes io_uring the only libblkio-based driver with a "filename"
> > option, as it accepts a regular file (even though it can also take a
> > block special file).
> >
> > Signed-off-by: Alberto Faria <afaria@redhat.com>
>
> Patch does not apply to master (344744e148e).  What's your base?

I forgot to mention this is based on Stefan's block tree:
https://gitlab.com/stefanha/qemu/-/commits/block

Re: [PATCH] block/blkio: Make driver nvme-io_uring take a "path" instead of a "filename"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Sat, Oct 29, 2022 at 10:50:40AM +0100, Alberto Faria wrote:
> On Sat, Oct 29, 2022 at 7:05 AM Markus Armbruster <armbru@redhat.com> wrote:
> > Alberto Faria <afaria@redhat.com> writes:
> >
> > > The nvme-io_uring driver expects a character special file such as
> > > /dev/ng0n1. Follow the convention of having a "filename" option when a
> > > regular file is expected, and a "path" option otherwise.
> >
> > I suspect this is by accident, not by design.  Is it desirable?
> 
> I'm not sure. Naturally I'd be happier if either "filename" or "path"
> was used everywhere. Maybe we should settle on a single one for all
> the libblkio drivers? Or maybe we should just leave things as is?

It wasn't an accident but maybe it was still a bad idea on my part.

My thinking was that io_uring takes regular files and therefore the
"filename" option name is appropriate. UNIX domain sockets and special
devices usually have the "path" option name (e.g. --chardev socket,path=
for AF_UNIX).

I agree with changing the option to "path" for nvme-io_uring.

The overall naming strategy is debatable. I think we can keep it, but if
the majority wants everything to be "filename" or "path" I'd be okay
with that.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] block/blkio: Make driver nvme-io_uring take a "path" instead of a "filename"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

On Sat, Oct 29, 2022 at 12:38:54AM +0100, Alberto Faria wrote:
> The nvme-io_uring driver expects a character special file such as
> /dev/ng0n1. Follow the convention of having a "filename" option when a
> regular file is expected, and a "path" option otherwise.
> 
> This makes io_uring the only libblkio-based driver with a "filename"
> option, as it accepts a regular file (even though it can also take a
> block special file).
> 
> Signed-off-by: Alberto Faria <afaria@redhat.com>
> ---
>  block/blkio.c        | 12 ++++++++----
>  qapi/block-core.json |  4 ++--
>  2 files changed, 10 insertions(+), 6 deletions(-)

I have applied this so I can prepare a final pull request for QEMU 7.2.
If we decide to follow a different naming strategy in the next day (QEMU
soft freeze) then I'll drop the patch, but for now I think this is the
most likely way forward.

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] block/blkio: Make driver nvme-io_uring take a "path" instead of a "filename"

[Stefano Garzarella]

On Sat, Oct 29, 2022 at 12:38:54AM +0100, Alberto Faria wrote:
>The nvme-io_uring driver expects a character special file such as
>/dev/ng0n1. Follow the convention of having a "filename" option when a
>regular file is expected, and a "path" option otherwise.
>
>This makes io_uring the only libblkio-based driver with a "filename"
>option, as it accepts a regular file (even though it can also take a
>block special file).
>
>Signed-off-by: Alberto Faria <afaria@redhat.com>
>---
> block/blkio.c        | 12 ++++++++----
> qapi/block-core.json |  4 ++--
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
>diff --git a/block/blkio.c b/block/blkio.c
>index 82f26eedd2..5f600e5e9e 100644
>--- a/block/blkio.c
>+++ b/block/blkio.c
>@@ -639,12 +639,17 @@ static int blkio_io_uring_open(BlockDriverState *bs, QDict *options, int flags,
> static int blkio_nvme_io_uring(BlockDriverState *bs, QDict *options, int flags,
>                                Error **errp)
> {
>-    const char *filename = qdict_get_str(options, "filename");
>+    const char *path = qdict_get_try_str(options, "path");
>     BDRVBlkioState *s = bs->opaque;
>     int ret;
>
>-    ret = blkio_set_str(s->blkio, "path", filename);
>-    qdict_del(options, "filename");
>+    if (!path) {
>+        error_setg(errp, "missing 'path' option");
>+        return -EINVAL;
>+    }
>+
>+    ret = blkio_set_str(s->blkio, "path", path);
>+    qdict_del(options, "path");
>     if (ret < 0) {
>         error_setg_errno(errp, -ret, "failed to set path: %s",
>                          blkio_get_error_msg());
>@@ -986,7 +991,6 @@ static BlockDriver bdrv_io_uring = BLKIO_DRIVER(
>
> static BlockDriver bdrv_nvme_io_uring = BLKIO_DRIVER(
>     DRIVER_NVME_IO_URING,
>-    .bdrv_needs_filename = true,
> );
>
> static BlockDriver bdrv_virtio_blk_vhost_user = BLKIO_DRIVER(
>diff --git a/qapi/block-core.json b/qapi/block-core.json
>index cb5079e645..6d36c0ed8b 100644
>--- a/qapi/block-core.json
>+++ b/qapi/block-core.json
>@@ -3703,12 +3703,12 @@
> #
> # Driver specific block device options for the nvme-io_uring backend.
> #
>-# @filename: path to the image file
>+# @path: path to the image file

Maybe we can update the "path" description with something similar to 
what we have in the libblkio docs:
   path to the NVMe namespace's character device (e.g., `/dev/ng0n1`).

Thanks,
Stefano