Re: [PATCH v4 4/4] libxl: fix cd-eject

["Roger Pau Monné" <roger.pau@citrix.com>]

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.