Xen devel - Jun 2011 - [PATCH] xen_disk: cope with missing xenstore "params" node

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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.
--8323329-1773869790-1308930095=:12963
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

--8323329-1773869790-1308930095=:12963--

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel