Re: [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node

[Stefano Stabellini]

On Fri, 24 Jun 2011, Peter Maydell wrote:
> On 24 June 2011 15:50,  <address@hidden> wrote:
> >     /* read xenstore entries */
> >     if (blkdev->params == NULL) {
> >         blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
> > +        if (blkdev->params != NULL)
> > +            h = strchr(blkdev->params, ':');
> >         h = strchr(blkdev->params, ':');
> 
> This adds the if () statement but it looks like you forgot to remove
> the strchr that is outside the if(), so this will still segfault...
> (Also, coding style demands braces.)
> 
> You could also make that "char *h" local to this 'if' block.

Thank you very much for the review, I'll make the changes.

> 
> > @@ -672,11 +674,15 @@ static int blk_init(struct XenDevice *xendev)
> >         /* setup via xenbus -> create new block driver instance */
> >         xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus 
> > setup)\n");
> >         blkdev->bs = bdrv_new(blkdev->dev);
> > -        if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
> > -                      bdrv_find_whitelisted_format(blkdev->fileproto)) != 
> > 0) {
> > -            bdrv_delete(blkdev->bs);
> > -            return -1;
> > +        if (blkdev->bs) {
> > +            if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
> > +                        bdrv_find_whitelisted_format(blkdev->fileproto)) 
> > != 0) {
> > +                bdrv_delete(blkdev->bs);
> > +                blkdev->bs = NULL;
> > +            }
> >         }
> > +        if (!blkdev->bs)
> > +            return -1;
> 
> Doesn't this error return leak the strings allocated by
> xenstore_read_be_str() ?

Another very good point, I'll introduce an out_error label and free
everything there.