xen_disk: cope with missing xenstore "params" node

Submitted by Stefano Stabellini on June 24, 2011, 2:50 p.m.

Details

Message ID 1308927034-10209-1-git-send-email-stefano.stabellini@eu.citrix.com
State New
Headers show

Commit Message

Stefano Stabellini June 24, 2011, 2:50 p.m.
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

When disk is a cdrom and the drive is empty the "params" node in
xenstore might be missing completely: cope with it instead of
segfaulting.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen_disk.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

Comments

Peter Maydell June 24, 2011, 3:06 p.m.
On 24 June 2011 15:50,  <stefano.stabellini@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.

> @@ -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() ?

-- PMM
Stefano Stabellini June 24, 2011, 4:34 p.m.
On Fri, 24 Jun 2011, Peter Maydell wrote:
> On 24 June 2011 15:50,  <stefano.stabellini@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.

Patch hide | download patch | download mbox

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 096d1c9..801da58 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -616,11 +616,13 @@  static int blk_init(struct XenDevice *xendev)
 {
     struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
     int index, qflags, have_barriers, info = 0;
-    char *h;
+    char *h = NULL;
 
     /* 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, ':');
         if (h != NULL) {
             blkdev->fileproto = blkdev->params;
@@ -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;
     } else {
         /* setup via qemu cmdline -> already setup for us */
         xen_be_printf(&blkdev->xendev, 2, "get configured bdrv (cmdline setup)\n");