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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 24 Jun 2011, Peter Maydell wrote:
> On 24 June 2011 15:50,  <stefano.stabellini@xxxxxxxxxxxxx> 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel