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