[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v6 16/18] xen: automatically create XenBlockDevi
From: |
Paul Durrant |
Subject: |
Re: [Qemu-block] [PATCH v6 16/18] xen: automatically create XenBlockDevice-s |
Date: |
Wed, 19 Dec 2018 12:43:24 +0000 |
> -----Original Message-----
> From: Anthony PERARD [mailto:address@hidden
> Sent: 19 December 2018 12:39
> 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 v6 16/18] xen: automatically create XenBlockDevice-s
>
> On Mon, Dec 17, 2018 at 01:30:09PM +0000, Paul Durrant wrote:
> > +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 *str = 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);
> > +
> > + qdict_iter(qdict, add_item, &str);
> > +
> > + trace_xen_block_blockdev_add(str);
> > + g_free(str);
> > +
> > + v = qobject_input_visitor_new_flat_confused(qdict, errp);
>
> Kevin seems to say that this could be done without the _flat_confused
> version. The flat_confused version seems to be useful just because
> the key "cache.direct" is used earlier, and because everything in qdict
> is a string.
It could be, but there's a good reason for wanting everything as a string, and
that is so that I can do a qdict_iter to generate my trace message. Also I
really don't want to get too elaborate here... this is supposed to be mimicking
what would normally come via a json blob, and that would start out as strings.
Paul
>
> I think instead, qobject_input_visitor_new could be used. You would just
> need to replace
> qdict_put_str(qdict, "cache.direct", "on");
> by
> QDict *cache = qdict_new();
> qdict_put_str(cache, "direct", "on");
> qdict_put_obj(qdict, "cache", QOBJECT(cache));
>
> And also the property "read-only" which seems to be a bool as well. I've
> check all property in the qdict, and I think that the only two that
> needs to be changes. And then, we can use:
> v = qobject_input_visitor_new(qdict); which never fails.
>
> You'll just need "qapi/qobject-input-visitor.h" instead of "block/qdict.h"
>
> > + if (!v) {
> > + goto fail;
> > + }
> > +
> > + 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 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_str(qdict, "read-only", "on");
> > + }
> > +
> > + if (direct_io_safe) {
> > + unsigned long value;
> > +
> > + if (!qemu_strtoul(direct_io_safe, NULL, 2, &value) && !!value)
> {
> > + qdict_put_str(qdict, "cache.direct", "on");
> > + 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 */
>
> I don't think that is true, as I have this warning in QEMU:
> qemu-system-i386: warning: Opening a block device as a file using the
> 'file' driver is deprecated
>
> We would need a "raw" driver.
>
> > + 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;
> > +}
>
> --
> Anthony PERARD
- [Qemu-block] [PATCH v6 06/18] xen: add grant table interface for XenDevice-s, (continued)
- [Qemu-block] [PATCH v6 06/18] xen: add grant table interface for XenDevice-s, Paul Durrant, 2018/12/17
- [Qemu-block] [PATCH v6 03/18] xen: introduce 'xen-block', 'xen-disk' and 'xen-cdrom', Paul Durrant, 2018/12/17
- [Qemu-block] [PATCH v6 04/18] xen: create xenstore areas for XenDevice-s, Paul Durrant, 2018/12/17
- [Qemu-block] [PATCH v6 07/18] xen: add event channel interface for XenDevice-s, Paul Durrant, 2018/12/17
- [Qemu-block] [PATCH v6 05/18] xen: add xenstore watcher infrastructure, Paul Durrant, 2018/12/17
- [Qemu-block] [PATCH v6 09/18] xen: remove unnecessary code from dataplane/xen-block.c, Paul Durrant, 2018/12/17
- [Qemu-block] [PATCH v6 01/18] xen: re-name XenDevice to XenLegacyDevice..., Paul Durrant, 2018/12/17
- [Qemu-block] [PATCH v6 08/18] xen: duplicate xen_disk.c as basis of dataplane/xen-block.c, Paul Durrant, 2018/12/17
- [Qemu-block] [PATCH v6 16/18] xen: automatically create XenBlockDevice-s, Paul Durrant, 2018/12/17
[Qemu-block] [PATCH v6 17/18] MAINTAINERS: add myself as a Xen maintainer, Paul Durrant, 2018/12/17
[Qemu-block] [PATCH v6 14/18] xen: add implementations of xen-block connect and disconnect functions..., Paul Durrant, 2018/12/17
[Qemu-block] [PATCH v6 18/18] xen: remove the legacy 'xen_disk' backend, Paul Durrant, 2018/12/17
[Qemu-block] [PATCH v6 15/18] xen: add a mechanism to automatically create XenDevice-s..., Paul Durrant, 2018/12/17
[Qemu-block] [PATCH v6 11/18] xen: remove 'XenBlkDev' and 'blkdev' names from dataplane/xen-block, Paul Durrant, 2018/12/17
[Qemu-block] [PATCH v6 13/18] xen: purge 'blk' and 'ioreq' from function names in dataplane/xen-block.c, Paul Durrant, 2018/12/17
[Qemu-block] [PATCH v6 12/18] xen: remove 'ioreq' struct/varable/field names from dataplane/xen-block.c, Paul Durrant, 2018/12/17