All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eric Blake <eblake@redhat.com>
To: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>,
	qemu-devel@nongnu.org
Cc: kwolf@redhat.com, pkrempa@redhat.com, stefanha@gmail.com,
	jcody@redhat.com, deepakcs@redhat.com,
	bharata@linux.vnet.ibm.com, rtalur@redhat.com
Subject: Re: [Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers
Date: Tue, 10 Nov 2015 09:07:20 -0700	[thread overview]
Message-ID: <56421638.2050305@redhat.com> (raw)
In-Reply-To: <1447146556-7328-4-git-send-email-prasanna.kalever@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 20754 bytes --]

On 11/10/2015 02:09 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.
> 

[...]

> 2.
>  'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
>          "path":"/path/a.qcow2","servers":
>          [{"host":"1.2.3.4","port":"24007","transport":"tcp"},
>           {"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }'
> 
> 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 <kwolf@redhat.com> and
> "Deepak C Shetty" <deepakcs@redhat.com> for inputs and all their support
> 
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> ---

> v10:
> fix mem-leak as per Peter Krempa <pkrempa@redhat.com> review comments
> 
> v11:
> using qapi-types* defined structures as per "Eric Blake" <eblake@redhat.com>
> review comments.
> 
> v12:
> fix crash caused in qapi_free_BlockdevOptionsGluster
> 
> v13:
> address comments from "Jeff Cody" <jcody@redhat.com>

I had some other comments against v10 that I don't see addressed yet:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06377.html

> ---
>  block/gluster.c      | 468 ++++++++++++++++++++++++++++++++++++++++++++-------
>  qapi/block-core.json |  60 ++++++-
>  2 files changed, 461 insertions(+), 67 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index ededda2..8939072 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -11,6 +11,19 @@
>  #include "block/block_int.h"
>  #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_SERVERS_PATTERN "servers."
> +
> +#define GLUSTER_DEFAULT_PORT        24007
> +
> +#define MAX_SERVERS                 "10000"

Why is this a string rather than an integer?

> +
> +
>  typedef struct GlusterAIOCB {
>      int64_t size;
>      int ret;
> @@ -29,15 +42,6 @@ typedef struct BDRVGlusterReopenState {
>      struct glfs_fd *fd;
>  } BDRVGlusterReopenState;
>  
> -typedef struct GlusterConf {
> -    char *host;
> -    int port;
> -    char *volume;
> -    char *path;
> -    char *transport;
> -} GlusterConf;
> -

This patch feels pretty big. It may be smarter to break it into two
pieces - one that adds GlusterConf to qapi/block-core.json and replaces
existing uses of this definition to the qapi type but with no changes in
semantics; and the other that then extends things to add support for
multiple servers (so that we aren't trying to do too much in one patch).

> @@ -143,8 +176,11 @@ static int parse_volume_options(GlusterConf *gconf, char *path)
>   * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
>   * file=gluster+rdma://1.2.3.4:24007/testvol/a.img
>   */
> -static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> +static int qemu_gluster_parseuri(BlockdevOptionsGluster **pgconf,
> +                                 const char *filename)
>  {
> +    BlockdevOptionsGluster *gconf;
> +    GlusterServer *gsconf;
>      URI *uri;
>      QueryParams *qp = NULL;
>      bool is_unix = false;
> @@ -155,20 +191,24 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
>          return -EINVAL;
>      }
>  
> +    gconf = g_new0(BlockdevOptionsGluster, 1);
> +    gsconf = g_new0(GlusterServer, 1);

gconf and gsconf are both allocated here...

> +
>      /* transport */
>      if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> -        gconf->transport = g_strdup("tcp");
> +        gsconf->transport = GLUSTER_TRANSPORT_TCP;
>      } else if (!strcmp(uri->scheme, "gluster+tcp")) {
> -        gconf->transport = g_strdup("tcp");
> +        gsconf->transport = GLUSTER_TRANSPORT_TCP;
>      } else if (!strcmp(uri->scheme, "gluster+unix")) {
> -        gconf->transport = g_strdup("unix");
> +        gsconf->transport = GLUSTER_TRANSPORT_UNIX;
>          is_unix = true;
>      } else if (!strcmp(uri->scheme, "gluster+rdma")) {
> -        gconf->transport = g_strdup("rdma");
> +        gsconf->transport = GLUSTER_TRANSPORT_RDMA;
>      } else {
>          ret = -EINVAL;
>          goto out;

...but you can error here...

>      }
> +    gsconf->has_transport = true;
>  
>      ret = parse_volume_options(gconf, uri->path);
>      if (ret < 0) {
> @@ -190,13 +230,27 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
>              ret = -EINVAL;
>              goto out;
>          }
> -        gconf->host = g_strdup(qp->p[0].value);
> +        gsconf->host = g_strdup(qp->p[0].value);
>      } else {
> -        gconf->host = g_strdup(uri->server ? uri->server : "localhost");
> -        gconf->port = uri->port;
> +        gsconf->host = g_strdup(uri->server ? uri->server : "localhost");
> +        if (uri->port) {
> +            gsconf->port = uri->port;
> +        } else {
> +            gsconf->port = GLUSTER_DEFAULT_PORT;
> +        }
> +        gsconf->has_port = true;
>      }
>  
> +    gconf->servers = g_new0(GlusterServerList, 1);
> +    gconf->servers->value = gsconf;
> +    gconf->servers->next = NULL;

Dead assignment (gconf->servers->next is already NULL because of g_new0).

> +
> +    *pgconf = gconf;
> +
>  out:
> +    if (ret < 0) {
> +        qapi_free_BlockdevOptionsGluster(gconf);
> +    }
>      if (qp) {
>          query_params_free(qp);
>      }
> @@ -204,30 +258,26 @@ out:
>      return ret;
>  }

...which means you leak gsconf if you hit an error.

>  
> -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
> -                                      Error **errp)
> +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
> +                                           Error **errp)
>  {
> -    struct glfs *glfs = NULL;
> +    struct glfs *glfs;
>      int ret;
>      int old_errno;
> -
> -    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, gconf->transport, gconf->host,
> -            gconf->port);
> -    if (ret < 0) {
> -        goto out;
> +    for (server = gconf->servers; server != NULL; server = server->next) {

It's okay to use 'server;' rather than 'server != NULL;' as the loop
condition.  Matter of personal style.

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

port and transport are optional; which means you should probably be
checking has_port and has_transport before blindly using them (unless
you made sure that ALL initialization paths set things to sane defaults
when the user omitted the arguments).

> +        if (ret < 0) {
> +            goto out;
> +        }
>      }
>  
>      /*
> @@ -242,10 +292,9 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
>      ret = glfs_init(glfs);
>      if (ret) {
>          error_setg_errno(errp, errno,
> -                         "Gluster connection failed for host=%s port=%d "
> -                         "volume=%s path=%s transport=%s", gconf->host,
> -                         gconf->port, gconf->volume, gconf->path,
> -                         gconf->transport);
> +                         "Error: Gluster connection failed for given hosts "

Don't start messages with "Error: ", error_setg_errno() already does
that for you.

> +                         "volume:'%s' path:'%s' host1: %s", gconf->volume,

Inconsistent on whether there is a space after ':'.  "given hosts
volume" sounds odd.

> +                         gconf->path, gconf->servers->value->host);
>  
>          /* glfs_init sometimes doesn't set errno although docs suggest that */
>          if (errno == 0)
> @@ -264,6 +313,300 @@ out:
>      return NULL;
>  }
>  
> +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 -EINVAL;
> +}
> +
> +/*
> +*
> +*  Basic command line syntax looks like:
> +*

You have 110 lines from /* to */.  In v10, I already mentioned that this
comment is probably 100 lines too long.  You do NOT need to repeat the
syntax, examples, or even more-readable example here; having them in the
commit body was enough.  Someone that knows how to read qapi will be
able to deduce what this function is doing if you were to simplify to
just this:

