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

[Eric Blake]

On 10/19/2015 06:13 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.

When sending a multi-patch series, it is best to also include a 0/3
cover letter.  Git can be configured to do this automatically with:
git config format.coverLetter auto

> 
> Problem:
> 
> Currenly VM Image on gluster volume is specified like this:
> 
> file=gluster[+tcp]://host[:port]/testvol/a.img
> 
> Assuming we have three hosts in trustred pool with replica 3 volume

s/trustred/trusted/ ?

> 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.
> 
> ---
> This series of patches are sent based on "Peter Krempa" <address@hidden>
> review comments on v8 sent earlier.

Even though that was a single patch and you now have a series, it is
still probably worth including v9 in the subject line (git send-email
-v9) to make it obvious how to find the earlier review.


> +++ b/block/gluster.c
> @@ -11,6 +11,16 @@
>  #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_SERVER_PATTERN "servers."

Should the macro name be GLUSTER_OPT_SERVERS_PATTERN?

> +
> +#define GLUSTER_DEFAULT_PORT       24007
> +
>  typedef struct GlusterAIOCB {
>      int64_t size;
>      int ret;
> @@ -24,22 +34,72 @@ typedef struct BDRVGlusterState {
>      struct glfs_fd *fd;
>  } BDRVGlusterState;
>  
> -typedef struct GlusterConf {
> +typedef struct GlusterServerConf {
>      char *server;
>      int port;
> +    char *transport;

How do you know how many transport tuples are present? I'd expect a size
argument somewhere.

> +} GlusterServerConf;

It looks like 2/3 fixes confusing naming in preparation for this patch,
which means your series should probably be reordered to do the trivial
code cleanups and renames first, and then the new feature last.

> +
> +typedef struct GlusterConf {
>      char *volname;
>      char *image;
> -    char *transport;
> +    GlusterServerConf *gsconf;
>  } GlusterConf;
>  
> +static QemuOptsList runtime_json_opts = {
> +    .name = "gluster_json",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
> +    .desc = {
> +        {
> +            .name = GLUSTER_OPT_VOLUME,
> +            .type = QEMU_OPT_STRING,
> +            .help = "name of gluster volume where our VM image resides",

s/our //

> +        },
> +        {
> +            .name = GLUSTER_OPT_PATH,
> +            .type = QEMU_OPT_STRING,
> +            .help = "absolute path to image file in gluster volume",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +static QemuOptsList runtime_tuple_opts = {
> +    .name = "gluster_tuple",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_tuple_opts.head),
> +    .desc = {
> +        {
> +            .name = GLUSTER_OPT_HOST,
> +            .type = QEMU_OPT_STRING,
> +            .help = "host address (hostname/ipv4/ipv6 addresses)",
> +        },
> +        {
> +            .name = GLUSTER_OPT_PORT,
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "port number on which glusterd is listening(default 
> 24007)",

Space before ( in English.

> +        },
> +        {
> +            .name = GLUSTER_OPT_TRANSPORT,
> +            .type = QEMU_OPT_STRING,
> +            .help = "transport type used to connect to glusterd(default 
> tcp)",

And again.  This should not be an open-coded string, but a finite set of
enum values, so you may want to list all the valid possibilities rather
than just the default of tcp.

/me reads ahead

Aha, the only other valid value is rdma.

> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  static void qemu_gluster_gconf_free(GlusterConf *gconf)
>  {
>      if (gconf) {
> -        g_free(gconf->server);
>          g_free(gconf->volname);
>          g_free(gconf->image);
> -        g_free(gconf->transport);
> +        if (gconf->gsconf) {
> +            g_free(gconf->gsconf[0].server);
> +            g_free(gconf->gsconf[0].transport);
> +            g_free(gconf->gsconf);
> +            gconf->gsconf = NULL;

Dead assignment, given that...

> +        }
>          g_free(gconf);

...you are just about to free gconf itself.

> +        gconf = NULL;

Dead assignment, given that...

>      }
>  }

...gconf is about to go out of scope.


> @@ -117,16 +178,19 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
> const char *filename)
> return -EINVAL;
> }
> 
> + gconf = g_new0(GlusterConf, 1);
> + gconf->gsconf = g_new0(GlusterServerConf, 1);

Wow - you are hard-coding things to exactly one server.  The subject
line of the patch claims multiple gluster servers, but I don't see
anything that supports more than one.  So something needs to be fixed up
(if this patch is just renaming things, and a later patch adds support
for more than one, that's okay - but it needs to be described that way).

> +static int qemu_gluster_parsejson(GlusterConf **pgconf, QDict *options)
> +{

> +    /* create opts info from runtime_tuple_opts list */
> +    str = g_malloc(40);

That looks fishy.  For something that small, stack-allocation rather
than malloc may suffice.  Or...

> +    for (i = 0; i < num_servers; i++) {
> +        opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort);
> +        snprintf(str, 40, GLUSTER_OPT_SERVER_PATTERN"%d.", i);

If you stick with snprintf(), it would probably be worth at least an
assert() that you didn't overflow.

...just use g_string_printf() in the first place, and then you don't
have to worry about checking whether your malloc was big enough.  But
then you'd have to be careful to not leak it on iterations.

> +static struct glfs *qemu_gluster_init(GlusterConf **gconf, const char 
> *filename,
> +                                      QDict *options, Error **errp)
> +{
> +    int ret;
> +    int num_servers = 1;
> +
> +    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 {
> +        num_servers = qemu_gluster_parsejson(gconf, options);
> +        if (num_servers < 1) {
> +            error_setg(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,] ..."
> +                             "\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\"}, ...]}}'");

That's a bit long for an error message; it might be better to split
things into a short error, plus optional informative messages using
error_append_hint().


> +++ b/qapi/block-core.json
> @@ -1380,10 +1380,10 @@
>  ##
>  { 'enum': 'BlockdevDriver',
>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> -            'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> -            'host_floppy', 'http', 'https', 'null-aio', 'null-co', 
> 'parallels',
> -            'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
> -            'vmdk', 'vpc', 'vvfat' ] }
> +            'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> +            'host_device', 'host_floppy', 'http', 'https', 'null-aio',
> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> +            'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }

Missing documentation that 'gluster' was supported since 2.5.

>  
>  ##
>  # @BlockdevOptionsBase
> @@ -1794,6 +1794,56 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @GlusterTransport
> +#
> +# An enumeration of Gluster transport type
> +#
> +# @tcp:   TCP  - Transmission Control Protocol
> +#
> +# @rdma:  RDMA - Remote direct memory access
> +#
> +# Since: 2.5
> +##
> +{ 'enum': 'GlusterTransport', 'data': [ 'tcp', 'rdma' ] }
> +
> +##
> +# @GlusterTuple
> +#
> +# Gluster tuple set
> +#
> +# @host:       host address (hostname/ipv4/ipv6 addresses)
> +#
> +# @port:       port number on which glusterd is listening. (default 24007)

Missing an #optional tag.

> +#
> +# @transport:  #transport type used to connect to gluster management daemon

s/#transport/#optional/

> +#               it can be tcp|rdma (default 'tcp')

Mentioning tcp|rdma is redundant with the fact that you typed this
parameter as GlusterTransport (looking at the enum already tells you the
valid values).

> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'GlusterTuple',
> +  'data': { 'host': 'str',
> +            '*port': 'int',
> +            '*transport': 'GlusterTransport' } }
> +
> +##
> +# @BlockdevOptionsGluster
> +#
> +# Driver specific block device options for Gluster
> +#
> +# @volume:   name of gluster volume where our VM image resides
> +#
> +# @path:     absolute path to image file in gluster volume
> +#
> +# @servers:  holds multiple tuples of {host, transport, port}

For this patch, it looks like it holds exactly one tuple.  But it looks
like you plan to support multiple tuples later on; maybe a better
wording is:

@servers: one or more gluster host descriptions (host, port, and transport)

> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevOptionsGluster',
> +  'data': { 'volume': 'str',
> +            'path': 'str',
> +            'servers': [ 'GlusterTuple' ] } }

I'm not necessarily sold that GlusterTuple is the best name (maybe
GlusterServer is better?)  But at least the naming is not part of the
ABI, so I'm not too fussed.  Overall, the API looks like it is starting
to converge to something usable.

> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.
> @@ -1813,7 +1863,7 @@
>        'file':       'BlockdevOptionsFile',
>        'ftp':        'BlockdevOptionsFile',
>        'ftps':       'BlockdevOptionsFile',
> -# TODO gluster: Wait for structured options
> +      'gluster':    'BlockdevOptionsGluster',
>        'host_cdrom': 'BlockdevOptionsFile',
>        'host_device':'BlockdevOptionsFile',
>        'host_floppy':'BlockdevOptionsFile',
> 

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

signature.asc
Description: OpenPGP digital signature