From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53063) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEaQ9-000812-OC for qemu-devel@nongnu.org; Thu, 20 Sep 2012 02:39:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEaQ7-0003LI-JF for qemu-devel@nongnu.org; Thu, 20 Sep 2012 02:39:33 -0400 Received: from e28smtp05.in.ibm.com ([122.248.162.5]:39485) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEaQ7-0003Ky-0D for qemu-devel@nongnu.org; Thu, 20 Sep 2012 02:39:31 -0400 Received: from /spool/local by e28smtp05.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 20 Sep 2012 12:09:27 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q8K6dN1i39059646 for ; Thu, 20 Sep 2012 12:09:23 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q8KC8xWP003476 for ; Thu, 20 Sep 2012 17:39:00 +0530 Date: Thu, 20 Sep 2012 12:11:14 +0530 From: Bharata B Rao Message-ID: <20120920064114.GD5873@in.ibm.com> References: <20120917152149.GB6879@in.ibm.com> <20120917152620.GG6879@in.ibm.com> <50587ED6.9040504@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50587ED6.9040504@redhat.com> Subject: Re: [Qemu-devel] [PATCH v7 5/5] block: Support GlusterFS as a QEMU block backend. Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Anthony Liguori , Anand Avati , Vijay Bellur , Stefan Hajnoczi , Amar Tumballi , qemu-devel@nongnu.org, Markus Armbruster , Blue Swirl , Avi Kivity , Paolo Bonzini On Tue, Sep 18, 2012 at 04:01:58PM +0200, Kevin Wolf wrote: > > + > > +#define GLUSTER_TRANSPORT_DEFAULT "gluster://" > > +#define GLUSTER_TRANSPORT_DEFAULT_SZ strlen(GLUSTER_TRANSPORT_DEFAULT) > > +#define GLUSTER_TRANSPORT_TCP "gluster+tcp://" > > +#define GLUSTER_TRANSPORT_TCP_SZ strlen(GLUSTER_TRANSPORT_TCP) > > +#define GLUSTER_TRANSPORT_UNIX "gluster+unix://" > > +#define GLUSTER_TRANSPORT_UNIX_SZ strlen(GLUSTER_TRANSPORT_UNIX) > > +#define GLUSTER_TRANSPORT_RDMA "gluster+rdma://" > > +#define GLUSTER_TRANSPORT_RDMA_SZ strlen(GLUSTER_TRANSPORT_RDMA) > > + > > + > > +static int parse_gluster_spec(GlusterURI *uri, char *spec) > > +{ > > + char *token, *saveptr; > > + int ret; > > + QemuOpts *opts; > > + char *p, *q; > > + > > + /* transport */ > > + p = spec; > > + if (!strncmp(p, GLUSTER_TRANSPORT_DEFAULT, GLUSTER_TRANSPORT_DEFAULT_SZ)) { > > + uri->transport = g_strdup("tcp"); > > + p += GLUSTER_TRANSPORT_DEFAULT_SZ; > > + } else if (!strncmp(p, GLUSTER_TRANSPORT_TCP, GLUSTER_TRANSPORT_TCP_SZ)) { > > + uri->transport = g_strdup("tcp"); > > + p += GLUSTER_TRANSPORT_TCP_SZ; > > + } else if (!strncmp(p, GLUSTER_TRANSPORT_UNIX, GLUSTER_TRANSPORT_UNIX_SZ)) { > > + uri->transport = g_strdup("unix"); > > + p += GLUSTER_TRANSPORT_UNIX_SZ; > > + } else if (!strncmp(p, GLUSTER_TRANSPORT_RDMA, GLUSTER_TRANSPORT_RDMA_SZ)) { > > Would look a bit nicer with strstart() form cutils.c instead of strncmp(). strstart() works with const char pointers, but I have char pointers here which I need to modify. > > +static int qemu_gluster_parseuri(GlusterURI *uri, const char *filename) > > +{ > > + char *token, *saveptr; > > + char *p, *q, *gluster_spec = NULL; > > + int ret = -EINVAL; > > + > > + p = q = g_strdup(filename); > > Neither p nor q are changed, so one variable would be enough. Right, will fix. > > > + > > + /* Extract server, volname and image */ > > + token = strtok_r(p, "?", &saveptr); > > + if (!token) { > > + goto out; > > + } > > + gluster_spec = g_strdup(token); > > + > > + /* socket */ > > + token = strtok_r(NULL, "?", &saveptr); > > + ret = parse_socket(uri, token); > > + if (ret < 0) { > > + goto out; > > + } > > The is_unix thing feels a bit backwards. You set it whenever an option > is present, and fail if a protocol other than gluster+unix:// is used. > > Wouldn't it make more sense to set it depending on the protocol and then > check in the option parser if is_unix == true when there is a 'socket' > option? Otherwise adding non-unix options later is going to become hard. I see your point, will change this. > > The rest looks good now. Thanks for the review. Regards, Bharata.