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

[Paolo Bonzini]

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.

> 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?

> 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);

Paolo