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.
