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

[Paul Durrant]

> -----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