Re: [Qemu-devel] [PATCH v7 32/39] blockdev: Implement eject with basic operations

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]

On 23.10.2015 15:54, Kevin Wolf wrote:
> Am 19.10.2015 um 17:53 hat Max Reitz geschrieben:
>> Implement 'eject' by calling blockdev-open-tray and
>> blockdev-remove-medium.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  blockdev.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index a4c278f..0481686 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1941,16 +1941,15 @@ out:
>>  
>>  void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
>>  {
>> -    BlockBackend *blk;
>> +    Error *local_err = NULL;
>>  
>> -    blk = blk_by_name(device);
>> -    if (!blk) {
>> -        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>> -                  "Device '%s' not found", device);
>> +    qmp_blockdev_open_tray(device, has_force, force, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>>          return;
>>      }
> 
> This changes the behaviour, in the current patch series in two ways if
> the device is locked:
> 
> 1. With force, the qmp_blockdev_remove_medium() call will fail because
>    we only unlocked the device, but didn't actually open the tray (I
>    commented on this in the qmp_blockdev_open_tray() patch). This breaks
>    the API, obviously.

Yep, will fix.

> 2. Without force, eject previously sent an eject request and also
>    returned a "Device is locked" error. Now it fails with "Tray of
>    device is not open". Regression in the message quality, but not an
>    API breakage because tools must not parse the message.

I think this should be fine. The previous message wasn't too good in my
opinion either, because the most likely scenario is this: User issues
eject, management tool reports qemu's message: "Device is locked!" and
then the tray opens. So that's strange, too. Maybe "Tray of device is
not open" is actually the better message here, I don't know. It better
describes the state, but it does not describe the reason...

But in addition to that, there is no easy way around this. I could put a
check into qmp_eject() which returns a "Device is locked" message if the
tray is still closed after a successful qmp_blockdev_open_tray(), but I
don't know whether that's worth it.

Max

>> -    eject_device(blk, force, errp);
>> +    qmp_blockdev_remove_medium(device, errp);
>>  }
> 
> Kevin
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]