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

[Blue Swirl]

On Thu, Aug 2, 2012 at 3:55 AM, Bharata B Rao
<address@hidden> wrote:
> 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.

OK.

>
>> > +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 ?

Ignore but set the callback return to -EIO, see for example curl.c:249.

>
>> > +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.

OK.

>
> Thanks for your review.
>
> Regards,
> Bharata.
>