qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Stefan Hajnoczi
Subject: Re: [Qemu-devel] [RFC PATCH 3/3] block: gluster as block backend
Date: Mon, 18 Jun 2012 18:35:28 +0100

On Mon, Jun 11, 2012 at 3:21 PM, Bharata B Rao
<address@hidden> wrote:
> +#include "block_int.h"
> +#include "gluster-helpers.h"
> +
> +typedef void *gluster_file_t;

This typedef is already in gluster-helpers.h.  It's ugly BTW, "typedef
struct gluster_file gluster_file_t" is nicer since it won't cast to
other pointer types automatically.

> +
> +typedef struct glusterConf {
> +    char volfile[PATH_MAX];
> +    char image[PATH_MAX];
> +} glusterConf;

QEMU coding style always uses UpperCase for struct names.

> +static void qemu_gluster_aio_event_reader(void *opaque)
> +{
> +    BDRVGlusterState *s = opaque;
> +    ssize_t ret;
> +
> +    do {
> +        char *p = (char *)&s->event_gaiocb;

Why make this a BDRVGlusterState field?  It could be a local, I think.

> +    /* Use O_DSYNC for write-through caching, no flags for write-back 
> caching,
> +     * and O_DIRECT for no caching. */
> +    if ((bdrv_flags & BDRV_O_NOCACHE))
> +        s->open_flags |= O_DIRECT;
> +    if (!(bdrv_flags & BDRV_O_CACHE_WB))
> +        s->open_flags |= O_DSYNC;

Paolo has changed this recently, you might need to use
bs->enable_write_cache instead.

> +out:
> +    if (c) {
> +        g_free(c);
> +    }

g_free(NULL) is a nop, you never need to test that the pointer is non-NULL.

> +static void gluster_finish_aiocb(void *arg)
> +{
> +    int ret;
> +    gluster_aiocb_t *gaiocb = (gluster_aiocb_t *)arg;
> +    BDRVGlusterState *s = ((glusterAIOCB *)gaiocb->opaque)->s;
> +
> +    ret = qemu_gluster_send_pipe(s, gaiocb);
> +    if (ret < 0) {
> +        g_free(gaiocb);

What about the glusterAIOCB?  You need to invoke the callback with an
error value.

What about decrementing the in-flight I/O request count?

> +static BlockDriverAIOCB *qemu_gluster_aio_rw(BlockDriverState *bs,
> +        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
> +        BlockDriverCompletionFunc *cb, void *opaque, int write)
> +{
> +    int ret;
> +    glusterAIOCB *acb;
> +    gluster_aiocb_t *gaiocb;
> +    BDRVGlusterState *s = bs->opaque;
> +    char *buf;
> +    size_t size;
> +    off_t offset;
> +
> +    acb = qemu_aio_get(&gluster_aio_pool, bs, cb, opaque);
> +    acb->write = write;
> +    acb->qiov = qiov;
> +    acb->bounce = qemu_blockalign(bs, qiov->size);
> +    acb->ret = 0;
> +    acb->bh = NULL;
> +    acb->s = s;
> +
> +    if (write) {
> +        qemu_iovec_to_buffer(acb->qiov, acb->bounce);
> +    }
> +
> +    buf = acb->bounce;
> +    offset = sector_num * BDRV_SECTOR_SIZE;
> +    size = nb_sectors * BDRV_SECTOR_SIZE;
> +    s->qemu_aio_count++;
> +
> +    gaiocb = g_malloc(sizeof(gluster_aiocb_t));

Can you make this a field of glusterAIOCB?  Then you don't need to
worry about freeing gaiocb later.

> +static int64_t qemu_gluster_getlength(BlockDriverState *bs)
> +{
> +    BDRVGlusterState *s = bs->opaque;
> +    gluster_file_t fd = s->fd;
> +    struct stat st;
> +    int ret;
> +
> +    ret = gluster_fstat(fd, &st);
> +    if (ret < 0) {
> +        return -1;

Please return a negative errno instead of -1.

Stefan



reply via email to

[Prev in Thread] Current Thread [Next in Thread]