BUG 4.2.2: xl cd-insert corrupts xenstore state

BUG 4.2.2: xl cd-insert corrupts xenstore state

[George Dunlap]

$ git checkout RELEASE-4.2.2
$ git clean -ffdx

[do a build, install, reboot]

$ xl create a0

$ xl cd-eject a0 hdc

[However, the guest OS still sees the CD inserted]

$ xl block-list a0
Segmentation fault

$ xl cd-insert a0 hdc file:/images/xs-tools-6.0.0.iso
Segmentation fault

The problem seems to be that xl cd-eject is writing corrupt values to
xenstore.  Note that vbd/1/5632 is still there as when it booted, but
qdisk/1/5632 has some random stuff in it

$ xenstore-ls -f /local/domain/0/backend
/local/domain/0/backend/qdisk = ""
/local/domain/0/backend/qdisk/1 = ""
/local/domain/0/backend/qdisk/1/768 = ""
/local/domain/0/backend/qdisk/1/768/frontend = "/local/domain/1/device/vbd/768"
/local/domain/0/backend/qdisk/1/768/params = "qcow:/images/vm/a0"
/local/domain/0/backend/qdisk/1/768/frontend-id = "1"
/local/domain/0/backend/qdisk/1/768/online = "1"
/local/domain/0/backend/qdisk/1/768/removable = "0"
/local/domain/0/backend/qdisk/1/768/bootable = "1"
/local/domain/0/backend/qdisk/1/768/state = "2"
/local/domain/0/backend/qdisk/1/768/dev = "hda"
/local/domain/0/backend/qdisk/1/768/type = "qdisk"
/local/domain/0/backend/qdisk/1/768/mode = "w"
/local/domain/0/backend/qdisk/1/768/device-type = "disk"
/local/domain/0/backend/qdisk/1/768/feature-barrier = "1"
/local/domain/0/backend/qdisk/1/768/info = "0"
/local/domain/0/backend/qdisk/1/768/sector-size = "512"
/local/domain/0/backend/qdisk/1/768/sectors = "33554432"
/local/domain/0/backend/qdisk/1/768/hotplug-status = "connected"
/local/domain/0/backend/qdisk/1/5632 = ""
/local/domain/0/backend/qdisk/1/5632/type = "qdisk"
/local/domain/0/backend/qdisk/1/5632/params = ""
/local/domain/0/backend/vbd = ""
/local/domain/0/backend/vbd/1 = ""
/local/domain/0/backend/vbd/1/5632 = ""
/local/domain/0/backend/vbd/1/5632/frontend = "/local/domain/1/device/vbd/5632"
/local/domain/0/backend/vbd/1/5632/tapdisk-params =
"aio:/images/autoinstall//w2k3eesp2.iso"
/local/domain/0/backend/vbd/1/5632/params = "/dev/xen/blktap-2/tapdev0"
/local/domain/0/backend/vbd/1/5632/script = "/etc/xen/scripts/block"
/local/domain/0/backend/vbd/1/5632/physical-device = "fd:0"
/local/domain/0/backend/vbd/1/5632/frontend-id = "1"
/local/domain/0/backend/vbd/1/5632/online = "1"
/local/domain/0/backend/vbd/1/5632/removable = "1"
/local/domain/0/backend/vbd/1/5632/bootable = "1"
/local/domain/0/backend/vbd/1/5632/state = "2"
/local/domain/0/backend/vbd/1/5632/dev = "hdc"
/local/domain/0/backend/vbd/1/5632/type = "phy"
/local/domain/0/backend/vbd/1/5632/mode = "r"
/local/domain/0/backend/vbd/1/5632/device-type = "cdrom"

The disk string from the config file is as follows:

disk = [ 'qcow:/images/vm/a0,hda,w','file:/images/autoinstall//w2k3eesp2.iso,hdc:cdrom,r'
]

 -George

Re: BUG 4.2.2: xl cd-insert corrupts xenstore state

[George Dunlap]

Ha!  I knew I'd seen this before... the following git c/s needs to be
backported:

commit c3556e2a1aee3c9b7dda5d57e85e8867fff1b9da
Author: George Dunlap <george.dunlap@eu.citrix.com>
Commit: George Dunlap <george.dunlap@eu.citrix.com>

    libxl: Fix bug in libxl_cdrom_insert, make more robust against bad
