From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50641) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoRGY-0001SN-IK for qemu-devel@nongnu.org; Tue, 20 Oct 2015 03:23:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZoRGV-0007gt-CA for qemu-devel@nongnu.org; Tue, 20 Oct 2015 03:23:26 -0400 Received: from mx5-phx2.redhat.com ([209.132.183.37]:36569) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoRGV-0007gc-5H for qemu-devel@nongnu.org; Tue, 20 Oct 2015 03:23:23 -0400 Date: Tue, 20 Oct 2015 03:23:18 -0400 (EDT) From: Prasanna Kumar Kalever Message-ID: <2022500068.34470008.1445325798813.JavaMail.zimbra@redhat.com> In-Reply-To: <56254DC5.70009@redhat.com> References: <1445256783-28558-1-git-send-email-prasanna.kalever@redhat.com> <56254DC5.70009@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] block/gluster: add support for multiple gluster servers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, pkrempa@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, deepakcs@redhat.com, bharata@linux.vnet.ibm.com, rtalur@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