Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[ashish mittal]

On Mon, Mar 27, 2017 at 10:27 AM, Stefan Hajnoczi <address@hidden> wrote:
> On Sun, Mar 26, 2017 at 07:50:35PM -0700, Ashish Mittal wrote:
>
> Have you tested live migration?
>
> If live migration is not supported then a migration blocker should be
> added using migrate_add_blocker().
>

We do support live migration. We have been testing a fork of this code
(slightly different version) with live migration.

>> v10 changelog:
>> (1) Implemented accepting TLS creds per block device via the CLI
>>     (see 3rd e.g in commit log). Corresponding changes made to the
>>     libqnio library.
>> (2) iio_open() changed to accept TLS creds and use these internally
>>     to set up SSL connections.
>> (3) Got rid of hard-coded VXHS_UUID_DEF. qemu_uuid is no longer used
>>     for authentication in any way.
>
> Why does the code still access qemu_uuid and pass the UUID string to
> iio_init()?
>

I was of the opinion that knowing what instance (for qemu-kvm case)
was opening a block device could be a useful piece of information for
the block device to have in the future.

> In libqnio.git (66698ca47bc594a9f623c240d63ea535f5a42b47) the 'instance'
> field is unused and not sent over the wire.  Please drop it.
>

It is not used at present. I will drop it.

>> diff --git a/block/vxhs.c b/block/vxhs.c
>> new file mode 100644
>> index 0000000..b98b535
>> --- /dev/null
>> +++ b/block/vxhs.c
>> @@ -0,0 +1,595 @@
>> +/*
>> + * QEMU Block driver for Veritas HyperScale (VxHS)
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include <qnio/qnio_api.h>
>> +#include <sys/param.h>
>> +#include "block/block_int.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "qapi/qmp/qdict.h"
>> +#include "qapi/qmp/qstring.h"
>> +#include "trace.h"
>> +#include "qemu/uri.h"
>> +#include "qapi/error.h"
>> +#include "qemu/uuid.h"
>> +#include "crypto/tlscredsx509.h"
>> +
>> +#define VXHS_OPT_FILENAME           "filename"
>> +#define VXHS_OPT_VDISK_ID           "vdisk-id"
>> +#define VXHS_OPT_SERVER             "server"
>> +#define VXHS_OPT_HOST               "host"
>> +#define VXHS_OPT_PORT               "port"
>> +
>> +QemuUUID qemu_uuid __attribute__ ((weak));
>> +
>> +static uint32_t vxhs_ref;
>
> It would be nice to add:
> /* Only accessed under QEMU global mutex */
>
Will do.

