[v7,16/18] xen: automatically create XenBlockDevice-s

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>
---
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>

v7:
 - Don't use qobject_input_visitor_new_flat_confused()

v5:
 - Extensively re-worked to avoid using drive_new() and use
   qmp_blockdev_add() instead
 - Also use qmp_object_add() for IOThread
 - Dropped Anthony's R-b because of the code changes

v2:
 - Get rid of error_abort
 - Don't use qdev_init_nofail()
 - Explain why file locking needs to be off
---
 hw/block/trace-events       |   4 +
 hw/block/xen-block.c        | 404 ++++++++++++++++++++++++++++++++++++
 hw/xen/xen-legacy-backend.c |   1 -
 include/hw/xen/xen-block.h  |  13 ++
 4 files changed, 421 insertions(+), 1 deletion(-)

Ping Anthony & Kevin?

I believe this version is purged of all legacy interface use.

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 20 December 2018 17:15
> To: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-
> devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Anthony Perard
> <anthony.perard@citrix.com>; Kevin Wolf <kwolf@redhat.com>; Max Reitz
> <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
> 
> 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>
> ---
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> 
> v7:
>  - Don't use qobject_input_visitor_new_flat_confused()
> 
> v5:
>  - Extensively re-worked to avoid using drive_new() and use
>    qmp_blockdev_add() instead
>  - Also use qmp_object_add() for IOThread
>  - Dropped Anthony's R-b because of the code changes
> 
> v2:
>  - Get rid of error_abort
>  - Don't use qdev_init_nofail()
>  - Explain why file locking needs to be off
> ---
>  hw/block/trace-events       |   4 +
>  hw/block/xen-block.c        | 404 ++++++++++++++++++++++++++++++++++++
>  hw/xen/xen-legacy-backend.c |   1 -
>  include/hw/xen/xen-block.h  |  13 ++
>  4 files changed, 421 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 89e258319c..55e5a5500c 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -137,3 +137,7 @@ xen_disk_realize(void) ""
>  xen_disk_unrealize(void) ""
>  xen_cdrom_realize(void) ""
>  xen_cdrom_unrealize(void) ""
> +xen_block_blockdev_add(char *str) "%s"
> +xen_block_blockdev_del(const char *node_name) "%s"
> +xen_block_device_create(unsigned int number) "%u"
> +xen_block_device_destroy(unsigned int number) "%u"
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index a7c37c185a..1e34fe1527 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -7,12 +7,20 @@
> 
>  #include "qemu/osdep.h"
>  #include "qemu/cutils.h"
> +#include "qemu/option.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-commands-block-core.h"
> +#include "qapi/qapi-commands-misc.h"
> +#include "qapi/qapi-visit-block-core.h"
> +#include "qapi/qobject-input-visitor.h"
>  #include "qapi/visitor.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qstring.h"
>  #include "hw/hw.h"
>  #include "hw/xen/xen_common.h"
>  #include "hw/block/xen_blkif.h"
>  #include "hw/xen/xen-block.h"
> +#include "hw/xen/xen-backend.h"
>  #include "sysemu/blockdev.h"
>  #include "sysemu/block-backend.h"
>  #include "sysemu/iothread.h"
> @@ -474,6 +482,7 @@ static void xen_block_class_init(ObjectClass *class,
> void *data)
>      DeviceClass *dev_class = DEVICE_CLASS(class);
>      XenDeviceClass *xendev_class = XEN_DEVICE_CLASS(class);
> 
> +    xendev_class->backend = "qdisk";
>      xendev_class->device = "vbd";
>      xendev_class->get_name = xen_block_get_name;
>      xendev_class->realize = xen_block_realize;
> @@ -586,3 +595,398 @@ static void xen_block_register_types(void)
>  }
> 
>  type_init(xen_block_register_types)
> +
> +static void xen_block_blockdev_del(const char *node_name, Error **errp)
> +{
> +    trace_xen_block_blockdev_del(node_name);
> +
> +    qmp_blockdev_del(node_name, errp);
> +}
> +
> +static char *xen_block_blockdev_add(const char *id, QDict *qdict,
> +                                    Error **errp)
> +{
> +    const char *driver = qdict_get_try_str(qdict, "driver");
> +    BlockdevOptions *options = NULL;
> +    Error *local_err = NULL;
> +    char *node_name;
> +    Visitor *v;
> +
> +    if (!driver) {
> +        error_setg(errp, "no 'driver' parameter");
> +        return NULL;
> +    }
> +
> +    node_name = g_strdup_printf("%s-%s", id, driver);
> +    qdict_put_str(qdict, "node-name", node_name);
> +
> +    trace_xen_block_blockdev_add(node_name);
> +
> +    v = qobject_input_visitor_new(QOBJECT(qdict));
> +    visit_type_BlockdevOptions(v, NULL, &options, &local_err);
> +    visit_free(v);
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto fail;
> +    }
> +
> +    qmp_blockdev_add(options, &local_err);
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto fail;
> +    }
> +
> +    qapi_free_BlockdevOptions(options);
> +
> +    return node_name;
> +
> +fail:
> +    if (options) {
> +        qapi_free_BlockdevOptions(options);
> +    }
> +    g_free(node_name);
> +
> +    return NULL;
> +}
> +
> +static void xen_block_drive_destroy(XenBlockDrive *drive, Error **errp)
> +{
> +    while (drive->layers-- != 0) {
> +        char *node_name = drive->node_name[drive->layers];
> +        Error *local_err = NULL;
> +
> +        xen_block_blockdev_del(node_name, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            drive->layers++;
> +            return;
> +        }
> +    }
> +    g_free(drive->id);
> +    g_free(drive);
> +}
> +
> +static void xen_block_drive_layer_add(XenBlockDrive *drive, QDict *qdict,
> +                                      Error **errp)
> +{
> +    unsigned int i = drive->layers;
> +    char *node_name;
> +
> +    g_assert(drive->layers < ARRAY_SIZE(drive->node_name));
> +
> +    if (i != 0) {
> +        /* Link to the lower layer */
> +        qdict_put_str(qdict, "file", drive->node_name[i - 1]);
> +    }
> +
> +    node_name = xen_block_blockdev_add(drive->id, qdict, errp);
> +    if (!node_name) {
> +        return;
> +    }
> +
> +    drive->node_name[i] = node_name;
> +    drive->layers++;
> +}
> +
> +static XenBlockDrive *xen_block_drive_create(const char *id,
> +                                             const char *device_type,
> +                                             QDict *opts, Error **errp)
> +{
> +    const char *params = qdict_get_try_str(opts, "params");
> +    const char *mode = qdict_get_try_str(opts, "mode");
> +    const char *direct_io_safe = qdict_get_try_str(opts, "direct-io-
> safe");
> +    const char *discard_enable = qdict_get_try_str(opts, "discard-
> enable");
> +    char *driver = NULL;
> +    char *filename = NULL;
> +    XenBlockDrive *drive = NULL;
> +    Error *local_err = NULL;
> +    QDict *qdict;
> +
> +    if (params) {
> +        char **v = g_strsplit(params, ":", 2);
> +
> +        if (v[1] == NULL) {
> +            filename = g_strdup(v[0]);
> +            driver = g_strdup("file");
> +        } else {
> +            if (strcmp(v[0], "aio") == 0) {
> +                driver = g_strdup("file");
> +            } else if (strcmp(v[0], "vhd") == 0) {
> +                driver = g_strdup("vpc");
> +            } else {
> +                driver = g_strdup(v[0]);
> +            }
> +            filename = g_strdup(v[1]);
> +        }
> +
> +        g_strfreev(v);
> +    }
> +
> +    if (!filename) {
> +        error_setg(errp, "no filename");
> +        goto done;
> +    }
> +    assert(driver);
> +
> +    drive = g_new0(XenBlockDrive, 1);
> +    drive->id = g_strdup(id);
> +
> +    qdict = qdict_new();
> +
> +    qdict_put_str(qdict, "driver", "file");
> +    qdict_put_str(qdict, "filename", filename);
> +
> +    if (mode && *mode != 'w') {
> +        qdict_put_bool(qdict, "read-only", true);
> +    }
> +
> +    if (direct_io_safe) {
> +        unsigned long value;
> +
> +        if (!qemu_strtoul(direct_io_safe, NULL, 2, &value) && !!value) {
> +            QDict *cache_qdict = qdict_new();
> +
> +            qdict_put_bool(cache_qdict, "direct", true);
> +            qdict_put_obj(qdict, "cache", QOBJECT(cache_qdict));
> +
> +            qdict_put_str(qdict, "aio", "native");
> +        }
> +    }
> +
> +    if (discard_enable) {
> +        unsigned long value;
> +
> +        if (!qemu_strtoul(discard_enable, NULL, 2, &value) && !!value) {
> +            qdict_put_str(qdict, "discard", "unmap");
> +        }
> +    }
> +
> +    /*
> +     * It is necessary to turn file locking off as an emulated device
> +     * may have already opened the same image file.
> +     */
> +    qdict_put_str(qdict, "locking", "off");
> +
> +    xen_block_drive_layer_add(drive, qdict, &local_err);
> +    qobject_unref(qdict);
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto done;
> +    }
> +
> +    /* If the image is a raw file then we are done */
> +    if (!strcmp(driver, "file")) {
> +        goto done;
> +    }
> +
> +    qdict = qdict_new();
> +
> +    qdict_put_str(qdict, "driver", driver);
> +
> +    xen_block_drive_layer_add(drive, qdict, &local_err);
> +    qobject_unref(qdict);
> +
> +done:
> +    g_free(driver);
> +    g_free(filename);
> +
> +    if (local_err) {
> +        xen_block_drive_destroy(drive, NULL);
> +        return NULL;
> +    }
> +
> +    return drive;
> +}
> +
> +static const char *xen_block_drive_get_node_name(XenBlockDrive *drive)
> +{
> +    return drive->layers ? drive->node_name[drive->layers - 1] : "";
> +}
> +
> +static void xen_block_iothread_destroy(XenBlockIOThread *iothread,
> +                                       Error **errp)
> +{
> +    qmp_object_del(iothread->id, errp);
> +
> +    g_free(iothread->id);
> +    g_free(iothread);
> +}
> +
> +static XenBlockIOThread *xen_block_iothread_create(const char *id,
> +                                                   Error **errp)
> +{
> +    XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
> +    Error *local_err = NULL;
> +
> +    iothread->id = g_strdup(id);
> +
> +    qmp_object_add(TYPE_IOTHREAD, id, false, NULL, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +
> +        g_free(iothread->id);
> +        g_free(iothread);
> +        return NULL;
> +    }
> +
> +    return iothread;
> +}
> +
> +static void xen_block_device_create(XenBackendInstance *backend,
> +                                    QDict *opts, Error **errp)
> +{
> +    XenBus *xenbus = xen_backend_get_bus(backend);
> +    const char *name = xen_backend_get_name(backend);
> +    unsigned long number;
> +    const char *vdev, *device_type;
> +    XenBlockDrive *drive = NULL;
> +    XenBlockIOThread *iothread = NULL;
> +    XenDevice *xendev = NULL;
> +    Error *local_err = NULL;
> +    const char *type;
> +    XenBlockDevice *blockdev;
> +
> +    if (qemu_strtoul(name, NULL, 10, &number)) {
> +        error_setg(errp, "failed to parse name '%s'", name);
> +        goto fail;
> +    }
> +
> +    trace_xen_block_device_create(number);
> +
> +    vdev = qdict_get_try_str(opts, "dev");
> +    if (!vdev) {
> +        error_setg(errp, "no dev parameter");
> +        goto fail;
> +    }
> +
> +    device_type = qdict_get_try_str(opts, "device-type");
> +    if (!device_type) {
> +        error_setg(errp, "no device-type parameter");
> +        goto fail;
> +    }
> +
> +    if (!strcmp(device_type, "disk")) {
> +        type = TYPE_XEN_DISK_DEVICE;
> +    } else if (!strcmp(device_type, "cdrom")) {
> +        type = TYPE_XEN_CDROM_DEVICE;
> +    } else {
> +        error_setg(errp, "invalid device-type parameter '%s'",
> device_type);
> +        goto fail;
> +    }
> +
> +    drive = xen_block_drive_create(vdev, device_type, opts, &local_err);
> +    if (!drive) {
> +        error_propagate_prepend(errp, local_err, "failed to create drive:
> ");
> +        goto fail;
> +    }
> +
> +    iothread = xen_block_iothread_create(vdev, &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err,
> +                                "failed to create iothread: ");
> +        goto fail;
> +    }
> +
> +    xendev = XEN_DEVICE(qdev_create(BUS(xenbus), type));
> +    blockdev = XEN_BLOCK_DEVICE(xendev);
> +
> +    object_property_set_str(OBJECT(xendev), vdev, "vdev", &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err, "failed to set 'vdev':
> ");
> +        goto fail;
> +    }
> +
> +    object_property_set_str(OBJECT(xendev),
> +                            xen_block_drive_get_node_name(drive),
> "drive",
> +                            &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err, "failed to set 'drive':
> ");
> +        goto fail;
> +    }
> +
> +    object_property_set_str(OBJECT(xendev), iothread->id, "iothread",
> +                            &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err,
> +                                "failed to set 'iothread': ");
> +        goto fail;
> +    }
> +
> +    blockdev->iothread = iothread;
> +    blockdev->drive = drive;
> +
> +    object_property_set_bool(OBJECT(xendev), true, "realized",
> &local_err);
> +    if (local_err) {
> +        error_propagate_prepend(errp, local_err,
> +                                "realization of device %s failed: ",
> +                                type);
> +        goto fail;
> +    }
> +
> +    xen_backend_set_device(backend, xendev);
> +    return;
> +
> +fail:
> +    if (xendev) {
> +        object_unparent(OBJECT(xendev));
> +    }
> +
> +    if (iothread) {
> +        xen_block_iothread_destroy(iothread, NULL);
> +    }
> +
> +    if (drive) {
> +        xen_block_drive_destroy(drive, NULL);
> +    }
> +}
> +
> +static void xen_block_device_destroy(XenBackendInstance *backend,
> +                                     Error **errp)
> +{
> +    XenDevice *xendev = xen_backend_get_device(backend);
> +    XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
> +    XenBlockVdev *vdev = &blockdev->props.vdev;
> +    XenBlockDrive *drive = blockdev->drive;
> +    XenBlockIOThread *iothread = blockdev->iothread;
> +
> +    trace_xen_block_device_destroy(vdev->number);
> +
> +    object_unparent(OBJECT(xendev));
> +
> +    if (iothread) {
> +        Error *local_err = NULL;
> +
> +        xen_block_iothread_destroy(iothread, &local_err);
> +        if (local_err) {
> +            error_propagate_prepend(errp, local_err,
> +                                "failed to destroy iothread: ");
> +            return;
> +        }
> +    }
> +
> +    if (drive) {
> +        Error *local_err = NULL;
> +
> +        xen_block_drive_destroy(drive, &local_err);
> +        if (local_err) {
> +            error_propagate_prepend(errp, local_err,
> +                                "failed to destroy drive: ");
> +        }
> +    }
> +}
> +
> +static const XenBackendInfo xen_block_backend_info = {
> +    .type = "qdisk",
> +    .create = xen_block_device_create,
> +    .destroy = xen_block_device_destroy,
> +};
> +
> +static void xen_block_register_backend(void)
> +{
> +    xen_backend_register(&xen_block_backend_info);
> +}
> +
> +xen_backend_init(xen_block_register_backend);
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index 0c26023799..fb227de35d 100644
> --- a/hw/xen/xen-legacy-backend.c
> +++ b/hw/xen/xen-legacy-backend.c
> @@ -753,7 +753,6 @@ void xen_be_register_common(void)
> 
>      xen_be_register("console", &xen_console_ops);
>      xen_be_register("vkbd", &xen_kbdmouse_ops);
> -    xen_be_register("qdisk", &xen_blkdev_ops);
>  #ifdef CONFIG_VIRTFS
>      xen_be_register("9pfs", &xen_9pfs_ops);
>  #endif
> diff --git a/include/hw/xen/xen-block.h b/include/hw/xen/xen-block.h
> index c4223f9be1..6f5d675edb 100644
> --- a/include/hw/xen/xen-block.h
> +++ b/include/hw/xen/xen-block.h
> @@ -29,6 +29,7 @@ typedef struct XenBlockVdev {
>      unsigned long number;
>  } XenBlockVdev;
> 
> +
>  typedef struct XenBlockProperties {
>      XenBlockVdev vdev;
>      BlockConf conf;
> @@ -36,12 +37,24 @@ typedef struct XenBlockProperties {
>      IOThread *iothread;
>  } XenBlockProperties;
> 
> +typedef struct XenBlockDrive {
> +    char *id;
> +    char *node_name[2];
> +    unsigned int layers;
> +} XenBlockDrive;
> +
> +typedef struct XenBlockIOThread {
> +    char *id;
> +} XenBlockIOThread;
> +
>  typedef struct XenBlockDevice {
>      XenDevice xendev;
>      XenBlockProperties props;
>      const char *device_type;
>      unsigned int info;
>      XenBlockDataPlane *dataplane;
> +    XenBlockDrive *drive;
> +    XenBlockIOThread *iothread;
>  } XenBlockDevice;
> 
>  typedef void (*XenBlockDeviceRealize)(XenBlockDevice *blockdev, Error
> **errp);
> --
> 2.20.1.2.gb21ebb6

Almost done, there is one thing left which I believe is an issue.
Whenever I attach a raw file to QEMU, it print:
    qemu-system-i386: warning: Opening a block device as a file using the 'file' driver is deprecated

So, I think the comment below isn't true. We should create a "raw"
driver for "raw" files.

On Thu, Dec 20, 2018 at 05:14:37PM +0000, Paul Durrant wrote:
> +static XenBlockDrive *xen_block_drive_create(const char *id,
> +                                             const char *device_type,
> +                                             QDict *opts, Error **errp)
> +{
...

> +    if (params) {
> +        char **v = g_strsplit(params, ":", 2);
> +
> +        if (v[1] == NULL) {
> +            filename = g_strdup(v[0]);
> +            driver = g_strdup("file");
> +        } else {
> +            if (strcmp(v[0], "aio") == 0) {
> +                driver = g_strdup("file");
> +            } else if (strcmp(v[0], "vhd") == 0) {
> +                driver = g_strdup("vpc");
> +            } else {
> +                driver = g_strdup(v[0]);
> +            }
> +            filename = g_strdup(v[1]);
> +        }
> +
> +        g_strfreev(v);
> +    }
> +
...

> +    /* If the image is a raw file then we are done */

raw files should use the "raw" driver, so we aren't done yet.

> +    if (!strcmp(driver, "file")) {
> +        goto done;
> +    }

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 04 January 2019 16:31
> 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 v7 16/18] xen: automatically create XenBlockDevice-s
> 
> Almost done, there is one thing left which I believe is an issue.
> Whenever I attach a raw file to QEMU, it print:
>     qemu-system-i386: warning: Opening a block device as a file using the
> 'file' driver is deprecated

Oh, I'd not noticed that... but then I only use raw files occasionally.

> 
> So, I think the comment below isn't true. We should create a "raw"
> driver for "raw" files.
> 
> On Thu, Dec 20, 2018 at 05:14:37PM +0000, Paul Durrant wrote:
> > +static XenBlockDrive *xen_block_drive_create(const char *id,
> > +                                             const char *device_type,
> > +                                             QDict *opts, Error **errp)
> > +{
> ...
> 
> > +    if (params) {
> > +        char **v = g_strsplit(params, ":", 2);
> > +
> > +        if (v[1] == NULL) {
> > +            filename = g_strdup(v[0]);
> > +            driver = g_strdup("file");
> > +        } else {
> > +            if (strcmp(v[0], "aio") == 0) {
> > +                driver = g_strdup("file");
> > +            } else if (strcmp(v[0], "vhd") == 0) {
> > +                driver = g_strdup("vpc");
> > +            } else {
> > +                driver = g_strdup(v[0]);
> > +            }
> > +            filename = g_strdup(v[1]);
> > +        }
> > +
> > +        g_strfreev(v);
> > +    }
> > +
> ...
> 
> > +    /* If the image is a raw file then we are done */
> 
> raw files should use the "raw" driver, so we aren't done yet.
> 

Ok. Having a strictly 2-layer stack actually makes things simpler anyway :-)

  Paul

> > +    if (!strcmp(driver, "file")) {
> > +        goto done;
> > +    }
> 
> --
> Anthony PERARD

Am 04.01.2019 um 17:40 hat Paul Durrant geschrieben:
> > -----Original Message-----
> > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > Sent: 04 January 2019 16:31
> > 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 v7 16/18] xen: automatically create XenBlockDevice-s
> > 
> > Almost done, there is one thing left which I believe is an issue.
> > Whenever I attach a raw file to QEMU, it print:
> >     qemu-system-i386: warning: Opening a block device as a file using the
> > 'file' driver is deprecated
> 
> Oh, I'd not noticed that... but then I only use raw files occasionally.

