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

[Bharata B Rao]

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.