Re: [Qemu-devel] [PATCH v12 3/3] block/gluster: add support for multiple gluster servers

[Jeff Cody]

Hi,

For future revisions, could you send all patches in the series (even
if unchanged)?  It makes it much easier to know which patches are the
latest.

Also, if you can rebase each release on the qemu master branch, that
would be preferred (or on a submaintainers tree, just mention that in
the cover letter / patch).  This didn't apply cleanly to qemu master
as-is, which is why I mentioned it.

Thanks, and some more items below:

On Mon, Nov 09, 2015 at 03:32:13PM +0530, Prasanna Kumar Kalever wrote:
> This patch adds a way to specify multiple volfile servers to the gluster
> block backend of QEMU with tcp|rdma transport types and their port numbers.
> 
> Problem:
> 
> Currently VM Image on gluster volume is specified like this:
> 
> file=gluster[+tcp]://host[:port]/testvol/a.img
> 
> Assuming we have three hosts in trusted pool with replica 3 volume
> in action and unfortunately host (mentioned in the command above) went down
> for some reason, since the volume is replica 3 we now have other 2 hosts
> active from which we can boot the VM.
> 
> But currently there is no mechanism to pass the other 2 gluster host
> addresses to qemu.
> 
> Solution:
> 
> New way of specifying VM Image on gluster volume with volfile servers:
> (We still support old syntax to maintain backward compatibility)
>

[...]

> 
> This patch depends on a recent fix in libgfapi raised as part of this work:
> http://review.gluster.org/#/c/12114/
>

What happens if the user attempts to use this functionality, but does
not have the latest version of libfgapi?  How gracefully do we fail?

> Credits: Sincere thanks to Kevin Wolf <address@hidden> and
> "Deepak C Shetty" <address@hidden> for inputs and all their support
> 
> Signed-off-by: Prasanna Kumar Kalever <address@hidden>
> ---

[...]

> ---
>  block/gluster.c      | 460 
> ++++++++++++++++++++++++++++++++++++++++++++-------
>  qapi/block-core.json |  64 ++++++-
>  2 files changed, 455 insertions(+), 69 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index ededda2..2b944e6 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -11,6 +11,16 @@
>  #include "block/block_int.h"
>  #include "qemu/uri.h"
>

[...]

> +static int qemu_gluster_parsejson(BlockdevOptionsGluster **pgconf,
> +                                  QDict *options)
> +{
> +    QemuOpts *opts;
> +    BlockdevOptionsGluster *gconf;

This (gconf) needs to be initialized to NULL, because 'out' is
reachable before you allocated gconf.

> +    GlusterServer *gsconf;
> +    GlusterServerList **prev;
> +    GlusterServerList *curr;

This (curr) needs to be initialized to NULL (explained below).

> +    QDict *backing_options = NULL;
> +    Error *local_err = NULL;
> +    char *str;

This (str) needs to be initialized to NULL, because 'out' is reachable
before you allocated 'str'.

> +    const char *ptr;
> +    size_t num_servers;
> +    int i;
> +
> +    /* create opts info from runtime_json_opts list */
> +    opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
> +    qemu_opts_absorb_qdict(opts, options, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    gconf = g_new0(BlockdevOptionsGluster, 1);
> +    num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVERS_PATTERN);
> +    if (num_servers < 1) {
> +        error_setg(&local_err, "Error: qemu_gluster: please provide 
> 'servers' "
> +                               "option with valid fields in array of 
> tuples");
> +        goto out;
> +    }

Is there a maximum number of servers that can be specified?

> +
> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
> +    if (!ptr) {
> +        error_setg(&local_err, "Error: qemu_gluster: please provide 'volume' 
> "
> +                               "option");
> +        goto out;
> +    }
> +    gconf->volume = g_strdup(ptr);
> +
> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
> +    if (!ptr) {
> +        error_setg(&local_err, "Error: qemu_gluster: please provide 'path' "
> +                               "option");
> +        goto out;
> +    }
> +    gconf->path = g_strdup(ptr);
> +
> +    qemu_opts_del(opts);
> +
> +    /* create opts info from runtime_tuple_opts list */
> +    str = g_malloc(40);
> +    for (i = 0; i < num_servers; i++) {
> +        opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort);
> +        g_assert(sprintf(str, GLUSTER_OPT_SERVERS_PATTERN"%d.", i) <= 40);

Is 40 characters just the assumed maximum string length?  I know that
GLUSTER_OPT_SERVERS_PATTERN is pretty short, but it concerns me seeing
40 characters hardcoded in the malloc and check here.   It would be
better to use the maximum string size of the resulting string, for the
known maximum number of servers.

Also, using g_assert() here doesn't stop overwriting 'str', it just
alerts that it has happened.  We should use snprintf() instead (and we
could probably return an error rather than abort).


> +        qdict_extract_subqdict(options, &backing_options, str);
> +        qemu_opts_absorb_qdict(opts, backing_options, &local_err);
> +        if (local_err) {
> +            goto out;
> +        }
> +        qdict_del(backing_options, str);
> +
> +        ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
> +        if (!ptr) {
> +            error_setg(&local_err, "Error: qemu_gluster: servers.{tuple.%d} "
> +                                   "requires 'host' option", i);
> +            goto out;
> +        }
> +
> +        gsconf = g_new0(GlusterServer, 1);
> +
> +        gsconf->host = g_strdup(ptr);
> +
> +        ptr = qemu_opt_get(opts, GLUSTER_OPT_TRANSPORT);
> +        /* check whether transport type specified in json command is valid */
> +        if (parse_transport_option(ptr) < 0) {
> +            error_setg(&local_err, "Error: qemu_gluster: please set 
> 'transport'"
> +                                   " type in tuple.%d as tcp or rdma", i);
> +            goto out;
> +        }
> +        /* only if valid transport i.e. either of tcp|rdma is specified */
> +        gsconf->transport = parse_transport_option(ptr);
> +        gsconf->has_transport = true;
> +
> +        gsconf->port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT,
> +                                                    GLUSTER_DEFAULT_PORT);
> +        gsconf->has_port = true;
> +
> +        if (gconf->servers == NULL) {
> +            gconf->servers = g_new0(GlusterServerList, 1);
> +            gconf->servers->value = gsconf;
> +            curr = gconf->servers;
> +        } else {
> +            prev = &curr->next;

curr needs to be initialized to NULL, because the compiler has no way
of knowing that the gconf = g_new0() above means gconf->servers will
always be NULL the first iteration.  As-is, this causes a compile
error (ok, warning, but we use -werror).

> +            curr = g_new0(GlusterServerList, 1);
> +            curr->value = gsconf;
> +            *prev = curr;
> +        }
> +        curr->next = NULL;
> +
> +        qemu_opts_del(opts);
> +    }
> +
> +    *pgconf = gconf;
> +    g_free(str);
> +    return 0;
> +
> +out:
> +    error_report_err(local_err);
> +    qapi_free_BlockdevOptionsGluster(gconf);
> +    qemu_opts_del(opts);
> +    if (str) {
> +        qdict_del(backing_options, str);
> +        g_free(str);
> +    }
> +    errno = EINVAL;
> +    return -errno;
> +}
> +

[...]

-Jeff