xenstore data

    libxl_cdrom_insert was failing to initialize the backend type,
    resulting in the wrong default backend.  The result was not only that
    the CD was not inserted properly, but also that some improper xenstore
    entries were created, causing further block commands to fail.

    This patch fixes the bug by setting the disk backend type based on the
    type of the existing device.

    It also makes the system more robust by checking to see that it has
    got a valid path before proceeding to write a partial xenstore entry.

    Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
    Acked-by: Ian Campbell <ian.campbell@citrix.com>
    Committed-by: Ian Campbell <ian.campbell@citrix.com>

On Fri, Apr 26, 2013 at 3:34 PM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> $ git checkout RELEASE-4.2.2
> $ git clean -ffdx
>
> [do a build, install, reboot]
>
> $ xl create a0
>
> $ xl cd-eject a0 hdc
>
> [However, the guest OS still sees the CD inserted]
>
> $ xl block-list a0
> Segmentation fault
>
> $ xl cd-insert a0 hdc file:/images/xs-tools-6.0.0.iso
> Segmentation fault
>
> The problem seems to be that xl cd-eject is writing corrupt values to
> xenstore.  Note that vbd/1/5632 is still there as when it booted, but
> qdisk/1/5632 has some random stuff in it
>
> $ xenstore-ls -f /local/domain/0/backend
> /local/domain/0/backend/qdisk = ""
> /local/domain/0/backend/qdisk/1 = ""
> /local/domain/0/backend/qdisk/1/768 = ""
> /local/domain/0/backend/qdisk/1/768/frontend = "/local/domain/1/device/vbd/768"
> /local/domain/0/backend/qdisk/1/768/params = "qcow:/images/vm/a0"
> /local/domain/0/backend/qdisk/1/768/frontend-id = "1"
> /local/domain/0/backend/qdisk/1/768/online = "1"
> /local/domain/0/backend/qdisk/1/768/removable = "0"
> /local/domain/0/backend/qdisk/1/768/bootable = "1"
> /local/domain/0/backend/qdisk/1/768/state = "2"
> /local/domain/0/backend/qdisk/1/768/dev = "hda"
> /local/domain/0/backend/qdisk/1/768/type = "qdisk"
> /local/domain/0/backend/qdisk/1/768/mode = "w"
> /local/domain/0/backend/qdisk/1/768/device-type = "disk"
> /local/domain/0/backend/qdisk/1/768/feature-barrier = "1"
> /local/domain/0/backend/qdisk/1/768/info = "0"
> /local/domain/0/backend/qdisk/1/768/sector-size = "512"
> /local/domain/0/backend/qdisk/1/768/sectors = "33554432"
> /local/domain/0/backend/qdisk/1/768/hotplug-status = "connected"
> /local/domain/0/backend/qdisk/1/5632 = ""
> /local/domain/0/backend/qdisk/1/5632/type = "qdisk"
> /local/domain/0/backend/qdisk/1/5632/params = ""
> /local/domain/0/backend/vbd = ""
> /local/domain/0/backend/vbd/1 = ""
> /local/domain/0/backend/vbd/1/5632 = ""
> /local/domain/0/backend/vbd/1/5632/frontend = "/local/domain/1/device/vbd/5632"
> /local/domain/0/backend/vbd/1/5632/tapdisk-params =
> "aio:/images/autoinstall//w2k3eesp2.iso"
> /local/domain/0/backend/vbd/1/5632/params = "/dev/xen/blktap-2/tapdev0"
> /local/domain/0/backend/vbd/1/5632/script = "/etc/xen/scripts/block"
> /local/domain/0/backend/vbd/1/5632/physical-device = "fd:0"
> /local/domain/0/backend/vbd/1/5632/frontend-id = "1"
> /local/domain/0/backend/vbd/1/5632/online = "1"
> /local/domain/0/backend/vbd/1/5632/removable = "1"
> /local/domain/0/backend/vbd/1/5632/bootable = "1"
> /local/domain/0/backend/vbd/1/5632/state = "2"
> /local/domain/0/backend/vbd/1/5632/dev = "hdc"
> /local/domain/0/backend/vbd/1/5632/type = "phy"
> /local/domain/0/backend/vbd/1/5632/mode = "r"
> /local/domain/0/backend/vbd/1/5632/device-type = "cdrom"
>
> The disk string from the config file is as follows:
>
> disk = [ 'qcow:/images/vm/a0,hda,w','file:/images/autoinstall//w2k3eesp2.iso,hdc:cdrom,r'
> ]
>
>  -George

