| Message ID | 1455644269-40358-5-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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.
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.
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 &&
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é <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> --- tools/libxl/libxl_device.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)