[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 2/2] block: gluster as block backend
From: |
Bharata B Rao |
Subject: |
Re: [Qemu-devel] [RFC PATCH 2/2] block: gluster as block backend |
Date: |
Mon, 23 Jul 2012 14:02:57 +0530 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
> <address@hidden> 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: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().
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.