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

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

[stefano.stabellini]

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

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

[Qemu-devel] [PATCH] xen_disk: treat "aio" as "raw"

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Sometimes the toolstack uses "aio" without an additional format
identifier, in such cases use "raw".

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

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 801da58..745127d 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -633,6 +633,8 @@ static int blk_init(struct XenDevice *xendev)
             blkdev->filename  = blkdev->params;
         }
     }
+    if (!strcmp("aio", blkdev->fileproto))
+        blkdev->fileproto = "raw";
     if (blkdev->mode == NULL) {
         blkdev->mode = xenstore_read_be_str(&blkdev->xendev, "mode");
     }
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH] xen_disk: treat "aio" as "raw"

[Alexander Graf]

On 06/24/2011 04:50 PM, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>
> Sometimes the toolstack uses "aio" without an additional format
> identifier, in such cases use "raw".

Shouldn't this rather be a patch to the toolstack then? We do automatic 
file format recognition as default. I find magical "aio equals raw" 
subtleties rather unintuitive.


> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> ---
>   hw/xen_disk.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> index 801da58..745127d 100644
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -633,6 +633,8 @@ static int blk_init(struct XenDevice *xendev)
>               blkdev->filename  = blkdev->params;
>           }
>       }
> +    if (!strcmp("aio", blkdev->fileproto))
> +        blkdev->fileproto = "raw";

Braces

>       if (blkdev->mode == NULL) {
>           blkdev->mode = xenstore_read_be_str(&blkdev->xendev, "mode");
>       }


Alex

Re: [Qemu-devel] [PATCH] xen_disk: treat "aio" as "raw"

[Kevin Wolf]

Am 30.06.2011 13:47, schrieb Alexander Graf:
> On 06/24/2011 04:50 PM, stefano.stabellini@eu.citrix.com wrote:
>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>
>> Sometimes the toolstack uses "aio" without an additional format
>> identifier, in such cases use "raw".
> 
> Shouldn't this rather be a patch to the toolstack then? We do automatic 
> file format recognition as default. I find magical "aio equals raw" 
> subtleties rather unintuitive.

The asynchronous raw blktap driver is (was?) called "aio", so I guess
it's to stay compatible with that. Has always been this way in Xen (and
has always been confusing). As long as it stays in xen_disk.c, I don't
really mind...

Kevin

Re: [Qemu-devel] [PATCH] xen_disk: treat "aio" as "raw"

[Alexander Graf]

On 06/30/2011 02:47 PM, Kevin Wolf wrote:
> Am 30.06.2011 13:47, schrieb Alexander Graf:
>> On 06/24/2011 04:50 PM, stefano.stabellini@eu.citrix.com wrote:
>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>
>>> Sometimes the toolstack uses "aio" without an additional format
>>> identifier, in such cases use "raw".
>> Shouldn't this rather be a patch to the toolstack then? We do automatic
>> file format recognition as default. I find magical "aio equals raw"
>> subtleties rather unintuitive.
> The asynchronous raw blktap driver is (was?) called "aio", so I guess
> it's to stay compatible with that. Has always been this way in Xen (and
> has always been confusing). As long as it stays in xen_disk.c, I don't
> really mind...

Is there a realistic chance of changing the toolstack here? I'd really 
prefer not to drag this legacy around :).


Alex

Re: [Qemu-devel] [PATCH] xen_disk: treat "aio" as "raw"

[Stefano Stabellini]

On Thu, 30 Jun 2011, Alexander Graf wrote:
> On 06/30/2011 02:47 PM, Kevin Wolf wrote:
> > Am 30.06.2011 13:47, schrieb Alexander Graf:
> >> On 06/24/2011 04:50 PM, stefano.stabellini@eu.citrix.com wrote:
> >>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >>>
> >>> Sometimes the toolstack uses "aio" without an additional format
> >>> identifier, in such cases use "raw".
> >> Shouldn't this rather be a patch to the toolstack then? We do automatic
> >> file format recognition as default. I find magical "aio equals raw"
> >> subtleties rather unintuitive.
> > The asynchronous raw blktap driver is (was?) called "aio", so I guess
> > it's to stay compatible with that. Has always been this way in Xen (and
> > has always been confusing). As long as it stays in xen_disk.c, I don't
> > really mind...
> 
> Is there a realistic chance of changing the toolstack here? I'd really 
> prefer not to drag this legacy around :).