Strictly speaking, this is not about raw (regular) files, but raw block
devices. 'file' is fine for actual regular files, but the protocol
driver for block devices is 'host_device'.

> > raw files should use the "raw" driver, so we aren't done yet.
> 
> Ok. Having a strictly 2-layer stack actually makes things simpler anyway :-)

Using 'raw' there will make the block layer auto-detect the right
protocol layer, so this works. If you want to avoid the second layer,
you'd have to figure out manually whether to use 'file' or
'host_device'.

Kevin

> -----Original Message-----
> From: Kevin Wolf [mailto:kwolf@redhat.com]
> Sent: 08 January 2019 12:53
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>; qemu-devel@nongnu.org;
> qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Max Reitz
> <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
> 
> Am 04.01.2019 um 17:40 hat Paul Durrant geschrieben:
> > > -----Original Message-----
> > > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > > Sent: 04 January 2019 16:31
> > > 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 v7 16/18] xen: automatically create
> XenBlockDevice-s
> > >
> > > Almost done, there is one thing left which I believe is an issue.
> > > Whenever I attach a raw file to QEMU, it print:
> > >     qemu-system-i386: warning: Opening a block device as a file using
> the
> > > 'file' driver is deprecated
> >
> > Oh, I'd not noticed that... but then I only use raw files occasionally.
> 
> Strictly speaking, this is not about raw (regular) files, but raw block
> devices. 'file' is fine for actual regular files, but the protocol
> driver for block devices is 'host_device'.
> 
> > > raw files should use the "raw" driver, so we aren't done yet.
> >
> > Ok. Having a strictly 2-layer stack actually makes things simpler anyway
> :-)
> 
> Using 'raw' there will make the block layer auto-detect the right
> protocol layer, so this works. If you want to avoid the second layer,
> you'd have to figure out manually whether to use 'file' or
> 'host_device'.

