Patchwork xen_disk: cope with missing xenstore "params" node

login
register
mail settings
Submitter Stefano Stabellini
Date June 24, 2011, 2:50 p.m.
Message ID <1308927034-10209-1-git-send-email-stefano.stabellini@eu.citrix.com>
Download mbox | patch
Permalink /patch/101816/
State New
Headers show

Comments

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

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");