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

[Prasanna Kumar Kalever <pkalever@redhat.com>]

On  Tuesday, October 20, 2015 1:38:37 AM, Eric Blake wrote: 
> 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
> 

Thanks for the tip Eric :)

[...]
> > +#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.
> 
Its based on users choice I don't want to make it static

> > +} GlusterServerConf;

[...]

> > @@ -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).
> 

[1] I think you need to check 'qemu_gluster_parsejson' function for multiple gluster servers
usage which parse JSON syntax with multiple tuples, 'qemu_gluster_parseuri' function is to
parse URI syntax only and that supports single server usage only (kept for compatibility)

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

[...]

> > +#
> > +# 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)
> 
... [1] should clarify your understanding, but yes still I will bind to your comment 

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

-Prasanna