Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend.

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

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 <string.h>
>     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.