On Wed, Sep 26, 2012 at 12:00:47PM +0200, Kevin Wolf wrote: > Am 24.09.2012 11:13, schrieb Bharata B Rao: > > +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.
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 '?'. If you think using saveptr is fine, then I could use that as below... /* image */ if (!*saveptr) { return -EINVAL; } gconf->image = g_strdup(saveptr); > > > + if (!token) { > > + return -EINVAL; > > + } > > + gconf->image = g_strdup(token); > > + return 0; > > +} > > + > > + > > + if (uri->query) { > > + unescape_str = uri_string_unescape(uri->query, -1, NULL); > > + if (!unescape_str) { > > + ret = -EINVAL; > > + goto out; > > + } > > + } > > I agree with Paolo here, this need to go away. Ok will do that. > > + > > + if (is_unix) { > > + if (strcmp(qp->p[0].name, "socket")) { > > + ret = -EINVAL; > > + goto out; > > + } > > + gconf->server = g_strdup(qp->p[0].value); > > Maybe add a check that uri->server is empty? I am saying that we will ignore the server and port if transport type is unix. But I guess I will add this check and change the comments and patch description accordingly. > > > + } else { > > + gconf->server = g_strdup(uri->server); > > + gconf->port = uri->port; > > + } > > + > > +out: > > + if (qp) { > > + query_params_free(qp); > > + } > > + g_free(unescape_str); > > + uri_free(uri); > > + return ret; > > +} > > + > > +static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char > > *filename) > > +{ > > + struct glfs *glfs = NULL; > > + int ret; > > + > > + ret = qemu_gluster_parseuri(gconf, filename); > > + if (ret < 0) { > > + error_report("Usage: file=gluster[+transport]://[server[:port]]/" > > + "volname/image[?socket=...]"); > > + errno = -ret; > > + goto out; > > + } > > + > > + glfs = glfs_new(gconf->volname); > > + if (!glfs) { > > + goto out; > > + } > > + > > + ret = glfs_set_volfile_server(glfs, gconf->transport, gconf->server, > > + gconf->port); > > + if (ret < 0) { > > + goto out; > > + } > > + > > + /* > > + * TODO: Use GF_LOG_ERROR instead of hard code value of 4 here when > > + * GlusterFS makes GF_LOG_* macros available to libgfapi users. > > + */ > > + ret = glfs_set_logging(glfs, "-", 4); > > + if (ret < 0) { > > + goto out; > > + } > > + > > + ret = glfs_init(glfs); > > + if (ret) { > > + error_report("Gluster connection failed for server=%s port=%d " > > + "volume=%s image=%s transport=%s\n", gconf->server, > > gconf->port, > > + gconf->volname, gconf->image, gconf->transport); > > + goto out; > > + } > > + return glfs; > > + > > +out: > > + if (glfs) { > > + glfs_fini(glfs); > > Does this corrupt errno? Currently glfs_fini() isn't implemented and it returns -1. I guess it could modify errno when its implemented. At one point of time, I had a logic to save the errno value from previous calls and restore it to errno if glfs_fini() fails, but that looked ugly since I had to save errno values from 4 previous calls. Should I just save the errno from glfs_init() since that does most of the validation, connection establishment etc and is more likely to fail ? > > +static int qemu_gluster_open(BlockDriverState *bs, const char *filename, > > + int bdrv_flags) > > +{ > > + BDRVGlusterState *s = bs->opaque; > > + int open_flags = 0; > > + int ret = 0; > > + GlusterConf *gconf = g_malloc0(sizeof(GlusterConf)); > > + > > + s->glfs = qemu_gluster_init(gconf, filename); > > + if (!s->glfs) { > > + ret = -errno; > > + goto out; > > + } > > + > > + open_flags |= O_BINARY; > > + open_flags &= ~O_ACCMODE; > > open_flags == O_BINARY here, so no O_ACCMODE bits to clear. Right, will fix. > > > +static int qemu_gluster_send_pipe(BDRVGlusterState *s, GlusterAIOCB *acb) > > +{ > > + int ret = 0; > > + > > + while (1) { > > + int fd = s->fds[GLUSTER_FD_WRITE]; > > + > > + ret = write(fd, (void *)&acb, sizeof(acb)); > > + if (ret >= 0) { > > + break; > > + } > > + if (errno == EINTR) { > > + continue; > > + } > > + if (errno != EAGAIN) { > > + break; > > + } > > Variatio delectat? ;-) > > How about just do { ... } while (errno == EINTR || errno == EAGAIN); ? I will go with qemu_write_full(). With that I could get rid of qemu_gluster_send_pipe() totally. Regards, Bharata.