Re: [Qemu-devel] [PATCH v4 2/2] block: Support GlusterFS as a QEMU block backend

[Bharata B Rao]

On Wed, Aug 01, 2012 at 06:35:22PM +0000, Blue Swirl wrote:
> > +
> > +    if (!transport) {
> > +        uri->transport = strdup("socket");
> 
> g_strdup

Sorry about that, pitfalls of developing the parsing code out of line :(

> > +static int qemu_gluster_parseuri(GlusterURI *uri, const char *filename)
> > +{
> > +    char *token, *saveptr;
> > +    char *p, *r;
> > +    int ret = -EINVAL;
> > +
> > +    p = r = g_strdup(filename);
> 
> Why?

- Are you asking why use 2 variables ? I need them because I loose p and
  need r to free the string.
- Or are you asking why strdup ? That's because filename is const char *
  and I need to modify the filename when parsing.

> > +static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void 
> > *arg)
> > +{
> > +    GlusterAIOCB *acb = (GlusterAIOCB *)arg;
> > +    BDRVGlusterState *s = acb->common.bs->opaque;
> > +
> > +    acb->ret = ret;
> > +    if (qemu_gluster_send_pipe(s, acb) < 0) {
> > +        error_report("Could not complete read/write/flush from gluster");
> > +        abort();
> 
> Aborting is a bit drastic, it would be nice to save and exit gracefully.

I am not sure if there is an easy way to recover sanely and exit from this
kind of error.

Here the non-QEMU thread (gluster thread) failed to notify the QEMU thread
on the read side of the pipe about the IO completion. So essentially
bdrv_read or bdrv_write will never complete if this error happens.

Do you have any suggestions on how to exit gracefully here ?

> > +static QEMUOptionParameter qemu_gluster_create_options[] = {
> 
> 'const'?

Hmm no precedence of const usage for identical scenario in  other block
drivers in QEMU.

> 
> > +    {
> > +        .name = BLOCK_OPT_SIZE,
> > +        .type = OPT_SIZE,
> > +        .help = "Virtual disk size"
> > +    },
> > +    { NULL }
> > +};
> > +
> > +static BlockDriver bdrv_gluster = {
> 
> 'const'?

Again dodn't see the precedence for this.

Thanks for your review.

Regards,
Bharata.