Thanks for the explanation. I'll give it a spin using a device... I've posted v8 but, given what you say, I'm still not sure I have it right.

  Paul

> 
> Kevin

On Tue, Jan 08, 2019 at 01:07:49PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > Sent: 08 January 2019 12:53
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: Anthony Perard <anthony.perard@citrix.com>; qemu-devel@nongnu.org;
> > qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Max Reitz
> > <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> > Subject: Re: [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
> > 
> > Am 04.01.2019 um 17:40 hat Paul Durrant geschrieben:
> > > > -----Original Message-----
> > > > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > > > Sent: 04 January 2019 16:31
> > > > 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 v7 16/18] xen: automatically create
> > XenBlockDevice-s
> > > >
> > > > Almost done, there is one thing left which I believe is an issue.
> > > > Whenever I attach a raw file to QEMU, it print:
> > > >     qemu-system-i386: warning: Opening a block device as a file using
> > the
> > > > 'file' driver is deprecated
> > >
> > > Oh, I'd not noticed that... but then I only use raw files occasionally.
> > 
> > Strictly speaking, this is not about raw (regular) files, but raw block
> > devices. 'file' is fine for actual regular files, but the protocol
> > driver for block devices is 'host_device'.
> > 
> > > > raw files should use the "raw" driver, so we aren't done yet.
> > >
> > > Ok. Having a strictly 2-layer stack actually makes things simpler anyway
> > :-)
> > 
> > Using 'raw' there will make the block layer auto-detect the right
> > protocol layer, so this works. If you want to avoid the second layer,
> > you'd have to figure out manually whether to use 'file' or
> > 'host_device'.
> 
> Thanks for the explanation. I'll give it a spin using a device... I've posted v8 but, given what you say, I'm still not sure I have it right.