We could change the toolstack but it would mean introducing an
incompatibility between the qdisk backend and the blktap backend.
Considering that qdisk was written to replace blktap in the first place
I don't think is such a good idea. Besides all xen toolstacks have
always written "aio" for "raw" even before qdisk came along...

Re: [Qemu-devel] [PATCH] xen_disk: treat "aio" as "raw"

[Alexander Graf]

On 06/30/2011 04:20 PM, Stefano Stabellini wrote:
> On Thu, 30 Jun 2011, Alexander Graf wrote:
>> On 06/30/2011 02:47 PM, Kevin Wolf wrote:
>>> Am 30.06.2011 13:47, schrieb Alexander Graf:
>>>> On 06/24/2011 04:50 PM, stefano.stabellini@eu.citrix.com wrote:
>>>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>>>
>>>>> Sometimes the toolstack uses "aio" without an additional format
>>>>> identifier, in such cases use "raw".
>>>> Shouldn't this rather be a patch to the toolstack then? We do automatic
>>>> file format recognition as default. I find magical "aio equals raw"
>>>> subtleties rather unintuitive.
>>> The asynchronous raw blktap driver is (was?) called "aio", so I guess
>>> it's to stay compatible with that. Has always been this way in Xen (and
>>> has always been confusing). As long as it stays in xen_disk.c, I don't
>>> really mind...
>> Is there a realistic chance of changing the toolstack here? I'd really
>> prefer not to drag this legacy around :).
> We could change the toolstack but it would mean introducing an
> incompatibility between the qdisk backend and the blktap backend.
> Considering that qdisk was written to replace blktap in the first place
> I don't think is such a good idea. Besides all xen toolstacks have
> always written "aio" for "raw" even before qdisk came along...

Hrm. Ok. Please resend with braces then :).


Alex

Re: [Qemu-devel] [PATCH] xen_disk: treat "aio" as "raw"

[Stefano Stabellini]

On Thu, 30 Jun 2011, Alexander Graf wrote:
> On 06/30/2011 04:20 PM, Stefano Stabellini wrote:
> > On Thu, 30 Jun 2011, Alexander Graf wrote:
> >> On 06/30/2011 02:47 PM, Kevin Wolf wrote:
> >>> Am 30.06.2011 13:47, schrieb Alexander Graf:
> >>>> On 06/24/2011 04:50 PM, stefano.stabellini@eu.citrix.com wrote:
> >>>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >>>>>
> >>>>> Sometimes the toolstack uses "aio" without an additional format
> >>>>> identifier, in such cases use "raw".
> >>>> Shouldn't this rather be a patch to the toolstack then? We do automatic
> >>>> file format recognition as default. I find magical "aio equals raw"
> >>>> subtleties rather unintuitive.
> >>> The asynchronous raw blktap driver is (was?) called "aio", so I guess
> >>> it's to stay compatible with that. Has always been this way in Xen (and
> >>> has always been confusing). As long as it stays in xen_disk.c, I don't
> >>> really mind...
> >> Is there a realistic chance of changing the toolstack here? I'd really
> >> prefer not to drag this legacy around :).
> > We could change the toolstack but it would mean introducing an
> > incompatibility between the qdisk backend and the blktap backend.
> > Considering that qdisk was written to replace blktap in the first place
> > I don't think is such a good idea. Besides all xen toolstacks have
> > always written "aio" for "raw" even before qdisk came along...
> 
> Hrm. Ok. Please resend with braces then :).

I'll do :)

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

[Peter Maydell]

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

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

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.

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

[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.stabellini@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, ':');
 	if (h != NULL) {
 	    blkdev->fileproto = blkdev->params;
 	    blkdev->filename  = h+1;

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

[Kevin Wolf]

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.stabellini@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.

>  	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?

Kevin

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.stabellini@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;

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

[Kevin Wolf]

Am 11.02.2011 13:59, schrieb 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.stabellini@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.

Thanks, I missed that one.

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

Yes, I think this is more explicit, and therefore easier to read.

Kevin