| Message ID | 1455729638-41937-1-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Roger Pau Monne writes ("[PATCH v6] libxl: allow 'phy' backend to use empty files"):
> This was introduced by 97ee1f (~5 years ago), but was probably never
> surfaced because most people used regular files as CDROM images, so the PHY
> backend was actually never selected. A year ago this was changed, and now
> regular RAW files are also handled by the PHY backend, which has made this
> bug surface.
>
> Fix it by allowing empty disks to use the PHY backend, skipping the stat
> tests.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reported-by: Alex Braunegg <alex.braunegg@gmail.com>
Thanks, and thanks to Alex for the testing, but:
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 8bb5e93..2e08108 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -196,6 +196,14 @@ static int disk_try_backend(disk_try_backend_args *a,
> goto bad_format;
> }
>
> + if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY) {
> + assert(a->disk->pdev_path == NULL ||
> + !strcmp(a->disk->pdev_path, ""));
I agree that these things ought to be true but I don't see what code
in libxl ensures that they definitely are.
I think this check ought to be moved to libxl__device_disk_set_backend
or perhaps even earlier, and should generate an ERROR_INVAL rather
than an assertion failure.
The rest of the logic LGTM.
Thanks,
Ian.
El 19/2/16 a les 18:30, Ian Jackson ha escrit: > Roger Pau Monne writes ("[PATCH v6] libxl: allow 'phy' backend to use empty files"): >> This was introduced by 97ee1f (~5 years ago), but was probably never >> surfaced because most people used regular files as CDROM images, so the PHY >> backend was actually never selected. A year ago this was changed, and now >> regular RAW files are also handled by the PHY backend, which has made this >> bug surface. >> >> Fix it by allowing empty disks to use the PHY backend, skipping the stat >> tests. >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> Reported-by: Alex Braunegg <alex.braunegg@gmail.com> > > Thanks, and thanks to Alex for the testing, but: > >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c >> index 8bb5e93..2e08108 100644 >> --- a/tools/libxl/libxl_device.c >> +++ b/tools/libxl/libxl_device.c >> @@ -196,6 +196,14 @@ static int disk_try_backend(disk_try_backend_args *a, >> goto bad_format; >> } >> >> + if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY) { >> + assert(a->disk->pdev_path == NULL || >> + !strcmp(a->disk->pdev_path, "")); > > I agree that these things ought to be true but I don't see what code > in libxl ensures that they definitely are. > > I think this check ought to be moved to libxl__device_disk_set_backend, > or perhaps even earlier, and should generate an ERROR_INVAL rather > than an assertion failure. Thanks, libxl can return EINVAL without problems from libxl__device_disk_set_backend, so I think it's a fine place to put this check. libxl__device_disk_setdefault should also be a good place, but I feel _backend is where it makes more sense. v7 is on the way. Roger.
On Wed, Feb 17, 2016 at 5:20 PM, Roger Pau Monne <roger.pau@citrix.com> wrote: > This was introduced by 97ee1f (~5 years ago), but was probably never > surfaced because most people used regular files as CDROM images, so the PHY > backend was actually never selected. A year ago this was changed, and now > regular RAW files are also handled by the PHY backend, which has made this > bug surface. > > Fix it by allowing empty disks to use the PHY backend, skipping the stat > tests. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Reported-by: Alex Braunegg <alex.braunegg@gmail.com> So first of all, it's not 100% clear from this commit what the problem was that Alex reported. I take it that if he used a phy backend for a cdrom, that "xl cd-eject" failed? Unfortunately, after this changeset, creating a VM with an empty cdrom fails, because it tries to run the block script and the block script fails: # cat c6-01.cfg builder="hvm" name = "c6-01" memory = "2048" disk = [ 'format=raw,vdev=sda,target=/images/c6-01.raw','vdev=hdc,access=ro,devtype=cdrom' ] vif = [ 'mac=5a:39:bb:b6:84:b4' ] vcpus=1 on_crash = 'destroy' serial='pty' # xl create c6-01.cfg libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: /etc/xen/scripts/block add [7236] exited with error status 1 libxl: error: libxl_device.c:1114:device_hotplug_child_death_cb: script: /etc/xen/scripts/block failed; error detected. libxl: error: libxl_create.c:1247:domcreate_launch_dm: unable to add disk devices libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: /etc/xen/scripts/block remove [7319] exited with error status 1 libxl: error: libxl_device.c:1114:device_hotplug_child_death_cb: script: /etc/xen/scripts/block failed; error detected. libxl: error: libxl.c:1565:libxl__destroy_domid: non-existant domain 5 libxl: error: libxl.c:1523:domain_destroy_callback: unable to destroy guest with domid 5 libxl: error: libxl.c:1452:domain_destroy_cb: destruction of domain 5 failed -George
Hi George,
> I take it that if he used a phy backend for a cdrom, that "xl cd-eject" failed?
No - I was always using ISO images for cd-rom devices. In the 4.4 configuration I was specifying it as a file.
In Xen 4.4 and 4.6 (before patching) whenever I attempted to perform an 'xl cd-eject' it always failed. I didn’t perform the triage of what commit broke the functionality - so I cant advise on if this is what broke it.
A sample of the configurations of what I used is below:
================
# Xen 4.4
builder='hvm'
memory = 512
name = 'CentOS_Test'
disk = [ 'phy:/dev/zvol/storage/xen/CentOS_Test/disk_sda,hda,w','file:/storage/samba/xeniso/CentOS-6.5-x86_64-minimal.iso,hdc:cdrom,r' ]
# boot on floppy (a), hard disk (c) or CD-ROM (d)
# default: hard disk, cd-rom, floppy
boot='cd'
# Xen 4.6
builder='hvm'
memory = 512
name = 'CentOS_Test'
disk = [ '/dev/zvol/storage/xen/CentOS_Test/disk_sda,,hda','/storage/samba/xeniso/CentOS-6.5-x86_64-minimal.iso,,hdc,cdrom' ]
# boot on floppy (a), hard disk (c) or CD-ROM (d)
# default: hard disk, cd-rom, floppy
boot='cd'
================
After patching Xen 4.6, I can now perform the xl cd-eject and load an alternate ISO into the VM without issue. However if I just want to eject the ISO as per what you would normally do on a physical system after install, I have to 'eject' but then insert a 'dummy' ISO file to keep the cd-rom device available to the VM through reboots / shutdowns:
disk = [ '/dev/zvol/storage/xen/CentOS_Test/disk_sda,,hda','/storage/samba/xeniso/dummy.iso,,hdc,cdrom' ]
Without having some sort of valid 'dummy' ISO file that contains nothing, Xen has issues about creating the cd-rom device on creation and reboots, and certainly in the VM no cd-rom device is then available - meaning down the track if I want to load an ISO I cannot easily 'insert' another ISO file without powering off the VM, making the configuration change and powering the VM back on again.
It would be nice at some point to have the configuration where the cd-rom drive can presented to the VM without having a valid ISO / file loaded - which would then operate as per physical devices - cd-rom devices show up, but drive contents are empty:
disk = [ '/dev/zvol/storage/xen/CentOS_Test/disk_sda,,hda',',,hdc,cdrom' ]
Best regards,
Alex
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c index 8bb5e93..2e08108 100644 --- a/tools/libxl/libxl_device.c +++ b/tools/libxl/libxl_device.c @@ -196,6 +196,14 @@ static int disk_try_backend(disk_try_backend_args *a, goto bad_format; } + if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY) { + assert(a->disk->pdev_path == NULL || + !strcmp(a->disk->pdev_path, "")); + LOG(DEBUG, "Disk vdev=%s is empty, skipping physical device check", + a->disk->vdev); + return backend; + } + if (a->disk->backend_domid != LIBXL_TOOLSTACK_DOMID) { LOG(DEBUG, "Disk vdev=%s, is using a storage driver domain, " "skipping physical device check", a->disk->vdev);
This was introduced by 97ee1f (~5 years ago), but was probably never surfaced because most people used regular files as CDROM images, so the PHY backend was actually never selected. A year ago this was changed, and now regular RAW files are also handled by the PHY backend, which has made this bug surface. Fix it by allowing empty disks to use the PHY backend, skipping the stat tests. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reported-by: Alex Braunegg <alex.braunegg@gmail.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Ian Campbell <ian.campbell@citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Alex Braunegg <alex.braunegg@gmail.com> --- Changes since v4: - Split form the rest of the series. - Fix disk_try_backend. --- tools/libxl/libxl_device.c | 8 ++++++++ 1 file changed, 8 insertions(+)