/*
 * Convert the 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;
> +    size_t buff_size;
> +    int i;
> +
> +
> +    /* create opts info from runtime_json_opts list */

Why two blank lines?

> +    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' "

Again, error messages created with error_setg() need not start with
"Error: ".

A good thing to try when you are adding an error message for a command
line parsing scenario is to try and come up with the command line that
would trigger the error to see if the result looks sane.

> +                               "option with valid fields in array of tuples");
> +        goto out;
> +    }
> +
> +    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' "

More "Error: " prefixes.  I'll quit pointing them out.

> +                               "option");
> +        goto out;
> +    }
> +    gconf->path = g_strdup(ptr);
> +
> +    qemu_opts_del(opts);
> +
> +    /* create opts info from runtime_tuple_opts list */
> +    buff_size = strlen(GLUSTER_OPT_SERVERS_PATTERN) + strlen(MAX_SERVERS) + 2;
> +    str = g_malloc(buff_size);
> +    for (i = 0; i < num_servers; i++) {
> +        opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort);
> +        g_assert(snprintf(str, buff_size,
> +                          GLUSTER_OPT_SERVERS_PATTERN"%d.", i) < buff_size);

Gross - you have side effects inside g_assert().  (Absolute bug if you
do that inside plain assert(); possibly excusable in g_assert() but
still not good practice).

