diff mbox

[v2] xen_disk: cope with missing xenstore "params" node

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

Commit Message

Stefano Stabellini June 24, 2011, 4:35 p.m. UTC 
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.


Updated in v2:

- actually removed the strchr(blkdev->params, ':') that caused the
segfault;

- free all the allocated strings from xenstore before returning;

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

Comments

Peter Maydell June 25, 2011, 10:09 p.m. UTC | #1 
On 24 June 2011 17:35,  <stefano.stabellini@eu.citrix.com> wrote:
> +out_error:
> +    qemu_free(blkdev->params);
> +    qemu_free(blkdev->mode);
> +    qemu_free(blkdev->type);
> +    qemu_free(blkdev->dev);
> +    qemu_free(blkdev->devtype);
> +    return -1;

It occured to me that could result in a double-free if it's
possible to call init again (or to call free) after the init
routine has returned failure. I don't know enough about the
Xen device lifecycle to know if that's possible, though -- is it?

thanks
-- PMM
Stefano Stabellini June 27, 2011, 1:40 p.m. UTC | #2 
On Sat, 25 Jun 2011, Peter Maydell wrote:
> On 24 June 2011 17:35,  <stefano.stabellini@eu.citrix.com> wrote:
> > +out_error:
> > +    qemu_free(blkdev->params);
> > +    qemu_free(blkdev->mode);
> > +    qemu_free(blkdev->type);
> > +    qemu_free(blkdev->dev);
> > +    qemu_free(blkdev->devtype);
> > +    return -1;
> 
> It occured to me that could result in a double-free if it's
> possible to call init again (or to call free) after the init
> routine has returned failure. I don't know enough about the
> Xen device lifecycle to know if that's possible, though -- is it?

It shouldn't happen, but xen_disk should be able to cope with it
nonetheless.
I am going to resend the patch again setting to NULL all the blkdev
fields after freeing them.
diff mbox

Patch

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 096d1c9..eec05dd 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -616,12 +616,14 @@  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;
 
     /* read xenstore entries */
     if (blkdev->params == NULL) {
+        char *h = NULL;
         blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
-        h = strchr(blkdev->params, ':');
+        if (blkdev->params != NULL) {
+            h = strchr(blkdev->params, ':');
+        }
         if (h != NULL) {
             blkdev->fileproto = blkdev->params;
             blkdev->filename  = h+1;
@@ -649,7 +651,7 @@  static int blk_init(struct XenDevice *xendev)
         blkdev->mode == NULL   ||
         blkdev->type == NULL   ||
         blkdev->dev == NULL) {
-        return -1;
+        goto out_error;
     }
 
     /* read-only ? */
@@ -672,10 +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) {
+            goto out_error;
         }
     } else {
         /* setup via qemu cmdline -> already setup for us */
@@ -704,6 +711,14 @@  static int blk_init(struct XenDevice *xendev)
     xenstore_write_be_int(&blkdev->xendev, "sectors",
                           blkdev->file_size / blkdev->file_blk);
     return 0;
+
+out_error:
+    qemu_free(blkdev->params);
+    qemu_free(blkdev->mode);
+    qemu_free(blkdev->type);
+    qemu_free(blkdev->dev);
+    qemu_free(blkdev->devtype);
+    return -1;
 }
 
 static int blk_connect(struct XenDevice *xendev)