Message ID | 20221028233854.839933-1-afaria@redhat.com |
---|---|
State | New |
Headers | show |
Series | block/blkio: Make driver nvme-io_uring take a "path" instead of a "filename" | expand |
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?
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
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
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
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
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' } ##
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(-)