From: Paul Durrant <Paul.Durrant@citrix.com>
To: Anthony Perard <anthony.perard@citrix.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
Stefano Stabellini <sstabellini@kernel.org>,
"qemu-block@nongnu.org" <qemu-block@nongnu.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
Max Reitz <mreitz@redhat.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v6 16/18] xen: automatically create XenBlockDevice-s
Date: Wed, 19 Dec 2018 12:43:24 +0000 [thread overview]
Message-ID: <40bdef2962864ca8bc364249196bf9b3__229.499911847775$1545223344$gmane$org@AMSPEX02CL03.citrite.net> (raw)
In-Reply-To: <20181219123926.GA1288@perard.uk.xensource.com>
> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 19 December 2018 12:39
> 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 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2018-12-19 12:43 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-17 13:29 [PATCH v6 00/18] Xen PV backend 'qdevification' Paul Durrant
2018-12-17 13:29 ` [PATCH v6 01/18] xen: re-name XenDevice to XenLegacyDevice Paul Durrant
2018-12-17 13:29 ` [PATCH v6 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy Paul Durrant
2018-12-17 13:29 ` [PATCH v6 03/18] xen: introduce 'xen-block', 'xen-disk' and 'xen-cdrom' Paul Durrant
2018-12-17 13:29 ` [PATCH v6 04/18] xen: create xenstore areas for XenDevice-s Paul Durrant
2018-12-17 13:29 ` [PATCH v6 05/18] xen: add xenstore watcher infrastructure Paul Durrant
2018-12-17 13:29 ` [PATCH v6 06/18] xen: add grant table interface for XenDevice-s Paul Durrant
2018-12-17 13:30 ` [PATCH v6 07/18] xen: add event channel " Paul Durrant
2018-12-17 13:30 ` [PATCH v6 08/18] xen: duplicate xen_disk.c as basis of dataplane/xen-block.c Paul Durrant
2018-12-17 13:30 ` [PATCH v6 09/18] xen: remove unnecessary code from dataplane/xen-block.c Paul Durrant
2018-12-17 13:30 ` [PATCH v6 10/18] xen: add header and build dataplane/xen-block.c Paul Durrant
2018-12-17 13:30 ` [PATCH v6 11/18] xen: remove 'XenBlkDev' and 'blkdev' names from dataplane/xen-block Paul Durrant
2018-12-17 13:30 ` [PATCH v6 12/18] xen: remove 'ioreq' struct/varable/field names from dataplane/xen-block.c Paul Durrant
2018-12-17 13:30 ` [PATCH v6 13/18] xen: purge 'blk' and 'ioreq' from function names in dataplane/xen-block.c Paul Durrant
2018-12-17 13:30 ` [PATCH v6 14/18] xen: add implementations of xen-block connect and disconnect functions Paul Durrant
2018-12-17 13:30 ` [PATCH v6 15/18] xen: add a mechanism to automatically create XenDevice-s Paul Durrant
2018-12-19 12:40 ` Anthony PERARD
2018-12-17 13:30 ` [PATCH v6 16/18] xen: automatically create XenBlockDevice-s Paul Durrant
2018-12-17 13:30 ` [PATCH v6 17/18] MAINTAINERS: add myself as a Xen maintainer Paul Durrant
2018-12-17 13:30 ` [PATCH v6 18/18] xen: remove the legacy 'xen_disk' backend Paul Durrant
[not found] ` <20181217133011.31433-17-paul.durrant@citrix.com>
2018-12-19 12:39 ` [PATCH v6 16/18] xen: automatically create XenBlockDevice-s Anthony PERARD
[not found] ` <20181219123926.GA1288@perard.uk.xensource.com>
2018-12-19 12:43 ` Paul Durrant [this message]
[not found] ` <40bdef2962864ca8bc364249196bf9b3@AMSPEX02CL03.citrite.net>
2018-12-19 14:01 ` Anthony PERARD
2018-12-19 14:03 ` Paul Durrant
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='40bdef2962864ca8bc364249196bf9b3__229.499911847775$1545223344$gmane$org@AMSPEX02CL03.citrite.net' \
--to=paul.durrant@citrix.com \
--cc=anthony.perard@citrix.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).