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

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

On Thu, Sep 27, 2012 at 09:19:34AM +0200, Paolo Bonzini wrote:
> > 
> > For gluster://server/volname////path/to/image, the image is extracted as
> > "//path/to/image".
> 
> Should there be three /'s here?  I assume it's just a typo.

Yes it was a typo.

> 
> I'm concerned that there is no documentation of what saveptr actually
> points to.  In many cases the strtok specification doesn't leave much
> free room, but in the case you're testing here:
> 
> >>>     /* image */
> >>>     if (!*saveptr) {
> >>>         return -EINVAL;
> >>>     }
> 
> strtok_r may just as well leave saveptr = NULL for example.

Haven't seen that, but yes can't depend on that I suppose.

> 
> >>>     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).
> 
> Ok, that's easy enough to fix with an extra strspn,
> 
>     char *p = path + strpsn(path, "/");
>     p += strcspn(p, "/");

Ok, this is how its looking finally...

    char *p, *q;
    p = q = path + strspn(path, "/");
    p += strcspn(p, "/");
    if (*p == '\0') {
        return -EINVAL;
    }
    gconf->volname = g_strndup(q, p - q);

    p += strspn(p, "/");
    if (*p == '\0') {
        return -EINVAL;
    }
    gconf->image = g_strdup(p);

> 
> 
> > 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)
> 
> How would the URIs look like?

gluster://server/testvol//dir/a.img ?
Isn't this a valid URI ? Here volname=testvol and image=/dir/a.img.

If that's valid and needs to be supported, then the above strspn based
parsing logic needs some rewrite.

Regards,
Bharata.