From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36962) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TH7lE-0000pT-2c for qemu-devel@nongnu.org; Thu, 27 Sep 2012 02:39:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TH7l9-0000i3-Nb for qemu-devel@nongnu.org; Thu, 27 Sep 2012 02:39:48 -0400 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:36117) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TH7l9-0000hy-5s for qemu-devel@nongnu.org; Thu, 27 Sep 2012 02:39:43 -0400 Received: from /spool/local by e23smtp07.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 27 Sep 2012 16:37:14 +1000 Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q8R6dXl626476670 for ; Thu, 27 Sep 2012 16:39:33 +1000 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q8R6dWGE021281 for ; Thu, 27 Sep 2012 16:39:33 +1000 Date: Thu, 27 Sep 2012 12:11:32 +0530 From: Bharata B Rao Message-ID: <20120927064132.GB18285@in.ibm.com> References: <20120924091008.GJ18470@in.ibm.com> <20120924091340.GN18470@in.ibm.com> <5062D24F.7030304@redhat.com> <20120926161132.GA32195@in.ibm.com> <50632F6A.5020202@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50632F6A.5020202@redhat.com> Subject: Re: [Qemu-devel] [PATCH v9 4/4] 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: Paolo Bonzini Cc: Kevin Wolf , Anthony Liguori , Anand Avati , Vijay Bellur , Stefan Hajnoczi , Harsh Bora , Amar Tumballi , qemu-devel@nongnu.org, "Richard W.M. Jones" , Blue Swirl , Avi Kivity , Daniel Veillard On Wed, Sep 26, 2012 at 06:38:02PM +0200, Paolo Bonzini wrote: > Il 26/09/2012 18:11, Bharata B Rao ha scritto: > >>> +static int parse_volume_options(GlusterConf *gconf, char *path) > >>> > > +{ > >>> > > + char *token, *saveptr; > >>> > > + > >>> > > + /* volname */ > >>> > > + token = strtok_r(path, "/", &saveptr); > >>> > > + if (!token) { > >>> > > + return -EINVAL; > >>> > > + } > >>> > > + gconf->volname = g_strdup(token); > >>> > > + > >>> > > + /* image */ > >>> > > + token = strtok_r(NULL, "?", &saveptr); > >> > > >> > If I understand uri.c right, there is no ? in the path, so there's no > >> > reason to call strtok. You could just use the rest of the string. > > Actually there could be a %3F which uri.c would unescape to ? (only the > query part is left escaped), so your usage of strtok_r is incorrect. Ok the approch of using 2 strtok as above would fail for URI's like this: gluster+unix://server/volname/weird%3Fimage?socket=/path/to/socket > > > As you note, I don't need 2nd strtok strictly since the rest of the string > > is available in saveptr. But I thought using saveptr is not ideal or preferred. > > I wanted to use the most appropriate/safe delimiter to extract the image string > > in the 2nd strtok and decided to use '?'. > > I don't think it is defined what saveptr points to. > http://publib.boulder.ibm.com/infocenter/pseries/v5r3/index.jsp?topic=/com.ibm.aix.basetechref/doc/basetrf2/strtok_r.htm > says "the strtok_r subroutine also updates the Pointer parameter with > the starting address of the token following the first occurrence of the > Separators parameter". I read this as: > > *saveptr = token + strlen(token) + 1; > > which is consistent with this strtok example from the C standard: > > #include > static char str[] = "?a???b,"; > char *t; > > t = strtok(str, "?"); // t points to the token "a" > t = strtok(str, ","); // t points to the token "??b" > > Have you tested this code with multiple consecutive slashes? Yes, both 2-strtok and strtok+saveptr approach work correctly (= as per my expectation!) with multiple consecutive slashes. I do understand that 2-strtok approach will not work when we have %3F in the path component of the URI. For URIs like gluster://server/volname/path/to/image, both the approaches extract image as "path/to/image". For URIs like gluster://server/volname//path/to/image, both the approaches extract image as "/path/to/image". For gluster://server/volname////path/to/image, the image is extracted as "//path/to/image". > > > If you think using saveptr is fine, then I could use that as below... > > > > /* image */ > > if (!*saveptr) { > > return -EINVAL; > > } > > gconf->image = g_strdup(saveptr); > > > > I would avoid strtok_r completely: > > char *p = path + strcpsn(path, "/"); > if (*p == '\0') { > return -EINVAL; > } > gconf->volname = g_strndup(path, p - path); > > p += strspn(p, "/"); > if (*p == '\0') { > return -EINVAL; > } > gconf->image = g_strdup(p); This isn't working because for a URI like gluster://server/volname/path/to/image, uri_parse() will give "/volname/path/to/image" in uri->path. I would have expected to see uri->path as "volname/path/to/image" (without 1st slash). Note that gluster is currently able to resolve image paths that don't have a preceding slash (like dir/a.img). But I guess we should support specifying complete image paths too (like /dir/a.img) Regards, Bharata.