Re: [Xen-devel] [PATCH v6] libxl: allow 'phy' backend to use empty files

[Alex Braunegg]

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



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v6] libxl: allow 'phy' backend to use empty files

[George Dunlap]

On Wed, Feb 17, 2016 at 5:20 PM, Roger Pau Monne  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é 
> Reported-by: Alex Braunegg 

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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v6] libxl: allow 'phy' backend to use empty files

[Roger Pau Monné]

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é 
>> Reported-by: Alex Braunegg 
> 
> 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.




___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v6] libxl: allow 'phy' backend to use empty files

[Ian Jackson]

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é 
> Reported-by: Alex Braunegg 

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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v6] libxl: allow 'phy' backend to use empty files

[Alex Braunegg]

Hi Roger,

I have tested and applied the patch locally, and can confirm that the issue 
reported is now resolved.

Best regards,

Alex

-Original Message-
From: Roger Pau Monne [mailto:roger@citrix.com] 
Sent: Thursday, 18 February 2016 4:21 AM
To: xen-de...@lists.xenproject.org
Cc: Roger Pau Monne; Ian Jackson; Ian Campbell; Wei Liu; Alex Braunegg
Subject: [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é 
Reported-by: Alex Braunegg 
---
Cc: Ian Jackson 
Cc: Ian Campbell 
Cc: Wei Liu 
Cc: Alex Braunegg 
---
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(+)

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);
-- 
2.5.4 (Apple Git-61)


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v6] libxl: allow 'phy' backend to use empty files

[Roger Pau Monne]

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é 
Reported-by: Alex Braunegg 
---
Cc: Ian Jackson 
Cc: Ian Campbell 
Cc: Wei Liu 
Cc: Alex Braunegg 
---
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(+)

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);
-- 
2.5.4 (Apple Git-61)


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel