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