[PATCH v2] vduse-blk: Add name option

[PATCH v2] vduse-blk: Add name option

[Xie Yongji]

Currently we use 'id' option as the name of VDUSE device.
It's a bit confusing since we use one value for two different
purposes: the ID to identfy the export within QEMU (must be
distinct from any other exports in the same QEMU process, but
can overlap with names used by other processes), and the VDUSE
name to uniquely identify it on the host (must be distinct from
other VDUSE devices on the same host, but can overlap with other
export types like NBD in the same process). To make it clear,
this patch adds a separate 'name ' option to specify the VDUSE
name for the vduse-blk export instead.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 block/export/vduse-blk.c             | 9 ++++++---
 docs/tools/qemu-storage-daemon.rst   | 5 +++--
 qapi/block-export.json               | 7 ++++---
 storage-daemon/qemu-storage-daemon.c | 8 ++++----
 4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
index 3b10349173..d96993bdf5 100644
--- a/block/export/vduse-blk.c
+++ b/block/export/vduse-blk.c
@@ -245,7 +245,7 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
     }
     vblk_exp->num_queues = num_queues;
     vblk_exp->handler.blk = exp->blk;
-    vblk_exp->handler.serial = exp->id;
+    vblk_exp->handler.serial = g_strdup(vblk_opts->name);
     vblk_exp->handler.logical_block_size = logical_block_size;
     vblk_exp->handler.writable = opts->writable;
 
@@ -279,22 +279,24 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
         features |= 1ULL << VIRTIO_BLK_F_RO;
     }
 
-    vblk_exp->dev = vduse_dev_create(exp->id, VIRTIO_ID_BLOCK, 0,
+    vblk_exp->dev = vduse_dev_create(vblk_opts->name, VIRTIO_ID_BLOCK, 0,
                                      features, num_queues,
                                      sizeof(struct virtio_blk_config),
                                      (char *)&config, &vduse_blk_ops,
                                      vblk_exp);
     if (!vblk_exp->dev) {
         error_setg(errp, "failed to create vduse device");
+        g_free((void *)vblk_exp->handler.serial);
         return -ENOMEM;
     }
 
     vblk_exp->recon_file = g_strdup_printf("%s/vduse-blk-%s",
-                                           g_get_tmp_dir(), exp->id);
+                                           g_get_tmp_dir(), vblk_opts->name);
     if (vduse_set_reconnect_log_file(vblk_exp->dev, vblk_exp->recon_file)) {
         error_setg(errp, "failed to set reconnect log file");
         vduse_dev_destroy(vblk_exp->dev);
         g_free(vblk_exp->recon_file);
+        g_free((void *)vblk_exp->handler.serial);
         return -EINVAL;
     }
 
@@ -323,6 +325,7 @@ static void vduse_blk_exp_delete(BlockExport *exp)
     vduse_dev_destroy(vblk_exp->dev);
     unlink(vblk_exp->recon_file);
     g_free(vblk_exp->recon_file);
+    g_free((void *)vblk_exp->handler.serial);
 }
 
 static void vduse_blk_exp_request_shutdown(BlockExport *exp)
diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
index fbeaf76954..8a20ebd304 100644
--- a/docs/tools/qemu-storage-daemon.rst
+++ b/docs/tools/qemu-storage-daemon.rst
@@ -77,7 +77,7 @@ Standard options:
   --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,addr.type=unix,addr.path=<socket-path>[,writable=on|off][,logical-block-size=<block-size>][,num-queues=<num-queues>]
   --export [type=]vhost-user-blk,id=<id>,node-name=<node-name>,addr.type=fd,addr.str=<fd>[,writable=on|off][,logical-block-size=<block-size>][,num-queues=<num-queues>]
   --export [type=]fuse,id=<id>,node-name=<node-name>,mountpoint=<file>[,growable=on|off][,writable=on|off][,allow-other=on|off|auto]
-  --export [type=]vduse-blk,id=<id>,node-name=<node-name>[,writable=on|off][,num-queues=<num-queues>][,queue-size=<queue-size>][,logical-block-size=<block-size>]
+  --export [type=]vduse-blk,id=<id>,node-name=<node-name>,name=<vduse-name>[,writable=on|off][,num-queues=<num-queues>][,queue-size=<queue-size>][,logical-block-size=<block-size>]
 
   is a block export definition. ``node-name`` is the block node that should be
   exported. ``writable`` determines whether or not the export allows write