>> +/*
>> + * Parse the incoming URI and populate *options with the host information.
>> + * URI syntax has the limitation of supporting only one host info.
>> + * To pass multiple host information, use the JSON syntax.
>
> References to multiple hosts are out of date.  The driver only supports
> a single host now.
>
Will change.

>> + */
>> +static int vxhs_parse_uri(const char *filename, QDict *options)
>> +{
>> +    URI *uri = NULL;
>> +    char *hoststr, *portstr;
>> +    char *port;
>> +    int ret = 0;
>> +
>> +    trace_vxhs_parse_uri_filename(filename);
>> +    uri = uri_parse(filename);
>> +    if (!uri || !uri->server || !uri->path) {
>> +        uri_free(uri);
>> +        return -EINVAL;
>> +    }
>> +
>> +    hoststr = g_strdup(VXHS_OPT_SERVER".host");
>> +    qdict_put(options, hoststr, qstring_from_str(uri->server));
>> +    g_free(hoststr);
>> +
>> +    portstr = g_strdup(VXHS_OPT_SERVER".port");
>> +    if (uri->port) {
>> +        port = g_strdup_printf("%d", uri->port);
>> +        qdict_put(options, portstr, qstring_from_str(port));
>> +        g_free(port);
>> +    }
>> +    g_free(portstr);
>
> The g_strdup()/g_free() isn't necessary for the qdict_put() key
> argument.  The key belongs to the caller so we can pass a string
> literal:

Will Change.

>
>   qdict_put(options, VXHS_OPT_SERVER ".host", qstring_from_str(uri->server));
>   if (uri->port) {
>       port = g_strdup_printf("%d", uri->port);
>       qdict_put(options, VXHS_OPT_SERVER ".port", qstring_from_str(port));
>       g_free(port);
>   }
>
>> +
>> +    if (strstr(uri->path, "vxhs") == NULL) {
>
> What does this check do?
>

Not sure about the history, but it's been there since first code
draft. Will check if it serves any purpose, or remove it.

>> +static int vxhs_open(BlockDriverState *bs, QDict *options,
>> +                     int bdrv_flags, Error **errp)
>> +{
>> +    BDRVVXHSState *s = bs->opaque;
>> +    void *dev_handlep = NULL;
>> +    QDict *backing_options = NULL;
>> +    QemuOpts *opts, *tcp_opts;
>> +    char *of_vsa_addr = NULL;
>> +    Error *local_err = NULL;
>> +    const char *vdisk_id_opt;
>> +    const char *server_host_opt;
>> +    char *str = NULL;
>> +    int ret = 0;
>> +    char *cacert = NULL;
>> +    char *client_key = NULL;
>> +    char *client_cert = NULL;
>> +
>> +    ret = vxhs_init_and_ref();
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    /* Create opts info from runtime_opts and runtime_tcp_opts list */
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>> +    tcp_opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
>> +
>> +    qemu_opts_absorb_qdict(opts, options, &local_err);
>> +    if (local_err) {
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    /* vdisk-id is the disk UUID */
>> +    vdisk_id_opt = qemu_opt_get(opts, VXHS_OPT_VDISK_ID);
>> +    if (!vdisk_id_opt) {
>> +        error_setg(&local_err, QERR_MISSING_PARAMETER, VXHS_OPT_VDISK_ID);
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    /* vdisk-id may contain a leading '/' */
>> +    if (strlen(vdisk_id_opt) > UUID_FMT_LEN + 1) {
>> +        error_setg(&local_err, "vdisk-id cannot be more than %d characters",
>> +                   UUID_FMT_LEN);
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    s->vdisk_guid = g_strdup(vdisk_id_opt);
>> +    trace_vxhs_open_vdiskid(vdisk_id_opt);
>> +
>> +    /* get the 'server.' arguments */
>> +    str = g_strdup_printf(VXHS_OPT_SERVER".");
>> +    qdict_extract_subqdict(options, &backing_options, str);
>
> g_strdup_printf() is unnecessary.  You can eliminate the 'str' local
> variable and just do:
>
>   qdict_extract_subqdict(options, &backing_options, VXHS_OPT_SERVER ".");
>
Will do. Thanks!

>> +
>> +    qemu_opts_absorb_qdict(tcp_opts, backing_options, &local_err);
>> +    if (local_err != NULL) {
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    server_host_opt = qemu_opt_get(tcp_opts, VXHS_OPT_HOST);
>> +    if (!server_host_opt) {
>> +        error_setg(&local_err, QERR_MISSING_PARAMETER,
>> +                   VXHS_OPT_SERVER"."VXHS_OPT_HOST);
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    if (strlen(server_host_opt) > MAXHOSTNAMELEN) {
>> +        error_setg(&local_err, "server.host cannot be more than %d 
>> characters",
>> +                   MAXHOSTNAMELEN);
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    /* check if we got tls-creds via the --object argument */
>> +    s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
>> +    if (s->tlscredsid) {
>> +        vxhs_get_tls_creds(s->tlscredsid, &cacert, &client_key,
>> +                           &client_cert, &local_err);
>> +        if (local_err != NULL) {
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +        trace_vxhs_get_creds(cacert, client_key, client_cert);
>> +    }
>> +
>> +    s->vdisk_hostinfo.host = g_strdup(server_host_opt);
>> +    s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts,
>> +                                                          VXHS_OPT_PORT),
>> +                                                          NULL, 0);
>> +
>> +    trace_vxhs_open_hostinfo(s->vdisk_hostinfo.host,
>> +                             s->vdisk_hostinfo.port);
>> +
>> +    of_vsa_addr = g_strdup_printf("of://%s:%d",
>> +                                  s->vdisk_hostinfo.host,
>> +                                  s->vdisk_hostinfo.port);
>> +
>> +    /*
>> +     * Open qnio channel to storage agent if not opened before
>> +     */
>> +    dev_handlep = iio_open(of_vsa_addr, s->vdisk_guid, 0,
>> +                           cacert, client_key, client_cert);
>> +    if (dev_handlep == NULL) {
>> +        trace_vxhs_open_iio_open(of_vsa_addr);
>> +        ret = -ENODEV;
>> +        goto out;
>> +    }
>> +    s->vdisk_hostinfo.dev_handle = dev_handlep;
>> +
>> +out:
>> +    g_free(str);
>> +    g_free(of_vsa_addr);
>> +    QDECREF(backing_options);
>> +    qemu_opts_del(tcp_opts);
>> +    qemu_opts_del(opts);
>> +    g_free(cacert);
>> +    g_free(client_key);
>> +    g_free(client_cert);
>> +
>> +    if (ret < 0) {
>> +        vxhs_unref();
>> +        error_propagate(errp, local_err);
>> +        g_free(s->vdisk_hostinfo.host);
>> +        g_free(s->vdisk_guid);
>> +        g_free(s->tlscredsid);
>> +        s->vdisk_guid = NULL;
>> +        errno = -ret;
>
> .bdrv_open() does not promise anything about errno.  This line can be
> dropped.
>
Will do.

>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static const AIOCBInfo vxhs_aiocb_info = {
>> +    .aiocb_size = sizeof(VXHSAIOCB)
>> +};
>> +
>> +/*
>> + * This allocates QEMU-VXHS callback for each IO
>> + * and is passed to QNIO. When QNIO completes the work,
>> + * it will be passed back through the callback.
>> + */
>> +static BlockAIOCB *vxhs_aio_rw(BlockDriverState *bs, int64_t sector_num,
>> +                               QEMUIOVector *qiov, int nb_sectors,
>> +                               BlockCompletionFunc *cb, void *opaque,
>> +                               VDISKAIOCmd iodir)
>> +{
>> +    VXHSAIOCB *acb = NULL;
>> +    BDRVVXHSState *s = bs->opaque;
>> +    size_t size;
>> +    uint64_t offset;
>> +    int iio_flags = 0;
>> +    int ret = 0;
>> +    void *dev_handle = s->vdisk_hostinfo.dev_handle;
>> +
>> +    offset = sector_num * BDRV_SECTOR_SIZE;
>> +    size = nb_sectors * BDRV_SECTOR_SIZE;
>> +    acb = qemu_aio_get(&vxhs_aiocb_info, bs, cb, opaque);
>> +
>> +    /*
>> +     * Initialize VXHSAIOCB.
>> +     */
>> +    acb->err = 0;
>> +    acb->qiov = qiov;
>
> This field is unused, please remove it.
>
Yes! Thanks!

>> +static BlockDriver bdrv_vxhs = {
>> +    .format_name                  = "vxhs",
>> +    .protocol_name                = "vxhs",
>> +    .instance_size                = sizeof(BDRVVXHSState),
>> +    .bdrv_file_open               = vxhs_open,
>> +    .bdrv_parse_filename          = vxhs_parse_filename,
>> +    .bdrv_close                   = vxhs_close,
>> +    .bdrv_getlength               = vxhs_getlength,
>> +    .bdrv_aio_readv               = vxhs_aio_readv,
>> +    .bdrv_aio_writev              = vxhs_aio_writev,
>
> Missing .bdrv_aio_flush().  Does VxHS promise that every completed write
> request is persistent?
>

Yes, every acknowledged write request is persistent.

> In that case it may be better to disable the emulated disk write cache
> so the guest operating system and application know not to send flush
> commands.

We do pass "cache=none" on the qemu command line for every block
device. Are there any other code changes necessary? Any pointers will
help.

Thanks,
Ashish