On Tue, Jan 08, 2019 at 01:07:49PM +0000, Paul Durrant wrote: > > -----Original Message----- > > From: Kevin Wolf [mailto:kw...@redhat.com] > > Sent: 08 January 2019 12:53 > > To: Paul Durrant <paul.durr...@citrix.com> > > Cc: Anthony Perard <anthony.per...@citrix.com>; qemu-de...@nongnu.org; > > qemu-block@nongnu.org; xen-de...@lists.xenproject.org; Max Reitz > > <mre...@redhat.com>; Stefano Stabellini <sstabell...@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.per...@citrix.com] > > > > Sent: 04 January 2019 16:31 > > > > To: Paul Durrant <paul.durr...@citrix.com> > > > > Cc: qemu-de...@nongnu.org; qemu-block@nongnu.org; xen- > > > > de...@lists.xenproject.org; Kevin Wolf <kw...@redhat.com>; Max Reitz > > > > <mre...@redhat.com>; Stefano Stabellini <sstabell...@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 { -- Anthony PERARD -- Anthony PERARD