@@ -111,7 +111,8 @@ Standard options:
   ``allow-other`` to auto (the default) will try enabling this option, and on
   error fall back to disabling it.
 
-  The ``vduse-blk`` export type uses the ``id`` as the VDUSE device name.
+  The ``vduse-blk`` export type takes a ``name`` (must be unique across the host)
+  to create the VDUSE device.
   ``num-queues`` sets the number of virtqueues (the default is 1).
   ``queue-size`` sets the virtqueue descriptor table size (the default is 256).
 
diff --git a/qapi/block-export.json b/qapi/block-export.json
index e4bd4de363..f5a2713e59 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -182,6 +182,7 @@
 #
 # A vduse-blk block export.
 #
+# @name: the name of VDUSE device (must be unique across the host).
 # @num-queues: the number of virtqueues. Defaults to 1.
 # @queue-size: the size of virtqueue. Defaults to 256.
 # @logical-block-size: Logical block size in bytes. Range [512, PAGE_SIZE]
@@ -190,7 +191,8 @@
 # Since: 7.1
 ##
 { 'struct': 'BlockExportOptionsVduseBlk',
-  'data': { '*num-queues': 'uint16',
+  'data': { 'name': 'str',
+            '*num-queues': 'uint16',
             '*queue-size': 'uint16',
             '*logical-block-size': 'size'} }
 
@@ -314,8 +316,7 @@
 # Describes a block export, i.e. how single node should be exported on an
 # external interface.
 #
-# @id: A unique identifier for the block export (across the host for vduse-blk
-#      export type or across all export types for other types)
+# @id: A unique identifier for the block export (across all export types)
 #
 # @node-name: The node name of the block node to be exported (since: 5.2)
 #
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index d47fb2d468..4375282b2e 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -122,11 +122,11 @@ static void help(void)
 #endif /* CONFIG_VHOST_USER_BLK_SERVER */
 #ifdef CONFIG_VDUSE_BLK_EXPORT
 "  --export [type=]vduse-blk,id=<id>,node-name=<node-name>\n"
-"           [,writable=on|off][,num-queues=<num-queues>]\n"
-"           [,queue-size=<queue-size>]\n"
+"           ,name=<vduse-name>[,writable=on|off]\n"
+"           [,num-queues=<num-queues>][,queue-size=<queue-size>]\n"
 "           [,logical-block-size=<logical-block-size>]\n"
-"                         export the specified block node as a vduse-blk\n"
-"                         device using the id as the VDUSE device name\n"
+"                         export the specified block node as a\n"
+"                         vduse-blk device\n"
 "\n"
 #endif /* CONFIG_VDUSE_BLK_EXPORT */
 "  --monitor [chardev=]name[,mode=control][,pretty[=on|off]]\n"
-- 
2.20.1

Re: [PATCH v2] vduse-blk: Add name option

[Stefan Hajnoczi]

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

On Tue, May 31, 2022 at 05:52:21PM +0800, Xie Yongji wrote:
> Currently we use 'id' option as the name of VDUSE device.
> It's a bit confusing since we use one value for two different
> purposes: the ID to identfy the export within QEMU (must be
> distinct from any other exports in the same QEMU process, but
> can overlap with names used by other processes), and the VDUSE
> name to uniquely identify it on the host (must be distinct from
> other VDUSE devices on the same host, but can overlap with other
> export types like NBD in the same process). To make it clear,
> this patch adds a separate 'name ' option to specify the VDUSE
> name for the vduse-blk export instead.
> 
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  block/export/vduse-blk.c             | 9 ++++++---
>  docs/tools/qemu-storage-daemon.rst   | 5 +++--
>  qapi/block-export.json               | 7 ++++---
>  storage-daemon/qemu-storage-daemon.c | 8 ++++----
>  4 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
> index 3b10349173..d96993bdf5 100644
> --- a/block/export/vduse-blk.c
> +++ b/block/export/vduse-blk.c
> @@ -245,7 +245,7 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
>      }
>      vblk_exp->num_queues = num_queues;
>      vblk_exp->handler.blk = exp->blk;
> -    vblk_exp->handler.serial = exp->id;
> +    vblk_exp->handler.serial = g_strdup(vblk_opts->name);

