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

[Stefano Stabellini]

On Fri, 11 Feb 2011, Kevin Wolf wrote:
> Am 11.02.2011 13:38, schrieb Stefano Stabellini:
> > 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.stabell...@eu.citrix.com>
> > 
> > 
> > diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> > index 134ac33..e553c4c 100644
> > --- a/hw/xen_disk.c
> > +++ b/hw/xen_disk.c
> > @@ -577,12 +577,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");
> > -        h = strchr(blkdev->params, ':');
> > +        if (blkdev->params != NULL)
> > +            h = strchr(blkdev->params, ':');
> 
> The coding style requires braces here.
> 

Good point, I'll do.

> >     if (h != NULL) {
> >         blkdev->fileproto = blkdev->params;
> >         blkdev->filename  = h+1;
> 
> Let me add some more context:
> 
>     if (h != NULL) {
>         blkdev->fileproto = blkdev->params;
>         blkdev->filename  = h+1;
>         *h = 0;
>     } else {
>         blkdev->fileproto = "<unset>";
>         blkdev->filename  = blkdev->params;
>     }
> 
> So in the NULL case we now have blkdev->filename = NULL. Doesn't this
> just move the crash a few lines downwards when bdrv_open() tries to use
> NULL as its filename?

There is a check on blkdev->params being NULL few lines after so we just
return.
Maybe an explicit return -1 like in the appended patch here would be
better?



diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 134ac33..fc0de14 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -582,6 +582,9 @@ static int blk_init(struct XenDevice *xendev)
     /* read xenstore entries */
     if (blkdev->params == NULL) {
        blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
+        if (blkdev->params == NULL) {
+            return -1;
+        }
         h = strchr(blkdev->params, ':');
        if (h != NULL) {
            blkdev->fileproto = blkdev->params;