Indeed, in v8, even with the extra 'raw' layer, the warning is still
there. I was trying to understand why, and I only found out today that
we would need to use the 'host_device' driver as explain by Kevin.


BTW Paul, we can simplify the code that manage layers, by not managing
them.
Instead of (in JSON / QMP term):
    { 'driver': 'file', 'filename': '/file', 'node-name': 'node-file' }
    { 'driver': 'qcow2', 'file': 'node-file', 'node-name': 'node-qcow2' }
we can have:
    { 'driver': 'qcow2', 'node-name': 'node-qcow2',
      'file': { 'driver': 'file', 'filename': '/file' } }


Here is the patch I have, fill free to review and squash it, or not:

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 91f5b58993..c6ec1d9543 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -653,43 +653,23 @@ static char *xen_block_blockdev_add(const char *id, QDict *qdict,
 
 static void xen_block_drive_destroy(XenBlockDrive *drive, Error **errp)
 {
-    while (drive->layers-- != 0) {
-        char *node_name = drive->node_name[drive->layers];
+    char *node_name = drive->node_name;
+
+    if (node_name) {
         Error *local_err = NULL;
 
         xen_block_blockdev_del(node_name, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            drive->layers++;
             return;
         }
+        g_free(node_name);
+        drive->node_name = NULL;
     }
     g_free(drive->id);
     g_free(drive);
 }
 
-static void xen_block_drive_layer_add(XenBlockDrive *drive, QDict *qdict,
-                                      Error **errp)
-{
-    unsigned int i = drive->layers;
-    char *node_name;
-
-    g_assert(drive->layers < ARRAY_SIZE(drive->node_name));
-
-    if (i != 0) {
-        /* Link to the lower layer */
-        qdict_put_str(qdict, "file", drive->node_name[i - 1]);
-    }
-
-    node_name = xen_block_blockdev_add(drive->id, qdict, errp);
-    if (!node_name) {
-        return;
-    }
-
-    drive->node_name[i] = node_name;
-    drive->layers++;
-}
-
 static XenBlockDrive *xen_block_drive_create(const char *id,
                                              const char *device_type,
                                              QDict *opts, Error **errp)
@@ -702,7 +682,8 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
     char *filename = NULL;
     XenBlockDrive *drive = NULL;
     Error *local_err = NULL;
-    QDict *qdict;
+    QDict *file_layer;
+    QDict *driver_layer;
 
     if (params) {
         char **v = g_strsplit(params, ":", 2);
@@ -733,13 +714,13 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
     drive = g_new0(XenBlockDrive, 1);
     drive->id = g_strdup(id);
 
-    qdict = qdict_new();
+    file_layer = qdict_new();
 
-    qdict_put_str(qdict, "driver", "file");
-    qdict_put_str(qdict, "filename", filename);
+    qdict_put_str(file_layer, "driver", "file");
+    qdict_put_str(file_layer, "filename", filename);
 
     if (mode && *mode != 'w') {
-        qdict_put_bool(qdict, "read-only", true);
+        qdict_put_bool(file_layer, "read-only", true);
     }
 
     if (direct_io_safe) {
@@ -749,9 +730,9 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
             QDict *cache_qdict = qdict_new();
 
             qdict_put_bool(cache_qdict, "direct", true);
-            qdict_put_obj(qdict, "cache", QOBJECT(cache_qdict));
+            qdict_put_obj(file_layer, "cache", QOBJECT(cache_qdict));
 
-            qdict_put_str(qdict, "aio", "native");
+            qdict_put_str(file_layer, "aio", "native");
         }
     }
 
@@ -759,7 +740,7 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
         unsigned long value;
 
         if (!qemu_strtoul(discard_enable, NULL, 2, &value) && !!value) {
-            qdict_put_str(qdict, "discard", "unmap");
+            qdict_put_str(file_layer, "discard", "unmap");
         }
     }
 
@@ -767,22 +748,16 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
      * It is necessary to turn file locking off as an emulated device
      * may have already opened the same image file.
      */
-    qdict_put_str(qdict, "locking", "off");
-
-    xen_block_drive_layer_add(drive, qdict, &local_err);
-    qobject_unref(qdict);
-
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto done;
-    }
+    qdict_put_str(file_layer, "locking", "off");
 
