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

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Sun, Jul 22, 2012 at 04:38:00PM +0100, Stefan Hajnoczi wrote:
> On Sat, Jul 21, 2012 at 9:31 AM, Bharata B Rao
> <bharata@linux.vnet.ibm.com> wrote:
> > +typedef struct GlusterAIOCB {
> > +    BlockDriverAIOCB common;
> > +    QEMUIOVector *qiov;
> 
> The qiov field is unused.
> 
> > +    char *bounce;
> 
> Unused.

Yes, removed these two.

> 
> > +    struct BDRVGlusterState *s;
> 
> You can get this through common.bs->opaque, but if you like having a
> shortcut, that's fine.
> 
> > +    int cancelled;
> 
> bool

Ok.

> 
> > +} 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.

Are you suggesting to put a field

GlusterCBKData gcbk;

inside GlusterAIOCB and use gcbk from there or

Are you suggesting that I make the fields of GlusterCBKData part of
GlusterAIOCB and get rid of GlusterCBKData altogether ? This means I would
have to pass the GlusterAIOCB to gluster async calls and update its fields from
gluster callback routine. I can do this, but I am not sure if you can touch
the fields of GlusterAIOCB in non-QEMU threads (gluster callback thread).

> 
> > +
> > +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?

I realised that I am not supporting bdrv_aio_cancel(). I guess I will have
to add support for this in next version.

> 
> > +        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.

Ok.

> 
> > +/*
> > + * file=protocol:server@port: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;
> > +    }
> > +
> > +    /* server@port */
> > +    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().

Will use pstrcpy.

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

Or should we let gluster APIs to flag an error with truncated
server and volume names ?

> 
> 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.

Fixed.

> 
> > +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.

Ok, I can do glfs_set_logging(glfs, "/dev/stderr", loglevel);

> 
> > +     * 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?

It consists of PATH_MAX(4096), HOST_NAME_MAX(255) and GLUSTERD_MAX_VOLUME_NAME
(1000). A bit heavy to be on 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?

Yes, fixed.

> 
> > +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?

Changed to read and write only.

> 
> > +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?

Missed that, fixed now.

Thanks for your comments.

Regards,
Bharata.