Re: [Xen-devel] [PATCH v4 4/4] libxl: fix cd-eject
Roger Pau Monné writes ("Re: [PATCH v4 4/4] libxl: fix cd-eject"):
> Should we allow the PHY backend to handle empty files?
> (pdev_path == NULL || pdev_path == "").
Yes. That is how it is supposed to work.
I think this was broken in in 97ee1f5d "libxl: add support for image
files for NetBSD" in 2011 (!) where this happened:
-if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY &&
-!S_ISBLK(a->stab.st_mode)) {
-LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
- " unsuitable as phys path not a block device",
- a->disk->vdev);
-return 0;
-}
-return backend;
+if (libxl__try_phy_backend(a->stab.st_mode))
+return backend;
... [libxl_linux.c:] ...
+int libxl__try_phy_backend(mode_t st_mode)
+{
+if (!S_ISBLK(st_mode)) {
+return 0;
+}
+
+return 1;
+}
Note that the "a->disk->format != LIBXL_DISK_FORMAT_EMPTY" clause has
been lost.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 4/4] libxl: fix cd-eject
On Wed, 2016-02-17 at 12:20 +0100, Roger Pau Monné wrote:
> El 16/2/16 a les 18:58, Ian Jackson ha escrit:
> > Roger Pau Monne writes ("[PATCH v4 4/4] libxl: fix cd-eject"):
> > > Current libxl__device_disk_set_backend implementation tried to guess
> > > the
> > > backend of devices with format LIBXL_DISK_FORMAT_EMPTY, which is of
> > > course
> > > doomed to fail since the disk is empty. Instead just return early
> > > from the
> > > function if an empty disk is detected.
> > >
> > > This fixes cd ejection.
> >
> > DYK when this was broken ? Or, to put it another way, how did this
> > ever work ?
> >
> > ...looking at the code...
> >
> > AFAICT disk_try_backend should succeed for both LIBXL_DISK_BACKEND_PHY
> > and LIBXL_DISK_BACKEND_QDISK. So even before your patch:
> >
> > > }
> > > -memset(&a.stab, 0, sizeof(a.stab));
> > > +/* Disk is empty, so it's useless to try to guess the
> > > backend type. */
> > > +return 0;
> > > } else if ((disk->backend == LIBXL_DISK_BACKEND_UNKNOWN ||
> >
> > libxl__device_disk_set_backend should work.
>
> I've been looking at the code and I cannot really understand how this is
> supposed to work before, none of the recent changes seem to have broken
> it AFAICT, and the issue has been there for a long time (~1year), which
> IMHO makes no sense, so I'm quite sure I'm missing something.
cd-insert/eject seems to me to either be perpetually broken or is
incredibly prone to regressing (i..e this keeps coming up).
Getting a test step into osstest which used cd-insert/eject would be a good
idea IMHO, either a deliberate test step which checks it works or trying to
use it as a matter of course during e.g. guest install.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 4/4] libxl: fix cd-eject
El 16/2/16 a les 18:58, Ian Jackson ha escrit:
> Roger Pau Monne writes ("[PATCH v4 4/4] libxl: fix cd-eject"):
>> Current libxl__device_disk_set_backend implementation tried to guess the
>> backend of devices with format LIBXL_DISK_FORMAT_EMPTY, which is of course
>> doomed to fail since the disk is empty. Instead just return early from the
>> function if an empty disk is detected.
>>
>> This fixes cd ejection.
>
> DYK when this was broken ? Or, to put it another way, how did this
> ever work ?
>
> ...looking at the code...
>
> AFAICT disk_try_backend should succeed for both LIBXL_DISK_BACKEND_PHY
> and LIBXL_DISK_BACKEND_QDISK. So even before your patch:
>
>> }
>> -memset(&a.stab, 0, sizeof(a.stab));
>> +/* Disk is empty, so it's useless to try to guess the backend type.
>> */
>> +return 0;
>> } else if ((disk->backend == LIBXL_DISK_BACKEND_UNKNOWN ||
>
> libxl__device_disk_set_backend should work.
I've been looking at the code and I cannot really understand how this is
supposed to work before, none of the recent changes seem to have broken
it AFAICT, and the issue has been there for a long time (~1year), which
IMHO makes no sense, so I'm quite sure I'm missing something.
The problem is that in libxl_cdrom_insert the backend of the input
"disk" struct is set based on the backend that the disk with the same
vdev is using (see libxl.c:~2910). So if you have a CD plugged in with a
PHY backend, the backend of the input disk will also be set to PHY. Then
when you try to unplug it, the backend of the empty disk will also be
set to PHY, and disk_try_backend will fail miserably. In this case since
the backend is set _before_ calling libxl__device_disk_set_backend no
other backend is tested, and an error is returned.
I guess that all this steams from when we switched from handling RAW
files from QDISK to blkback (PHY). That was quite some time ago IIRC,
and I found it weird that nobody noticed this before. Prior to this
change the backend used for CDROM would be QDISK, which should make
everything work as expected (I've locally reverted a0a2dc and it solves
the issue).
Should I just force all devices of type CDROM to use QDISK as the
backend? Should we allow the PHY backend to handle empty files?
(pdev_path == NULL || pdev_path == "").
Roger.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 4/4] libxl: fix cd-eject
Roger Pau Monne writes ("[PATCH v4 4/4] libxl: fix cd-eject"):
> Current libxl__device_disk_set_backend implementation tried to guess the
> backend of devices with format LIBXL_DISK_FORMAT_EMPTY, which is of course
> doomed to fail since the disk is empty. Instead just return early from the
> function if an empty disk is detected.
>
> This fixes cd ejection.
DYK when this was broken ? Or, to put it another way, how did this
ever work ?
...looking at the code...
AFAICT disk_try_backend should succeed for both LIBXL_DISK_BACKEND_PHY
and LIBXL_DISK_BACKEND_QDISK. So even before your patch:
> }
> -memset(&a.stab, 0, sizeof(a.stab));
> +/* Disk is empty, so it's useless to try to guess the backend type.
> */
> +return 0;
> } else if ((disk->backend == LIBXL_DISK_BACKEND_UNKNOWN ||
libxl__device_disk_set_backend should work.
Worse, this change seems to leave disk->backend unset on return from
libxl__device_disk_set_backend, which seems quite wrong to me.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 4/4] libxl: fix cd-eject
Current libxl__device_disk_set_backend implementation tried to guess the
backend of devices with format LIBXL_DISK_FORMAT_EMPTY, which is of course
doomed to fail since the disk is empty. Instead just return early from the
function if an empty disk is detected.
This fixes cd ejection.
Signed-off-by: Roger Pau Monné
Reported-by: Alex Braunegg
---
Cc: Ian Jackson
Cc: Ian Campbell
Cc: Wei Liu
Cc: Alex Braunegg
---
tools/libxl/libxl_device.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 8bb5e93..b93cbbf 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -273,7 +273,8 @@ int libxl__device_disk_set_backend(libxl__gc *gc,
libxl_device_disk *disk) {
LOG(ERROR, "Disk vdev=%s is empty but not cdrom", disk->vdev);
return ERROR_INVAL;
}
-memset(&a.stab, 0, sizeof(a.stab));
+/* Disk is empty, so it's useless to try to guess the backend type. */
+return 0;
} else if ((disk->backend == LIBXL_DISK_BACKEND_UNKNOWN ||
disk->backend == LIBXL_DISK_BACKEND_PHY) &&
disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
--
2.5.4 (Apple Git-61)
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