Re: BUG 4.2.2: xl cd-insert corrupts xenstore state

[Ian Campbell]

On Fri, 2013-04-26 at 15:34 +0100, George Dunlap wrote:
> $ git checkout RELEASE-4.2.2
> $ git clean -ffdx
> 
> [do a build, install, reboot]
> 
> $ xl create a0
> 
> $ xl cd-eject a0 hdc
> 
> [However, the guest OS still sees the CD inserted]
> 
> $ xl block-list a0
> Segmentation fault
> 
> $ xl cd-insert a0 hdc file:/images/xs-tools-6.0.0.iso
> Segmentation fault

Can you get a backtrace out of gdb?

> The problem seems to be that xl cd-eject is writing corrupt values to
> xenstore.  Note that vbd/1/5632 is still there as when it booted, but
> qdisk/1/5632 has some random stuff in it
> 
> $ xenstore-ls -f /local/domain/0/backend
> /local/domain/0/backend/qdisk = ""
> /local/domain/0/backend/qdisk/1 = ""
> /local/domain/0/backend/qdisk/1/768 = ""
> /local/domain/0/backend/qdisk/1/768/frontend = "/local/domain/1/device/vbd/768"
> /local/domain/0/backend/qdisk/1/768/params = "qcow:/images/vm/a0"
> /local/domain/0/backend/qdisk/1/768/frontend-id = "1"
> /local/domain/0/backend/qdisk/1/768/online = "1"
> /local/domain/0/backend/qdisk/1/768/removable = "0"
> /local/domain/0/backend/qdisk/1/768/bootable = "1"
> /local/domain/0/backend/qdisk/1/768/state = "2"
> /local/domain/0/backend/qdisk/1/768/dev = "hda"
> /local/domain/0/backend/qdisk/1/768/type = "qdisk"
> /local/domain/0/backend/qdisk/1/768/mode = "w"
> /local/domain/0/backend/qdisk/1/768/device-type = "disk"
> /local/domain/0/backend/qdisk/1/768/feature-barrier = "1"
> /local/domain/0/backend/qdisk/1/768/info = "0"
> /local/domain/0/backend/qdisk/1/768/sector-size = "512"
> /local/domain/0/backend/qdisk/1/768/sectors = "33554432"
> /local/domain/0/backend/qdisk/1/768/hotplug-status = "connected"
> /local/domain/0/backend/qdisk/1/5632 = ""
> /local/domain/0/backend/qdisk/1/5632/type = "qdisk"
> /local/domain/0/backend/qdisk/1/5632/params = ""
> /local/domain/0/backend/vbd = ""
> /local/domain/0/backend/vbd/1 = ""
> /local/domain/0/backend/vbd/1/5632 = ""
> /local/domain/0/backend/vbd/1/5632/frontend = "/local/domain/1/device/vbd/5632"
> /local/domain/0/backend/vbd/1/5632/tapdisk-params =
> "aio:/images/autoinstall//w2k3eesp2.iso"
> /local/domain/0/backend/vbd/1/5632/params = "/dev/xen/blktap-2/tapdev0"
> /local/domain/0/backend/vbd/1/5632/script = "/etc/xen/scripts/block"
> /local/domain/0/backend/vbd/1/5632/physical-device = "fd:0"
> /local/domain/0/backend/vbd/1/5632/frontend-id = "1"
> /local/domain/0/backend/vbd/1/5632/online = "1"
> /local/domain/0/backend/vbd/1/5632/removable = "1"
> /local/domain/0/backend/vbd/1/5632/bootable = "1"
> /local/domain/0/backend/vbd/1/5632/state = "2"
> /local/domain/0/backend/vbd/1/5632/dev = "hdc"
> /local/domain/0/backend/vbd/1/5632/type = "phy"
> /local/domain/0/backend/vbd/1/5632/mode = "r"
> /local/domain/0/backend/vbd/1/5632/device-type = "cdrom"
> 
> The disk string from the config file is as follows:
> 
> disk = [ 'qcow:/images/vm/a0,hda,w','file:/images/autoinstall//w2k3eesp2.iso,hdc:cdrom,r'
> ]
> 
>  -George

Re: BUG 4.2.2: xl cd-insert corrupts xenstore state

[George Dunlap]

On 26/04/13 15:39, Ian Campbell wrote:
> On Fri, 2013-04-26 at 15:34 +0100, George Dunlap wrote:
>> $ git checkout RELEASE-4.2.2
>> $ git clean -ffdx
>>
>> [do a build, install, reboot]
>>
>> $ xl create a0
>>
>> $ xl cd-eject a0 hdc
>>
>> [However, the guest OS still sees the CD inserted]
>>
>> $ xl block-list a0
>> Segmentation fault
>>
>> $ xl cd-insert a0 hdc file:/images/xs-tools-6.0.0.iso
>> Segmentation fault
> Can you get a backtrace out of gdb?

Turns out I already fixed this once!

commit c3556e2a1aee3c9b7dda5d57e85e8867fff1b9da
Author: George Dunlap <george.dunlap@eu.citrix.com>
Commit: George Dunlap <george.dunlap@eu.citrix.com>

     libxl: Fix bug in libxl_cdrom_insert, make more robust against bad 
xenstore data

     libxl_cdrom_insert was failing to initialize the backend type,
     resulting in the wrong default backend.  The result was not only that
     the CD was not inserted properly, but also that some improper xenstore
     entries were created, causing further block commands to fail.

     This patch fixes the bug by setting the disk backend type based on the
     type of the existing device.

     It also makes the system more robust by checking to see that it has
     got a valid path before proceeding to write a partial xenstore entry.

     Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
     Acked-by: Ian Campbell <ian.campbell@citrix.com>
     Committed-by: Ian Campbell <ian.campbell@citrix.com>

Re: BUG 4.2.2: xl cd-insert corrupts xenstore state

[Ian Jackson]

George Dunlap writes ("Re: BUG 4.2.2: xl cd-insert corrupts xenstore state"):
> Turns out I already fixed this once!
> 
> commit c3556e2a1aee3c9b7dda5d57e85e8867fff1b9da
> Author: George Dunlap <george.dunlap@eu.citrix.com>
> Commit: George Dunlap <george.dunlap@eu.citrix.com>

Here's the backport, which was nearly trivial.

I'll push this to 4.2 staging unless someone objevts.

Ian.

commit 1eea2f30f79a1af817fe32c9e01673da1f31dd5f
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Wed May 1 16:56:54 2013 +0100

    libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data
    
    libxl_cdrom_insert was failing to initialize the backend type,
    resulting in the wrong default backend.  The result was not only that
    the CD was not inserted properly, but also that some improper xenstore
    entries were created, causing further block commands to fail.
    
    This patch fixes the bug by setting the disk backend type based on the
    type of the existing device.
    
    It also makes the system more robust by checking to see that it has
    got a valid path before proceeding to write a partial xenstore entry.
    
    Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
    Acked-by: Ian Campbell <ian.campbell@citrix.com>
    Committed-by: Ian Campbell <ian.campbell@citrix.com>
    (cherry picked from commit c3556e2a1aee3c9b7dda5d57e85e8867fff1b9da)
    
    Conflicts:
    	tools/libxl/libxl.c
    Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a6e9601..9f04e36 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2135,6 +2135,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
 
     libxl__device device;
     const char * path;
+    char * tmp;
 
     flexarray_t *insert = NULL;
 
@@ -2171,8 +2172,11 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     disks = libxl_device_disk_list(ctx, domid, &num);
     for (i = 0; i < num; i++) {
         if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev))
-            /* found */
+        {
+            /* Found.  Set backend type appropriately. */
+            disk->backend=disks[i].backend;
             break;
+        }
     }
     if (i == num) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual device not found");
