Re: [Qemu-devel] [PATCH v9 16/18] xen: automatically create XenBlockDevice-s

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 09 January 2019 12:09
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-
> devel@lists.xenproject.org; Kevin Wolf <kwolf@redhat.com>; Max Reitz
> <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v9 16/18] xen: automatically create XenBlockDevice-s
> 
> On Tue, Jan 08, 2019 at 02:49:01PM +0000, Paul Durrant wrote:
> > This patch adds create and destroy function for XenBlockDevice-s so that
> > they can be created automatically when the Xen toolstack instantiates a
> new
> > PV backend via xenstore. When the XenBlockDevice is created this way it
> is
> > also necessary to create a 'drive' which matches the configuration that
> the
> > Xen toolstack has written into xenstore. This is done by formulating the
> > parameters necessary for each 'blockdev' layer of the drive and then
> using
> > qmp_blockdev_add() to create the layers. Also, for compatibility with
> the
> > legacy 'xen_disk' implementation, an iothread is automatically created
> for
> > the new XenBlockDevice. This, like the driver layers, will be destroyed
> > after the XenBlockDevice is unrealized.
> >
> > The legacy backend scan for 'qdisk' is removed by this patch, which
> makes
> > the 'xen_disk' code is redundant. The code will be removed by a
> subsequent
> > patch.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Signed-off-by: Anthony Perard <anthony.perard@citrix.com>
> > ---
> 
> > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> > index a7c37c185a..c6ec1d9543 100644
> > --- a/hw/block/xen-block.c
> > +++ b/hw/block/xen-block.c
> > @@ -7,12 +7,20 @@
> ...
> 
> > +static XenBlockDrive *xen_block_drive_create(const char *id,
> > +                                             const char *device_type,
> > +                                             QDict *opts, Error **errp)
> > +{
> ...
> > +    Error *local_err = NULL;
> ...
> > +    if (!filename) {
> > +        error_setg(errp, "no filename");
> > +        goto done;
> > +    }
> ...
> > +    drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
> > +                                              &local_err);
> > +
> > +done:
> > +    g_free(driver);
> > +    g_free(filename);
> > +
> > +    if (local_err) {
> 
> When xen_block_blockdev_add failed, it sets local_err, but nothing after
> sets errp. I'm going to add this while preparing the pull request:
> 
>     error_propagate(errp, local_err);
> 
> Is this fine with you?

Yes, that's fine... the error should have indeed been propagated.

> 
> With that fix, I think the series is ready, so:
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> 

Thanks.

> > +        xen_block_drive_destroy(drive, NULL);
> > +        return NULL;
> > +    }
> > +
> > +    return drive;
> > +}
> 
> There is still the warning about using the 'file' driver when
> 'host_device' should be use, but I think we can try to fix that later.

TBH I'm hoping this code can go away before we have to worry about it. We need to teach libxl how to issue the necessary QMP commands directly; it should know the difference between a file and a device.

  Paul

> 
> Thanks,
> 
> --
> Anthony PERARD