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.
> + 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: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().
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?