-    qdict = qdict_new();
+    driver_layer = qdict_new();
 
-    qdict_put_str(qdict, "driver", driver);
+    qdict_put_str(driver_layer, "driver", driver);
+    qdict_put_obj(driver_layer, "file", QOBJECT(file_layer));
 
-    xen_block_drive_layer_add(drive, qdict, &local_err);
-    qobject_unref(qdict);
+    g_assert(!drive->node_name);
+    drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
+                                              &local_err);
 
 done:
     g_free(driver);
@@ -798,7 +773,7 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
 
 static const char *xen_block_drive_get_node_name(XenBlockDrive *drive)
 {
-    return drive->layers ? drive->node_name[drive->layers - 1] : "";
+    return drive->node_name ? drive->node_name : "";
 }
 
 static void xen_block_iothread_destroy(XenBlockIOThread *iothread,
diff --git a/include/hw/xen/xen-block.h b/include/hw/xen/xen-block.h
index 6f5d675edb..11d351b4b3 100644
--- a/include/hw/xen/xen-block.h
+++ b/include/hw/xen/xen-block.h
@@ -39,8 +39,7 @@ typedef struct XenBlockProperties {
 
 typedef struct XenBlockDrive {
     char *id;
-    char *node_name[2];
-    unsigned int layers;
+    char *node_name;
 } XenBlockDrive;
 
 typedef struct XenBlockIOThread {

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 08 January 2019 13:28
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: 'Kevin Wolf' <kwolf@redhat.com>; qemu-devel@nongnu.org; qemu-
> block@nongnu.org; xen-devel@lists.xenproject.org; Max Reitz
> <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
> 
> On Tue, Jan 08, 2019 at 01:07:49PM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Sent: 08 January 2019 12:53
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: Anthony Perard <anthony.perard@citrix.com>; qemu-devel@nongnu.org;
> > > qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Max Reitz
> > > <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> > > Subject: Re: [PATCH v7 16/18] xen: automatically create
> XenBlockDevice-s
> > >
> > > Am 04.01.2019 um 17:40 hat Paul Durrant geschrieben:
> > > > > -----Original Message-----
> > > > > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > > > > Sent: 04 January 2019 16:31
> > > > > 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 v7 16/18] xen: automatically create
> > > XenBlockDevice-s
> > > > >
> > > > > Almost done, there is one thing left which I believe is an issue.
> > > > > Whenever I attach a raw file to QEMU, it print:
> > > > >     qemu-system-i386: warning: Opening a block device as a file
> using
> > > the
> > > > > 'file' driver is deprecated
> > > >
> > > > Oh, I'd not noticed that... but then I only use raw files
> occasionally.
> > >
> > > Strictly speaking, this is not about raw (regular) files, but raw
> block
> > > devices. 'file' is fine for actual regular files, but the protocol
> > > driver for block devices is 'host_device'.
> > >
> > > > > raw files should use the "raw" driver, so we aren't done yet.
> > > >
> > > > Ok. Having a strictly 2-layer stack actually makes things simpler
> anyway
> > > :-)
> > >
> > > Using 'raw' there will make the block layer auto-detect the right
> > > protocol layer, so this works. If you want to avoid the second layer,
> > > you'd have to figure out manually whether to use 'file' or
> > > 'host_device'.
> >
> > Thanks for the explanation. I'll give it a spin using a device... I've
> posted v8 but, given what you say, I'm still not sure I have it right.
> 
> Indeed, in v8, even with the extra 'raw' layer, the warning is still
> there. I was trying to understand why, and I only found out today that
> we would need to use the 'host_device' driver as explain by Kevin.
> 
> 
> BTW Paul, we can simplify the code that manage layers, by not managing
> them.
> Instead of (in JSON / QMP term):
>     { 'driver': 'file', 'filename': '/file', 'node-name': 'node-file' }
>     { 'driver': 'qcow2', 'file': 'node-file', 'node-name': 'node-qcow2' }
> we can have:
>     { 'driver': 'qcow2', 'node-name': 'node-qcow2',
>       'file': { 'driver': 'file', 'filename': '/file' } }
>

I kind of like the clean separation though... From what Kevin said, it sounds like the lowest layer should use 'raw' instead of 'file' to DTRT, and then we should be back to only needing the single layer in that case. I'll revert back to v7 and give it a try.

  Paul

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Paul Durrant
> Sent: 08 January 2019 14:11
> To: Anthony Perard <anthony.perard@citrix.com>
> Cc: 'Kevin Wolf' <kwolf@redhat.com>; Stefano Stabellini
> <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu-devel@nongnu.org;
> Max Reitz <mreitz@redhat.com>; xen-devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH v7 16/18] xen: automatically create
> XenBlockDevice-s
> 
> > -----Original Message-----
> > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > Sent: 08 January 2019 13:28
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: 'Kevin Wolf' <kwolf@redhat.com>; qemu-devel@nongnu.org; qemu-
> > block@nongnu.org; xen-devel@lists.xenproject.org; Max Reitz
> > <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> > Subject: Re: [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
> >
> > On Tue, Jan 08, 2019 at 01:07:49PM +0000, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > Sent: 08 January 2019 12:53
> > > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > > Cc: Anthony Perard <anthony.perard@citrix.com>; qemu-
> devel@nongnu.org;
> > > > qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Max Reitz
> > > > <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> > > > Subject: Re: [PATCH v7 16/18] xen: automatically create
> > XenBlockDevice-s
> > > >
> > > > Am 04.01.2019 um 17:40 hat Paul Durrant geschrieben:
> > > > > > -----Original Message-----
> > > > > > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > > > > > Sent: 04 January 2019 16:31
> > > > > > 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 v7 16/18] xen: automatically create
> > > > XenBlockDevice-s
> > > > > >
> > > > > > Almost done, there is one thing left which I believe is an
> issue.
> > > > > > Whenever I attach a raw file to QEMU, it print:
> > > > > >     qemu-system-i386: warning: Opening a block device as a file
> > using
> > > > the
> > > > > > 'file' driver is deprecated
> > > > >
> > > > > Oh, I'd not noticed that... but then I only use raw files
> > occasionally.
> > > >
> > > > Strictly speaking, this is not about raw (regular) files, but raw
> > block
> > > > devices. 'file' is fine for actual regular files, but the protocol
> > > > driver for block devices is 'host_device'.
> > > >
> > > > > > raw files should use the "raw" driver, so we aren't done yet.
> > > > >
> > > > > Ok. Having a strictly 2-layer stack actually makes things simpler
> > anyway
> > > > :-)
> > > >
> > > > Using 'raw' there will make the block layer auto-detect the right
> > > > protocol layer, so this works. If you want to avoid the second
> layer,
> > > > you'd have to figure out manually whether to use 'file' or
> > > > 'host_device'.
> > >
> > > Thanks for the explanation. I'll give it a spin using a device... I've
> > posted v8 but, given what you say, I'm still not sure I have it right.
> >
> > Indeed, in v8, even with the extra 'raw' layer, the warning is still
> > there. I was trying to understand why, and I only found out today that
> > we would need to use the 'host_device' driver as explain by Kevin.
> >
> >
> > BTW Paul, we can simplify the code that manage layers, by not managing
> > them.
> > Instead of (in JSON / QMP term):
> >     { 'driver': 'file', 'filename': '/file', 'node-name': 'node-file' }
> >     { 'driver': 'qcow2', 'file': 'node-file', 'node-name': 'node-qcow2'
> }
> > we can have:
> >     { 'driver': 'qcow2', 'node-name': 'node-qcow2',
> >       'file': { 'driver': 'file', 'filename': '/file' } }
> >
> 
> I kind of like the clean separation though... From what Kevin said, it
> sounds like the lowest layer should use 'raw' instead of 'file' to DTRT,
> and then we should be back to only needing the single layer in that case.
> I'll revert back to v7 and give it a try.

