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