Re: [Qemu-block] [PATCH v7 16/18] xen: automatically create XenBlockDevice-s

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD [mailto:address@hidden
> Sent: 08 January 2019 13:28
> To: Paul Durrant <address@hidden>
> Cc: 'Kevin Wolf' <address@hidden>; address@hidden; qemu-
> address@hidden; address@hidden; Max Reitz
> <address@hidden>; Stefano Stabellini <address@hidden>
> 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:address@hidden
> > > Sent: 08 January 2019 12:53
> > > To: Paul Durrant <address@hidden>
> > > Cc: Anthony Perard <address@hidden>; address@hidden;
> > > address@hidden; address@hidden; Max Reitz
> > > <address@hidden>; Stefano Stabellini <address@hidden>
> > > 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:address@hidden
> > > > > Sent: 04 January 2019 16:31
> > > > > To: Paul Durrant <address@hidden>
> > > > > Cc: address@hidden; address@hidden; xen-
> > > > > address@hidden; Kevin Wolf <address@hidden>; Max
> Reitz
> > > > > <address@hidden>; Stefano Stabellini <address@hidden>
> > > > > 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