From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56102) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TGuBo-000317-8x for qemu-devel@nongnu.org; Wed, 26 Sep 2012 12:10:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TGuBi-0001cP-6D for qemu-devel@nongnu.org; Wed, 26 Sep 2012 12:10:20 -0400 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:55706) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TGuBh-0001HR-Dk for qemu-devel@nongnu.org; Wed, 26 Sep 2012 12:10:14 -0400 Received: from /spool/local by e23smtp09.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 27 Sep 2012 02:07:18 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q8QG9Xfr23461946 for ; Thu, 27 Sep 2012 02:09:33 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q8QG9XOP023294 for ; Thu, 27 Sep 2012 02:09:33 +1000 Date: Wed, 26 Sep 2012 21:41:32 +0530 From: Bharata B Rao Message-ID: <20120926161132.GA32195@in.ibm.com> References: <20120924091008.GJ18470@in.ibm.com> <20120924091340.GN18470@in.ibm.com> <5062D24F.7030304@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5062D24F.7030304@redhat.com> Subject: Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend. Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Anthony Liguori , Anand Avati , Vijay Bellur , Stefan Hajnoczi , Harsh Bora , Amar Tumballi , qemu-devel@nongnu.org, "Richard W.M. Jones" , Blue Swirl , Avi Kivity , Paolo Bonzini , Daniel Veillard 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.