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

[Anthony PERARD <anthony.perard@citrix.com>]

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?

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

> +        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.

Thanks,

-- 
Anthony PERARD