Do we want to expose the VDUSE device name to the guest? Maybe the
serial string should be a separate parameter.

>      vblk_exp->handler.logical_block_size = logical_block_size;
>      vblk_exp->handler.writable = opts->writable;
>  
> @@ -279,22 +279,24 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
>          features |= 1ULL << VIRTIO_BLK_F_RO;
>      }
>  
> -    vblk_exp->dev = vduse_dev_create(exp->id, VIRTIO_ID_BLOCK, 0,
> +    vblk_exp->dev = vduse_dev_create(vblk_opts->name, VIRTIO_ID_BLOCK, 0,
>                                       features, num_queues,
>                                       sizeof(struct virtio_blk_config),
>                                       (char *)&config, &vduse_blk_ops,
>                                       vblk_exp);
>      if (!vblk_exp->dev) {
>          error_setg(errp, "failed to create vduse device");
> +        g_free((void *)vblk_exp->handler.serial);

serial isn't const char * anymore, it's char *. Please update the struct
field and then these casts won't be necessary anymore.

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

Re: [PATCH v2] vduse-blk: Add name option

[Yongji Xie]

On Wed, Jun 1, 2022 at 9:03 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, May 31, 2022 at 05:52:21PM +0800, Xie Yongji wrote:
> > Currently we use 'id' option as the name of VDUSE device.
> > It's a bit confusing since we use one value for two different
> > purposes: the ID to identfy the export within QEMU (must be
> > distinct from any other exports in the same QEMU process, but
> > can overlap with names used by other processes), and the VDUSE
> > name to uniquely identify it on the host (must be distinct from
> > other VDUSE devices on the same host, but can overlap with other
> > export types like NBD in the same process). To make it clear,
> > this patch adds a separate 'name ' option to specify the VDUSE
> > name for the vduse-blk export instead.
> >
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> >  block/export/vduse-blk.c             | 9 ++++++---
> >  docs/tools/qemu-storage-daemon.rst   | 5 +++--
> >  qapi/block-export.json               | 7 ++++---
> >  storage-daemon/qemu-storage-daemon.c | 8 ++++----
> >  4 files changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
> > index 3b10349173..d96993bdf5 100644
> > --- a/block/export/vduse-blk.c
> > +++ b/block/export/vduse-blk.c
> > @@ -245,7 +245,7 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
> >      }
> >      vblk_exp->num_queues = num_queues;
> >      vblk_exp->handler.blk = exp->blk;
> > -    vblk_exp->handler.serial = exp->id;
> > +    vblk_exp->handler.serial = g_strdup(vblk_opts->name);
>
> Do we want to expose the VDUSE device name to the guest? Maybe the
> serial string should be a separate parameter.
>

OK, it makes sense to me. But we might need a default value. Any suggestions?

> >      vblk_exp->handler.logical_block_size = logical_block_size;
> >      vblk_exp->handler.writable = opts->writable;
> >
> > @@ -279,22 +279,24 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
> >          features |= 1ULL << VIRTIO_BLK_F_RO;
> >      }
> >
> > -    vblk_exp->dev = vduse_dev_create(exp->id, VIRTIO_ID_BLOCK, 0,
> > +    vblk_exp->dev = vduse_dev_create(vblk_opts->name, VIRTIO_ID_BLOCK, 0,
> >                                       features, num_queues,
> >                                       sizeof(struct virtio_blk_config),
> >                                       (char *)&config, &vduse_blk_ops,
> >                                       vblk_exp);
> >      if (!vblk_exp->dev) {
> >          error_setg(errp, "failed to create vduse device");
> > +        g_free((void *)vblk_exp->handler.serial);
>
> serial isn't const char * anymore, it's char *. Please update the struct
> field and then these casts won't be necessary anymore.

OK.

Thanks,
Yongji

Re: [PATCH v2] vduse-blk: Add name option

[Stefan Hajnoczi]

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

On Wed, Jun 01, 2022 at 09:10:58PM +0800, Yongji Xie wrote:
> On Wed, Jun 1, 2022 at 9:03 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Tue, May 31, 2022 at 05:52:21PM +0800, Xie Yongji wrote:
> > > Currently we use 'id' option as the name of VDUSE device.
> > > It's a bit confusing since we use one value for two different
> > > purposes: the ID to identfy the export within QEMU (must be
> > > distinct from any other exports in the same QEMU process, but
> > > can overlap with names used by other processes), and the VDUSE
> > > name to uniquely identify it on the host (must be distinct from
> > > other VDUSE devices on the same host, but can overlap with other
> > > export types like NBD in the same process). To make it clear,
> > > this patch adds a separate 'name ' option to specify the VDUSE
> > > name for the vduse-blk export instead.
> > >
> > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > ---
> > >  block/export/vduse-blk.c             | 9 ++++++---
> > >  docs/tools/qemu-storage-daemon.rst   | 5 +++--
> > >  qapi/block-export.json               | 7 ++++---
> > >  storage-daemon/qemu-storage-daemon.c | 8 ++++----
> > >  4 files changed, 17 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
> > > index 3b10349173..d96993bdf5 100644
> > > --- a/block/export/vduse-blk.c
> > > +++ b/block/export/vduse-blk.c
> > > @@ -245,7 +245,7 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
> > >      }
> > >      vblk_exp->num_queues = num_queues;
> > >      vblk_exp->handler.blk = exp->blk;
> > > -    vblk_exp->handler.serial = exp->id;
> > > +    vblk_exp->handler.serial = g_strdup(vblk_opts->name);
> >
> > Do we want to expose the VDUSE device name to the guest? Maybe the
> > serial string should be a separate parameter.
> >
> 
> OK, it makes sense to me. But we might need a default value. Any suggestions?

hw/block/virtio-blk.c defaults to the empty string:

  const char *serial = s->conf.serial ? s->conf.serial : "";

I think it's reasonable to say that anyone who wants to use serial will
also want to set the value explicitly.

Stefan

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

Re: [PATCH v2] vduse-blk: Add name option

[Yongji Xie]

On Mon, Jun 6, 2022 at 7:05 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Wed, Jun 01, 2022 at 09:10:58PM +0800, Yongji Xie wrote:
> > On Wed, Jun 1, 2022 at 9:03 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > On Tue, May 31, 2022 at 05:52:21PM +0800, Xie Yongji wrote:
> > > > Currently we use 'id' option as the name of VDUSE device.
> > > > It's a bit confusing since we use one value for two different
> > > > purposes: the ID to identfy the export within QEMU (must be
> > > > distinct from any other exports in the same QEMU process, but
> > > > can overlap with names used by other processes), and the VDUSE
> > > > name to uniquely identify it on the host (must be distinct from
> > > > other VDUSE devices on the same host, but can overlap with other
> > > > export types like NBD in the same process). To make it clear,
> > > > this patch adds a separate 'name ' option to specify the VDUSE
> > > > name for the vduse-blk export instead.
> > > >
> > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > ---
> > > >  block/export/vduse-blk.c             | 9 ++++++---
> > > >  docs/tools/qemu-storage-daemon.rst   | 5 +++--
> > > >  qapi/block-export.json               | 7 ++++---
> > > >  storage-daemon/qemu-storage-daemon.c | 8 ++++----
> > > >  4 files changed, 17 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
> > > > index 3b10349173..d96993bdf5 100644
> > > > --- a/block/export/vduse-blk.c
> > > > +++ b/block/export/vduse-blk.c
> > > > @@ -245,7 +245,7 @@ static int vduse_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
> > > >      }
> > > >      vblk_exp->num_queues = num_queues;
> > > >      vblk_exp->handler.blk = exp->blk;
> > > > -    vblk_exp->handler.serial = exp->id;
> > > > +    vblk_exp->handler.serial = g_strdup(vblk_opts->name);
> > >
> > > Do we want to expose the VDUSE device name to the guest? Maybe the
> > > serial string should be a separate parameter.
> > >
> >
> > OK, it makes sense to me. But we might need a default value. Any suggestions?
>
> hw/block/virtio-blk.c defaults to the empty string:
>
>   const char *serial = s->conf.serial ? s->conf.serial : "";
>
> I think it's reasonable to say that anyone who wants to use serial will
> also want to set the value explicitly.
>

LGTM.

Thanks,
Yongji