On 10/27/2015 07:06 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.
> 

No 0/3 cover letter? Even if only one patch of a series changes, it's
still nice to have a cover letter explaining where to find the rest of
the series.

> Problem:
> 
> Currenly VM Image on gluster volume is specified like this:

s/Currenly/Currently/

[...]

> 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)
> 
> This patch depends on a recent fix in libgfapi raised as part of this work:
> http://review.gluster.org/#/c/12114/
> 
> 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>
> v1:

Need a '---' line before v1.  Everything above the line is okay for the
commit message, everything after the line is informational for reviewers
but doesn't belong in qemu.git (and 'git am' will automatically strip
comments after the --- separator).  A year from now, we won't care how
many versions it took you to get to the version that was committed.

> +++ b/block/gluster.c

> -typedef struct GlusterConf {
> +typedef struct GlusterServerConf {
>      char *host;
>      int port;
> +    char *transport;
> +} GlusterServerConf;

Why are you defining this type, instead of reusing 'GlusterTuple' that
gets auto-defined for you by your changes to block-core.json?

> 
> +typedef struct GlusterConf {
>      char *volume;
>      char *path;
> -    char *transport;
> +    int num_servers;

size_t is better than int when counting items in an array.

> +    GlusterServerConf *gsconf;

Naming it 'num_servers' vs. 'gsconf' is confusing.  In fact, why are you
even defining this type?  Why not just directly use
'BlockdevOptionsGluster' that you defined by your changes to
block-core.json?  Of course, the generated type uses a single
GlusterTupleList (a linked list with NULL terminator) rather than a pair
of size and GlusterTuple[] for tracking the multiple servers, but
directly using the qapi types now will save you some conversion efforts
down the road.

> +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 listen (default 24007)",

s/listen/is listening/

> +        },
> +        {
> +            .name = GLUSTER_OPT_TRANSPORT,
> +            .type = QEMU_OPT_STRING,
> +            .help = "transport type 'tcp' or 'rdma' (default 'tcp')",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  
>  static void qemu_gluster_gconf_free(GlusterConf *gconf)
>  {

Unneeded.  Just use the auto-generated
qapi_BlockdevOptionsGluster_free() that you got by adding and using the
appropriate types in block-core.json.

> @@ -155,16 +218,20 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
>          return -EINVAL;
>      }
>  
> +    gconf = g_new0(GlusterConf, 1);
> +    gconf->num_servers = 1;
> +    gconf->gsconf = g_new0(GlusterServerConf, gconf->num_servers);

If you use the qapi types, as I've suggested above, then you will need
to be working with the list of servers as a linked list rather than an
array.

> +
>      /* transport */
>      if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> -        gconf->transport = g_strdup("tcp");
> +        gconf->gsconf[0].transport = g_strdup("tcp");
>      } else if (!strcmp(uri->scheme, "gluster+tcp")) {
> -        gconf->transport = g_strdup("tcp");
> +        gconf->gsconf[0].transport = g_strdup("tcp");
>      } else if (!strcmp(uri->scheme, "gluster+unix")) {
> -        gconf->transport = g_strdup("unix");
> +        gconf->gsconf[0].transport = g_strdup("unix");
>          is_unix = true;
>      } else if (!strcmp(uri->scheme, "gluster+rdma")) {
> -        gconf->transport = g_strdup("rdma");
> +        gconf->gsconf[0].transport = g_strdup("rdma");

The generated qapi code also contains convenience functions from mapping
from a finite set of strings to a set of enum values, although I don't
see "unix" listed in your set of values for 'GlusterTransport'.


> +/*
> +*
> +*  Basic command line syntax looks like:
> +*
> +* Pattern I:

> +* Pattern II:

> +*
> +* Examples:
> +* Pattern I:

> +*
> +* Pattern II:
> +* 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",

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

Wow. Overly long comment. It was fine in the commit message, but I don't
think you need quite so much filler here.


> +    } else {
> +        ret = qemu_gluster_parsejson(gconf, options);
> +        if (ret < 0) {
> +            error_setg(errp, "Wrong Usage!");

Drop the '!'.  None of our other error messages need that much emphasis.


> +++ b/qapi/block-core.json
> @@ -1375,15 +1375,16 @@
>  #
>  # @host_device, @host_cdrom, @host_floppy: Since 2.1
>  # @host_floppy: deprecated since 2.3
> +# @gluster: Since 2.5
>  #
>  # Since: 2.0
>  ##
>  { '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' ] }
>  
>  ##
>  # @BlockdevOptionsBase
> @@ -1794,6 +1795,57 @@
>              '*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' ] }

As mentioned above, isn't this missing 'unix'?

> +
> +##
> +# @GlusterTuple
> +#
> +# Gluster tuple set
> +#
> +# @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': 'GlusterTuple',
> +  'data': { 'host': 'str',
> +            '*port': 'int',
> +            '*transport': 'GlusterTransport' } }

I'm still not sold that 'GlusterTuple' is the best name for this type,
but it doesn't affect ABI and I don't have any better suggestions off-hand.

> +
> +##
> +# @BlockdevOptionsGluster
> +#
> +# Driver specific block device options for Gluster
> +#
> +# @volume:   name of gluster volume where our VM image resides

s/our //

> +#
> +# @path:     absolute path to image file in gluster volume
> +#
> +# @servers:  one or more gluster server descriptions (host, port, and transport)
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevOptionsGluster',
> +  'data': { 'volume': 'str',
> +            'path': 'str',
> +            'servers': [ 'GlusterTuple' ] } }

This one looks okay, as far as I can tell.

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