@@ -2192,6 +2196,16 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     if (rc) goto out;
     path = libxl__device_backend_path(gc, &device);
 
+    /* Sanity check: make sure the backend exists before writing here */
+    tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/frontend", path));
+    if (!tmp)
+    {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Internal error: %s does not exist",
+            libxl__sprintf(gc, "%s/frontend", path));
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     insert = flexarray_make(4, 1);
 
     flexarray_append_pair(insert, "type",

Re: BUG 4.2.2: xl cd-insert corrupts xenstore state

[George Dunlap]

On 01/05/13 16:59, Ian Jackson wrote:
> George Dunlap writes ("Re: BUG 4.2.2: xl cd-insert corrupts xenstore state"):
>> Turns out I already fixed this once!
>>
>> commit c3556e2a1aee3c9b7dda5d57e85e8867fff1b9da
>> Author: George Dunlap <george.dunlap@eu.citrix.com>
>> Commit: George Dunlap <george.dunlap@eu.citrix.com>
> Here's the backport, which was nearly trivial.
>
> I'll push this to 4.2 staging unless someone objevts.

FYI, I'm pretty sure the effect this will have is to change the failure 
I was seeing in 4.2 (xl crashing) to the failure Fabio and I were seeing 
in 4.3 with cd-eject and cd-insert.

I think the situation without this patch is:
* cd-eject messes up xenstore, causes future block commands to crash
* cd-insert works
With this patch, I think what happen sis:
* cd-eject fails but does not mess up xenstore
* cd-insert also fails

It should be noted that this is only true *if blktap is available*; if 
blktap is not available, libxl uses qdev and everything works fine.

(And I should emphasize, this is prediction based on my recent 
experience with unstable, and hasn't been verified by testing.)

The problem actually turns out to be that if blktap is available, that 
libxl will choose it for cdroms, but the cd-insert and cd-eject stuff 
don't work with blktap.  I think I'm going to submit a patch for 4.3 
that prevents blktap from being used for cdroms -- you may want to wait 
and backport them both.

  -George

Re: BUG 4.2.2: xl cd-insert corrupts xenstore state

[Ian Jackson]

George Dunlap writes ("Re: BUG 4.2.2: xl cd-insert corrupts xenstore state"):
> The problem actually turns out to be that if blktap is available, that 
> libxl will choose it for cdroms, but the cd-insert and cd-eject stuff 
> don't work with blktap.  I think I'm going to submit a patch for 4.3 
> that prevents blktap from being used for cdroms -- you may want to wait 
> and backport them both.

Right, thanks.

Ian.

Re: BUG 4.2.2: xl cd-insert corrupts xenstore state

[Ian Jackson]

Ian Jackson writes ("Re: BUG 4.2.2: xl cd-insert corrupts xenstore state"):
> George Dunlap writes ("Re: BUG 4.2.2: xl cd-insert corrupts xenstore state"):
> > Turns out I already fixed this once!
> > 
> > commit c3556e2a1aee3c9b7dda5d57e85e8867fff1b9da
> > Author: George Dunlap <george.dunlap@eu.citrix.com>
> > Commit: George Dunlap <george.dunlap@eu.citrix.com>
> 
> Here's the backport, which was nearly trivial.
> 
> I'll push this to 4.2 staging unless someone objevts.

I have now done this.  Sorry for the delay.

> commit 1eea2f30f79a1af817fe32c9e01673da1f31dd5f
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Wed May 1 16:56:54 2013 +0100
> 
>     libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data
>     
>     libxl_cdrom_insert was failing to initialize the backend type,
>     resulting in the wrong default backend.  The result was not only that
>     the CD was not inserted properly, but also that some improper xenstore
>     entries were created, causing further block commands to fail.
>     
>     This patch fixes the bug by setting the disk backend type based on the
>     type of the existing device.
>     
>     It also makes the system more robust by checking to see that it has
>     got a valid path before proceeding to write a partial xenstore entry.
>     
>     Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>     Acked-by: Ian Campbell <ian.campbell@citrix.com>
>     Committed-by: Ian Campbell <ian.campbell@citrix.com>
>     (cherry picked from commit c3556e2a1aee3c9b7dda5d57e85e8867fff1b9da)