[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block
From: |
Bharata B Rao |
Subject: |
Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend. |
Date: |
Wed, 26 Sep 2012 21:41:32 +0530 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
- [Qemu-devel] [PATCH v9 1/4] aio: Fix qemu_aio_wait() to maintain correct walking_handlers count, (continued)
- [Qemu-devel] [PATCH v9 1/4] aio: Fix qemu_aio_wait() to maintain correct walking_handlers count, Bharata B Rao, 2012/09/24
- [Qemu-devel] [PATCH v9 2/4] qemu: URI parsing library, Bharata B Rao, 2012/09/24
- [Qemu-devel] [PATCH v9 3/4] configure: Add a config option for GlusterFS as block backend, Bharata B Rao, 2012/09/24
- [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend., Bharata B Rao, 2012/09/24
- Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend., Paolo Bonzini, 2012/09/24
- Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend., Kevin Wolf, 2012/09/26
- Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend., Paolo Bonzini, 2012/09/26
- Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend.,
Bharata B Rao <=
- Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend., Paolo Bonzini, 2012/09/26
- Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend., Bharata B Rao, 2012/09/27
- Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend., Paolo Bonzini, 2012/09/27
- Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend., Bharata B Rao, 2012/09/27
- Re: [Qemu-devel] [PATCH v9 4/4] block: Support GlusterFS as a QEMU block backend., Paolo Bonzini, 2012/09/27