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