qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 4/4] block/gluster: add support for multiple glu


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 4/4] block/gluster: add support for multiple gluster servers
Date: Thu, 12 Nov 2015 15:36:17 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

On 11/12/2015 03:22 AM, 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.
> 

> This patch gives a mechanism to provide all the server addresses, which are in
> replica set, so in case host1 is down VM can still boot from any of the
> active hosts.
> 
> This is equivalent to the backup-volfile-servers option supported by
> mount.glusterfs (FUSE way of mounting gluster volume)
> 
> 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      | 288 
> ++++++++++++++++++++++++++++++++++++++++++++-------
>  qapi/block-core.json |   4 +-
>  2 files changed, 252 insertions(+), 40 deletions(-)

All right - the diffstat is smaller this time around (288 is nicer than
468 lines changed in v13).  There's always a psychological barrier to
reviewing large patches, and breaking things into bite-sized chunks
helps even if the same amount of work is done overall.

> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 615f28b..ba209cf 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -12,6 +12,13 @@
>  #include "qemu/uri.h"
>  
>  #define GLUSTER_OPT_FILENAME        "filename"
> +#define GLUSTER_OPT_VOLUME          "volume"
> +#define GLUSTER_OPT_PATH            "path"
> +#define GLUSTER_OPT_HOST            "host"
> +#define GLUSTER_OPT_PORT            "port"
> +#define GLUSTER_OPT_TRANSPORT       "transport"
> +#define GLUSTER_OPT_SERVER_PATTERN  "server."
> +
>  #define GLUSTER_DEFAULT_PORT        24007

Once again, I'm jumping to the interface first [1]


> @@ -131,6 +178,7 @@ static int qemu_gluster_parseuri(BlockdevOptionsGluster 
> **pgconf,
>                                   const char *filename)
>  {
>      BlockdevOptionsGluster *gconf;
> +    GlusterServer *gsconf;
>      URI *uri;
>      QueryParams *qp = NULL;
>      bool is_unix = false;
> @@ -142,23 +190,24 @@ static int qemu_gluster_parseuri(BlockdevOptionsGluster 
> **pgconf,
>      }
>  
>      gconf = g_new0(BlockdevOptionsGluster, 1);
> -    gconf->server = g_new0(GlusterServer, 1);
> +    gconf->server = g_new0(GlusterServerList, 1);
> +    gconf->server->value = gsconf = g_new0(GlusterServer, 1);
>  
>      /* transport */
>      if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> -        gconf->server->transport = GLUSTER_TRANSPORT_TCP;
> +        gsconf->transport = GLUSTER_TRANSPORT_TCP;

Most of the changes here in parseuri could have been in patch 3/4 if we
weren't churning on the qapi definition.  But looks like your conversion
here is correct.

> -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster **pgconf,
> -                                      const char *filename, Error **errp)
> +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
> +                                           Error **errp)
>  {

I might have split the refactoring of qemu_gluster_glfs_init() into its
own patch, but not the end of the world the way it was done here.

>      struct glfs *glfs;
>      int ret;
>      int old_errno;
> -    BlockdevOptionsGluster *gconf;
> -
> -    ret = qemu_gluster_parseuri(&gconf, filename);
> -    if (ret < 0) {
> -        error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
> -                         "volume/path[?socket=...]");
> -        errno = -ret;
> -        goto out;
> -    }
> +    GlusterServerList *server;
>  
>      glfs = glfs_new(gconf->volume);
>      if (!glfs) {
>          goto out;
>      }
>  
> -    ret = glfs_set_volfile_server(glfs,
> -                                  
> GlusterTransport_lookup[gconf->server->transport],
> -                                  gconf->server->host, gconf->server->port);
> -    if (ret < 0) {
> -        goto out;
> +    for (server = gconf->server; server; server = server->next) {

I still wonder if gconf->servers (and therefore servers.0, servers.1 in
the command line, instead of server.0, server.1) would have been a
better name for the list, but I don't know if it is worth repainting the
bikeshed at this point in time.  On the other hand, it's user-visible,
so once it gets released, we're stuck with the name, but up until then,
we can do a followup patch if anyone else has a strong opinion.

> +        ret = glfs_set_volfile_server(glfs,
> +                                      
> GlusterTransport_lookup[server->value->transport],
> +                                      server->value->host, 
> server->value->port);

I asked in v13 if all initializations set the optional transport and
port.  See [3] below

> +        if (ret < 0) {
> +            goto out;
> +        }
>      }
>  
>      /*
> @@ -244,10 +287,9 @@ static struct glfs 
> *qemu_gluster_init(BlockdevOptionsGluster **pgconf,
>      ret = glfs_init(glfs);
>      if (ret) {
>          error_setg_errno(errp, errno,
> -                         "Gluster connection failed for host=%s port=%ld "
> -                         "volume=%s path=%s transport=%s", 
> gconf->server->host,
> -                         gconf->server->port, gconf->volume, gconf->path,
> -                         GlusterTransport_lookup[gconf->server->transport]);
> +                         "Gluster connection failed for given hosts "
> +                         "volume:'%s' path:'%s' host1:'%s'", gconf->volume,

Reads a bit awkwardly: why "host1"?  I might have done (pseudocode):

"Gluster connection for '%s/%s' failed for host '%s'", volume, path, host

but not worth a respin by itself.

>  
> +static int parse_transport_option(const char *opt)
> +{
> +    int i;
> +
> +    if (!opt) {
> +        /* Set tcp as default */
> +        return GLUSTER_TRANSPORT_TCP;
> +    }
> +
> +    for (i = 0; i < GLUSTER_TRANSPORT_MAX; i++) {
> +        if (!strcmp(opt, GlusterTransport_lookup[i])) {
> +            return i;
> +        }
> +    }
> +
> +    return i;
> +}

