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

[Bharata B Rao]

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.