Re: [Qemu-devel] [RFC PATCH 2/2] block: gluster as block backend

[Stefan Hajnoczi]

On Sat, Jul 21, 2012 at 9:31 AM, Bharata B Rao
<address@hidden> wrote:
> +typedef struct GlusterAIOCB {
> +    BlockDriverAIOCB common;
> +    QEMUIOVector *qiov;

The qiov field is unused.

> +    char *bounce;

Unused.

> +    struct BDRVGlusterState *s;

You can get this through common.bs->opaque, but if you like having a
shortcut, that's fine.

> +    int cancelled;

bool

> +} GlusterAIOCB;
> +
> +typedef struct GlusterCBKData {
> +    GlusterAIOCB *acb;
> +    struct BDRVGlusterState *s;
> +    int64_t size;
> +    int ret;
> +} GlusterCBKData;

I think GlusterCBKData could just be part of GlusterAIOCB.  That would
simplify the code a little and avoid some malloc/free.

> +
> +typedef struct BDRVGlusterState {
> +    struct glfs *glfs;
> +    int fds[2];
> +    int open_flags;
> +    struct glfs_fd *fd;
> +    int qemu_aio_count;
> +    int event_reader_pos;
> +    GlusterCBKData *event_gcbk;
> +} BDRVGlusterState;
> +
> +#define GLUSTER_FD_READ 0
> +#define GLUSTER_FD_WRITE 1
> +
> +static void qemu_gluster_complete_aio(GlusterCBKData *gcbk)
> +{
> +    GlusterAIOCB *acb = gcbk->acb;
> +    int ret;
> +
> +    if (acb->cancelled) {

Where does cancelled get set?

> +        qemu_aio_release(acb);
> +        goto done;
> +    }
> +
> +    if (gcbk->ret == gcbk->size) {
> +        ret = 0; /* Success */
> +    } else if (gcbk->ret < 0) {
> +        ret = gcbk->ret; /* Read/Write failed */
> +    } else {
> +        ret = -EINVAL; /* Partial read/write - fail it */

EINVAL is for invalid arguments.  EIO would be better.

> +/*
> + * file=protocol:address@hidden:volname:image
> + */
> +static int qemu_gluster_parsename(GlusterConf *c, const char *filename)
> +{
> +    char *file = g_strdup(filename);
> +    char *token, *next_token, *saveptr;
> +    char *token_s, *next_token_s, *saveptr_s;
> +    int ret = -EINVAL;
> +
> +    /* Discard the protocol */
> +    token = strtok_r(file, ":", &saveptr);
> +    if (!token) {
> +        goto out;
> +    }
> +
> +    /* address@hidden */
> +    next_token = strtok_r(NULL, ":", &saveptr);
> +    if (!next_token) {
> +        goto out;
> +    }
> +    if (strchr(next_token, '@')) {
> +        token_s = strtok_r(next_token, "@", &saveptr_s);
> +        if (!token_s) {
> +            goto out;
> +        }
> +        strncpy(c->server, token_s, HOST_NAME_MAX);

strncpy(3) will not NUL-terminate when token_s is HOST_NAME_MAX
characters long.  QEMU has cutils.c:pstrcpy().

When the argument is too long we should probably report an error
instead of truncating.

Same below.

> +        next_token_s = strtok_r(NULL, "@", &saveptr_s);
> +        if (!next_token_s) {
> +            goto out;
> +        }
> +        c->port = atoi(next_token_s);

No error checking.  If the input is invalid an error message would
help the user here.

> +static struct glfs *qemu_gluster_init(GlusterConf *c, const char *filename)
> +{
> +    struct glfs *glfs = NULL;
> +    int ret;
> +
> +    ret = qemu_gluster_parsename(c, filename);
> +    if (ret < 0) {
> +        errno = -ret;
> +        goto out;
> +    }
> +
> +    glfs = glfs_new(c->volname);
> +    if (!glfs) {
> +        goto out;
> +    }
> +
> +    ret = glfs_set_volfile_server(glfs, "socket", c->server, c->port);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
> +    /*
> +     * TODO: Logging is not necessary but instead nice to have.
> +     * Can QEMU optionally log into a standard place ?

QEMU prints to stderr, can you do that here too?  The global log file
is not okay, especially when multiple QEMU instances are running.

> +     * Need to use defines like gf_loglevel_t:GF_LOG_INFO instead of
> +     * hard coded values like 7 here.
> +     */
> +    ret = glfs_set_logging(glfs, "/tmp/qemu-gluster.log", 7);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
> +    ret = glfs_init(glfs);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +    return glfs;
> +
> +out:
> +    if (glfs) {
> +        (void)glfs_fini(glfs);
> +    }
> +    return NULL;
> +}
> +
> +static int qemu_gluster_open(BlockDriverState *bs, const char *filename,
> +    int bdrv_flags)
> +{
> +    BDRVGlusterState *s = bs->opaque;
> +    GlusterConf *c = g_malloc(sizeof(GlusterConf));

Can this be allocated on the stack?

> +    int ret;
> +
> +    s->glfs = qemu_gluster_init(c, filename);
> +    if (!s->glfs) {
> +        ret = -errno;
> +        goto out;
> +    }
> +
> +    s->open_flags |=  O_BINARY;

Can open_flags be a local variable?

> +static int qemu_gluster_create(const char *filename,
> +        QEMUOptionParameter *options)
> +{
> +    struct glfs *glfs;
> +    struct glfs_fd *fd;
> +    GlusterConf *c = g_malloc(sizeof(GlusterConf));
> +    int ret = 0;
> +    int64_t total_size = 0;
> +
> +    glfs = qemu_gluster_init(c, filename);
> +    if (!glfs) {
> +        ret = -errno;
> +        goto out;
> +    }
> +
> +    /* Read out options */
> +    while (options && options->name) {
> +        if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> +            total_size = options->value.n / BDRV_SECTOR_SIZE;
> +        }
> +        options++;
> +    }
> +
> +    fd = glfs_creat(glfs, c->image, O_WRONLY|O_CREAT|O_TRUNC|O_BINARY, 
> S_IRWXU);

Why set the execute permission bit?

> +static void qemu_gluster_close(BlockDriverState *bs)
> +{
> +    BDRVGlusterState *s = bs->opaque;
> +
> +    if (s->fd) {
> +        glfs_close(s->fd);
> +        s->fd = NULL;
> +    }

Why not call glfs_fini() here?