This function feels like it could be replaced by qapi_enum_parse() with
appropriate arguments.  See [2] below

> +
> +/*
> + * Convert the json formatted command line into qapi.
> +*/
> +static int qemu_gluster_parsejson(BlockdevOptionsGluster **pgconf,
> +                                  QDict *options)
> +{
> +    QemuOpts *opts;
> +    BlockdevOptionsGluster *gconf = NULL;
> +    GlusterServer *gsconf;
> +    GlusterServerList **prev;
> +    GlusterServerList *curr = NULL;
> +    QDict *backing_options = NULL;
> +    Error *local_err = NULL;
> +    char *str = NULL;
> +    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_SERVER_PATTERN);
> +    if (num_servers < 1) {
> +        error_setg(&local_err, "qemu_gluster: please provide 'server' "
> +                               "option with valid fields in array of 
> tuples");

I'm not convinced the 'qemu_gluster: ' prefix on the errors is
necessary, but to date I haven't applied the patch to actually try
triggering an error to see what gets printed.  At any rate, even if the
message could use some polish, it is at least accurate, and shouldn't
hold up getting the initial patch in.

> +        goto out;
> +    }
> +
> +    ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
> +    if (!ptr) {
> +        error_setg(&local_err, "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, "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 */
> +    for (i = 0; i < num_servers; i++) {
> +        opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort);
> +         str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);

Indentation is off.

str is allocated on first iteration, then overwritten in second, for a
memory leak.

> +        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, "qemu_gluster: server.{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 */
> +        gsconf->transport = parse_transport_option(ptr);
> +        if (gsconf->transport == GLUSTER_TRANSPORT_MAX) {
> +            error_setg(&local_err, "qemu_gluster: please set 'transport'"
> +                                   " type in tuple.%d as tcp or rdma", i);

[2] Yep, as this looks to be the only caller of parse_transport_option,
I really wonder if using qapi_enum_parse() here would be better to make
your error message more consistent with other failed string-to-enum
conversions.

> +            g_free(gsconf);

Jeff pointed out the leak; fix it by using
qapi_free_GlusterServer(gsconf) instead of g_free().

> +            goto out;
> +        }
> +        gsconf->has_transport = true;
> +
> +        gsconf->port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT,
> +                                           GLUSTER_DEFAULT_PORT);
> +        gsconf->has_port = true;

[3] Okay, so all initialization paths from the command line do set the
optional parameters.  And blockdev-add does its QMP magic by converting
to a QDict as if it had gone through the command line, so even that
looks like it is safe.  Phew.  It would still be nice if qapi supported
the notion of default values, but that won't land before 2.6, so your
approach is fine in the meantime.

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

Once the leaks are fixed, it looks like you have things working here.

> +
> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster **gconf,
> +                                      const char *filename,
> +                                      QDict *options, Error **errp)
> +{
> +    int ret;
> +
> +    if (filename) {
> +        ret = qemu_gluster_parseuri(gconf, filename);
> +        if (ret < 0) {
> +            error_setg(errp, "Usage: 
> file=gluster[+transport]://[host[:port]]/"
> +                             "volume/path[?socket=...]");

Hmm, just noticing this now, even though this error message is just code
motion.  It looks like the optional [?socket=...] part of a URI is only
important when using gluster+unix (is it silently ignored otherwise?).
And if it is used, you are then assigning it to the host field?

I almost wonder if GlusterServer should be a discriminated union.  That
is, in qapi-pseudocode (won't actually compile yet, because it depends
on features that I have queued for 2.6):

{ 'union':'GlusterServer', 'base':{'transport':'GlusterTransport'},
  'discriminator':'transport', 'data':{
    'tcp':{'host':'str', '*port':'uint16'},
    'unix':{'socket':'str'},
    'rdma':{...} } }

Hmm. Qapi doesn't (yet) allow for an optional discriminator (where the
omission of the discriminator picks a default branch) - another RFE for
my qapi work for 2.6.

Command-line wise, this would mean you could do in JSON:

'servers':[{'transport':'tcp', 'host':'foo'},
           {'transport':'unix', 'socket':'/path/to/bar'},
           {'host':'blah'}]

where the third entry defaults to transport tcp.

If we think that description is better than what we proposed in 3/4,
then it's really late to be adding it now, especially since (without
qapi changes) we'd have a mandatory rather than optional 'transport';
but worse if we commit to the interface of 3/4 and don't get the
conversion made in time to the nicer interface.  At least it's okay from
back-compat perspective to make a mandatory field become optional in
later releases.

If it were just gluster code I was worried about, then I could live with
the interface proposal.  But since seeing this error message is making
me double-guess the interface correctness, and that will have an impact
on libvirt, I'm starting to have doubts on what it means for qemu 2.5.

> +            errno = -ret;
> +            return NULL;
> +        }
> +    } else {
> +        ret = qemu_gluster_parsejson(gconf, options);
> +        if (ret < 0) {
> +            error_setg(errp, "Usage: "

Rather broad error message; might be nice for a later patch to change
parsejson() to take an errp parameter, so it can complain about the
particular problem. But that's internal to gluster, and doesn't affect
the overall interface.


> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index bbefe43..c28cb9f 100644
> --- a/qapi/block-core.json

[1] Interface review

> +++ b/qapi/block-core.json
> @@ -1841,14 +1841,14 @@
>  #
>  # @path:     absolute path to image file in gluster volume
>  #
> -# @servers:  gluster server description
> +# @servers:  one or more gluster server descriptions
>  #
>  # Since: 2.5
>  ##
>  { 'struct': 'BlockdevOptionsGluster',
>    'data': { 'volume': 'str',
>              'path': 'str',
> -            'server': 'GlusterServer' } }
> +            'server': [ 'GlusterServer' ] } }

As mentioned in 3/4, I would have preferred to have just enforced a
one-element array then, and no interface change here. But it's not the
end of the world the way you split it, and it looks like the patches
will go in together or not at all, so we won't see the churn in released
versions.

Back up to the implementation, before returning here.

Overall summary:
I think most of the problems that have been pointed out could be fixed
by a maintainer (Jeff, up to you if you want to require a v15), but I'm
very worried that we still haven't quite nailed the interface of
GlusterServer.  If I knew for sure that we had time to clean up any mess
before 2.5 bakes things in, then you can add:
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]