If I were writing this, then instead of futzing around with snprintf,
I'd just use g_strdup_printf() to malloc the appropriately sized string
without worrying about sizing myself, and be sure I didn't leak things.

> +        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 is allocated here...

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

...but if you error here...

> +        }
> +        /* only if valid transport i.e. either of tcp|rdma is specified */
> +        gsconf->transport = parse_transport_option(ptr);

Why are you calling parse_transport_option() twice?

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

Indentation is off.

> +        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 = 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;
> +}

...then gsconf is leaked.

> +
> +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=...]");
> +            errno = -ret;
> +            return NULL;
> +        }
> +    } else {
> +        ret = qemu_gluster_parsejson(gconf, options);
> +        if (ret < 0) {
> +            error_setg(errp, "Wrong Usage.");

Don't end error_setg() messages with a period.  And Talking In All Camel
Case Is Odd.

> +            error_append_hint(errp,
> +                             "Usage1: "
> +                             "-drive driver=qcow2,file.driver=gluster,"
> +                             "file.volume=testvol,file.path=/path/a.qcow2,"
> +                             "file.servers.0.host=1.2.3.4,"
> +                             "[file.servers.0.port=24007,]"
> +                             "[file.servers.0.transport=tcp,]"
> +                             "file.servers.1.host=5.6.7.8,"
> +                             "[file.servers.1.port=24008,]"
> +                             "[file.servers.1.transport=rdma,] ...");
> +            error_append_hint(errp,
> +                             "\nUsage2: "
> +                             "'json:{\"driver\":\"qcow2\",\"file\":"
> +                             "{\"driver\":\"gluster\",\"volume\":\""
> +                             "testvol\",\"path\":\"/path/a.qcow2\","
> +                             "\"servers\":[{\"host\":\"1.2.3.4\","
> +                             "\"port\":\"24007\",\"transport\":\"tcp\"},"
> +                             "{\"host\":\"4.5.6.7\",\"port\":\"24007\","
> +                             "\"transport\":\"rdma\"}, ...]}}'");

Rather long. I think a single hint is long enough; you don't need to
display the json:{} usage.


> @@ -523,7 +863,7 @@ static int qemu_gluster_create(const char *filename,
>      } else if (!strcmp(tmp, "full") && gluster_supports_zerofill()) {
>          prealloc = 1;
>      } else {
> -        error_setg(errp, "Invalid preallocation mode: '%s'"
> +        error_setg(errp, "Error: Invalid preallocation mode: '%s'"
>                           " or GlusterFS doesn't support zerofill API", tmp);

Spurious hunk.


> +++ b/qapi/block-core.json
> @@ -1375,13 +1375,14 @@
>  # Drivers that are supported in block device operations.
>  #
>  # @host_device, @host_cdrom: Since 2.1
> +# @gluster: Since 2.5

Sadly, I think we've found enough issues that this will not make 2.5
hard freeze deadline, so at this point, you should change things to
state 2.6.

> +
> +##
> +# @GlusterServer
> +#
> +# Gluster tuple set

Not very descriptive.  Better might be 'Details for connecting to a
gluster server'.

> +#
> +# @host:       host address (hostname/ipv4/ipv6 addresses)
> +#
> +# @port:       #optional port number on which glusterd is listening
> +#               (default 24007)
> +#
> +# @transport:  #optional transport type used to connect to gluster management
> +#               daemon (default 'tcp')
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'GlusterServer',
> +  'data': { 'host': 'str',
> +            '*port': 'int',
> +            '*transport': 'GlusterTransport' } }

I really do think it might be easier to split this into two patches; one
that introduces GlusterServer in the .json and converts existing uses to
it, and the other that introduces BlockdevOptionsGluster along with its
ability to use a GlusterServerList.

> +
> +##
> +# @BlockdevOptionsGluster
> +#
> +# Driver specific block device options for Gluster
> +#
> +# @volume:   name of gluster volume where VM image resides
> +#
> +# @path:     absolute path to image file in gluster volume
> +#
> +# @servers:  one or more gluster server descriptions (host, port, and transport)

80 columns; might be nice to keep things at 79 or less.  In fact, since
GlusterServer is already documented, you could get away with:

# @servers: one or more gluster server descriptions

> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevOptionsGluster',
> +  'data': { 'volume': 'str',
> +            'path': 'str',
> +            'servers': [ 'GlusterServer' ] } }
> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.
> @@ -1816,7 +1870,7 @@
>        'file':       'BlockdevOptionsFile',
>        'ftp':        'BlockdevOptionsFile',
>        'ftps':       'BlockdevOptionsFile',
> -# TODO gluster: Wait for structured options
> +      'gluster':    'BlockdevOptionsGluster',
>        'host_cdrom': 'BlockdevOptionsFile',
>        'host_device':'BlockdevOptionsFile',
>        'http':       'BlockdevOptionsFile',
> 

Overall, I think we are probably on the right track for the QMP
interface; but since blockdev-add is NOT stable yet for 2.5, it won't
hurt to wait to get this in until 2.6, to make sure we have plenty of
time; and it would also be nice to make sure we get nbd, nfs, rbd,
sheepdog all supported in the same release; possibly by sharing common
types instead of introducing GlusterServer as a one-off type.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

  reply	other threads:[~2015-11-10 16:07 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-11-10  9:09 [Qemu-devel] [PATCH 0/3] block/gluster: add support for multiple gluster servers Prasanna Kumar Kalever
2015-11-10  9:09 ` [Qemu-devel] [PATCH v2 1/3] block/gluster: rename [server, volname, image] -> [host, volume, path] Prasanna Kumar Kalever
2015-11-10 15:28   ` Eric Blake
2015-11-10  9:09 ` [Qemu-devel] [PATCH v2 2/3] block/gluster: code cleanup Prasanna Kumar Kalever
2015-11-10  9:09 ` [Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers Prasanna Kumar Kalever
2015-11-10 16:07   ` Eric Blake [this message]
2015-11-10 17:19     ` Jeff Cody
2015-11-12 10:04     ` Prasanna Kumar Kalever
2015-11-12 16:23       ` Jeff Cody
2015-11-13  4:54         ` Jeff Cody
2015-11-10 17:24   ` Jeff Cody
2015-11-12  9:46     ` Prasanna Kumar Kalever
2015-11-10 15:26 ` [Qemu-devel] [PATCH 0/3] " Eric Blake

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=56421638.2050305@redhat.com \
    --to=eblake@redhat.com \
    --cc=bharata@linux.vnet.ibm.com \
    --cc=deepakcs@redhat.com \
    --cc=jcody@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=pkrempa@redhat.com \
    --cc=prasanna.kalever@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=rtalur@redhat.com \
    --cc=stefanha@gmail.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.