No, that doesn't work as we can't deal with locking correctly unless we use driver=file so maybe I misunderstood.

  Paul

> 
>   Paul
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 08 January 2019 13:28
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: 'Kevin Wolf' <kwolf@redhat.com>; qemu-devel@nongnu.org; qemu-
> block@nongnu.org; xen-devel@lists.xenproject.org; Max Reitz
> <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
> 
> On Tue, Jan 08, 2019 at 01:07:49PM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > Sent: 08 January 2019 12:53
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: Anthony Perard <anthony.perard@citrix.com>; qemu-devel@nongnu.org;
> > > qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Max Reitz
> > > <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> > > Subject: Re: [PATCH v7 16/18] xen: automatically create
> XenBlockDevice-s
> > >
> > > Am 04.01.2019 um 17:40 hat Paul Durrant geschrieben:
> > > > > -----Original Message-----
> > > > > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > > > > Sent: 04 January 2019 16:31
> > > > > 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 v7 16/18] xen: automatically create
> > > XenBlockDevice-s
> > > > >
> > > > > Almost done, there is one thing left which I believe is an issue.
> > > > > Whenever I attach a raw file to QEMU, it print:
> > > > >     qemu-system-i386: warning: Opening a block device as a file
> using
> > > the
> > > > > 'file' driver is deprecated
> > > >
> > > > Oh, I'd not noticed that... but then I only use raw files
> occasionally.
> > >
> > > Strictly speaking, this is not about raw (regular) files, but raw
> block
> > > devices. 'file' is fine for actual regular files, but the protocol
> > > driver for block devices is 'host_device'.
> > >
> > > > > raw files should use the "raw" driver, so we aren't done yet.
> > > >
> > > > Ok. Having a strictly 2-layer stack actually makes things simpler
> anyway
> > > :-)
> > >
> > > Using 'raw' there will make the block layer auto-detect the right
> > > protocol layer, so this works. If you want to avoid the second layer,
> > > you'd have to figure out manually whether to use 'file' or
> > > 'host_device'.
> >
> > Thanks for the explanation. I'll give it a spin using a device... I've
> posted v8 but, given what you say, I'm still not sure I have it right.
> 
> Indeed, in v8, even with the extra 'raw' layer, the warning is still
> there. I was trying to understand why, and I only found out today that
> we would need to use the 'host_device' driver as explain by Kevin.
> 
> 
> BTW Paul, we can simplify the code that manage layers, by not managing
> them.
> Instead of (in JSON / QMP term):
>     { 'driver': 'file', 'filename': '/file', 'node-name': 'node-file' }
>     { 'driver': 'qcow2', 'file': 'node-file', 'node-name': 'node-qcow2' }
> we can have:
>     { 'driver': 'qcow2', 'node-name': 'node-qcow2',
>       'file': { 'driver': 'file', 'filename': '/file' } }
> 
> 
> Here is the patch I have, fill free to review and squash it, or not:

I've tested your patch and it does seem like the best way forward. I'll squash it in. Do you want me to put your S-o-b on the combined patch?

  Paul

> 
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 91f5b58993..c6ec1d9543 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -653,43 +653,23 @@ static char *xen_block_blockdev_add(const char *id,
> QDict *qdict,
> 
>  static void xen_block_drive_destroy(XenBlockDrive *drive, Error **errp)
>  {
> -    while (drive->layers-- != 0) {
> -        char *node_name = drive->node_name[drive->layers];
> +    char *node_name = drive->node_name;
> +
> +    if (node_name) {
>          Error *local_err = NULL;
> 
>          xen_block_blockdev_del(node_name, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> -            drive->layers++;
>              return;
>          }
> +        g_free(node_name);
> +        drive->node_name = NULL;
>      }
>      g_free(drive->id);
>      g_free(drive);
>  }
> 
> -static void xen_block_drive_layer_add(XenBlockDrive *drive, QDict *qdict,
> -                                      Error **errp)
> -{
> -    unsigned int i = drive->layers;
> -    char *node_name;
> -
> -    g_assert(drive->layers < ARRAY_SIZE(drive->node_name));
> -
> -    if (i != 0) {
> -        /* Link to the lower layer */
> -        qdict_put_str(qdict, "file", drive->node_name[i - 1]);
> -    }
> -
> -    node_name = xen_block_blockdev_add(drive->id, qdict, errp);
> -    if (!node_name) {
> -        return;
> -    }
> -
> -    drive->node_name[i] = node_name;
> -    drive->layers++;
> -}
> -
>  static XenBlockDrive *xen_block_drive_create(const char *id,
>                                               const char *device_type,
>                                               QDict *opts, Error **errp)
> @@ -702,7 +682,8 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
>      char *filename = NULL;
>      XenBlockDrive *drive = NULL;
>      Error *local_err = NULL;
> -    QDict *qdict;
> +    QDict *file_layer;
> +    QDict *driver_layer;
> 
>      if (params) {
>          char **v = g_strsplit(params, ":", 2);
> @@ -733,13 +714,13 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
>      drive = g_new0(XenBlockDrive, 1);
>      drive->id = g_strdup(id);
> 
> -    qdict = qdict_new();
> +    file_layer = qdict_new();
> 
> -    qdict_put_str(qdict, "driver", "file");
> -    qdict_put_str(qdict, "filename", filename);
> +    qdict_put_str(file_layer, "driver", "file");
> +    qdict_put_str(file_layer, "filename", filename);
> 
>      if (mode && *mode != 'w') {
> -        qdict_put_bool(qdict, "read-only", true);
> +        qdict_put_bool(file_layer, "read-only", true);
>      }
> 
>      if (direct_io_safe) {
> @@ -749,9 +730,9 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
>              QDict *cache_qdict = qdict_new();
> 
>              qdict_put_bool(cache_qdict, "direct", true);
> -            qdict_put_obj(qdict, "cache", QOBJECT(cache_qdict));
> +            qdict_put_obj(file_layer, "cache", QOBJECT(cache_qdict));
> 
> -            qdict_put_str(qdict, "aio", "native");
> +            qdict_put_str(file_layer, "aio", "native");
>          }
>      }
> 
> @@ -759,7 +740,7 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
>          unsigned long value;
> 
>          if (!qemu_strtoul(discard_enable, NULL, 2, &value) && !!value) {
> -            qdict_put_str(qdict, "discard", "unmap");
> +            qdict_put_str(file_layer, "discard", "unmap");
>          }
>      }
> 
> @@ -767,22 +748,16 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
>       * It is necessary to turn file locking off as an emulated device
>       * may have already opened the same image file.
>       */
> -    qdict_put_str(qdict, "locking", "off");
> -
> -    xen_block_drive_layer_add(drive, qdict, &local_err);
> -    qobject_unref(qdict);
> -
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        goto done;
> -    }
> +    qdict_put_str(file_layer, "locking", "off");
> 
> -    qdict = qdict_new();
> +    driver_layer = qdict_new();
> 
> -    qdict_put_str(qdict, "driver", driver);
> +    qdict_put_str(driver_layer, "driver", driver);
> +    qdict_put_obj(driver_layer, "file", QOBJECT(file_layer));
> 
> -    xen_block_drive_layer_add(drive, qdict, &local_err);
> -    qobject_unref(qdict);
> +    g_assert(!drive->node_name);
> +    drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
> +                                              &local_err);
> 
>  done:
>      g_free(driver);
> @@ -798,7 +773,7 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
> 
>  static const char *xen_block_drive_get_node_name(XenBlockDrive *drive)
>  {
> -    return drive->layers ? drive->node_name[drive->layers - 1] : "";
> +    return drive->node_name ? drive->node_name : "";
>  }
> 
>  static void xen_block_iothread_destroy(XenBlockIOThread *iothread,
> diff --git a/include/hw/xen/xen-block.h b/include/hw/xen/xen-block.h
> index 6f5d675edb..11d351b4b3 100644
> --- a/include/hw/xen/xen-block.h
> +++ b/include/hw/xen/xen-block.h
> @@ -39,8 +39,7 @@ typedef struct XenBlockProperties {
> 
>  typedef struct XenBlockDrive {
>      char *id;
> -    char *node_name[2];
> -    unsigned int layers;
> +    char *node_name;
>  } XenBlockDrive;
> 
>  typedef struct XenBlockIOThread {
> --
> Anthony PERARD
> 
> --
> Anthony PERARD

> I've tested your patch and it does seem like the best way forward. I'll squash it in. Do you want me to put your S-o-b on the combined patch?

You can, I'll have to add it anyway when I'll prepare the pull request.

Thanks,

Am 08.01.2019 um 15:20 hat Paul Durrant geschrieben:
> > -----Original Message-----
> > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> > Of Paul Durrant
> > Sent: 08 January 2019 14:11
> > To: Anthony Perard <anthony.perard@citrix.com>
> > Cc: 'Kevin Wolf' <kwolf@redhat.com>; Stefano Stabellini
> > <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu-devel@nongnu.org;
> > Max Reitz <mreitz@redhat.com>; xen-devel@lists.xenproject.org
> > Subject: Re: [Xen-devel] [PATCH v7 16/18] xen: automatically create
> > XenBlockDevice-s
> > 
> > > -----Original Message-----
> > > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > > Sent: 08 January 2019 13:28
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: 'Kevin Wolf' <kwolf@redhat.com>; qemu-devel@nongnu.org; qemu-
> > > block@nongnu.org; xen-devel@lists.xenproject.org; Max Reitz
> > > <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> > > Subject: Re: [PATCH v7 16/18] xen: automatically create XenBlockDevice-s
> > >
> > > On Tue, Jan 08, 2019 at 01:07:49PM +0000, Paul Durrant wrote:
> > > > > -----Original Message-----
> > > > > From: Kevin Wolf [mailto:kwolf@redhat.com]
> > > > > Sent: 08 January 2019 12:53
> > > > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > > > Cc: Anthony Perard <anthony.perard@citrix.com>; qemu-
> > devel@nongnu.org;
> > > > > qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Max Reitz
> > > > > <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> > > > > Subject: Re: [PATCH v7 16/18] xen: automatically create
> > > XenBlockDevice-s
> > > > >
> > > > > Am 04.01.2019 um 17:40 hat Paul Durrant geschrieben:
> > > > > > > -----Original Message-----
> > > > > > > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > > > > > > Sent: 04 January 2019 16:31
> > > > > > > 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 v7 16/18] xen: automatically create
> > > > > XenBlockDevice-s
> > > > > > >
> > > > > > > Almost done, there is one thing left which I believe is an
> > issue.
> > > > > > > Whenever I attach a raw file to QEMU, it print:
> > > > > > >     qemu-system-i386: warning: Opening a block device as a file
> > > using
> > > > > the
> > > > > > > 'file' driver is deprecated
> > > > > >
> > > > > > Oh, I'd not noticed that... but then I only use raw files
> > > occasionally.
> > > > >
> > > > > Strictly speaking, this is not about raw (regular) files, but raw
> > > block
> > > > > devices. 'file' is fine for actual regular files, but the protocol
> > > > > driver for block devices is 'host_device'.
> > > > >
> > > > > > > raw files should use the "raw" driver, so we aren't done yet.
> > > > > >
> > > > > > Ok. Having a strictly 2-layer stack actually makes things simpler
> > > anyway
> > > > > :-)
> > > > >
> > > > > Using 'raw' there will make the block layer auto-detect the right
> > > > > protocol layer, so this works. If you want to avoid the second
> > layer,
> > > > > you'd have to figure out manually whether to use 'file' or
> > > > > 'host_device'.
> > > >
> > > > Thanks for the explanation. I'll give it a spin using a device... I've
> > > posted v8 but, given what you say, I'm still not sure I have it right.
> > >
> > > Indeed, in v8, even with the extra 'raw' layer, the warning is still
> > > there. I was trying to understand why, and I only found out today that
> > > we would need to use the 'host_device' driver as explain by Kevin.
> > >
> > >
> > > BTW Paul, we can simplify the code that manage layers, by not managing
> > > them.
> > > Instead of (in JSON / QMP term):
> > >     { 'driver': 'file', 'filename': '/file', 'node-name': 'node-file' }
> > >     { 'driver': 'qcow2', 'file': 'node-file', 'node-name': 'node-qcow2'
> > }
> > > we can have:
> > >     { 'driver': 'qcow2', 'node-name': 'node-qcow2',
> > >       'file': { 'driver': 'file', 'filename': '/file' } }
> > >
> > 
> > I kind of like the clean separation though... From what Kevin said, it
> > sounds like the lowest layer should use 'raw' instead of 'file' to DTRT,
> > and then we should be back to only needing the single layer in that case.
> > I'll revert back to v7 and give it a try.
> 
> No, that doesn't work as we can't deal with locking correctly unless
> we use driver=file so maybe I misunderstood.

The lowest layer should use 'host_device' there. 'raw' is the format
layer that is stacked on top.

I'm not sure what your locking requirements are, but 'host_device'
supports the same options as 'file'.

Kevin