Re: [Qemu-devel] [PATCH 07/55] block: Make BlockDriver method bdrv_set_locked() return void

[Kevin Wolf]

Am 21.07.2011 17:07, schrieb Markus Armbruster:
> Luiz Capitulino <address@hidden> writes:
> 
>> On Wed, 20 Jul 2011 18:23:41 +0200
>> Markus Armbruster <address@hidden> wrote:
>>
>>> The only caller is bdrv_set_locked(), and it ignores the value.
>>>
>>> Callees always return 0, except for FreeBSD's cdrom_set_locked(),
>>> which returns -ENOTSUP when the device is in a terminally wedged
>>> state.
>>
>> The fact that we're ignoring ioctl() failures caught my attention. Is it
>> because the state we store in bs->locked is what really matters?
>>
>> Otherwise the right thing to do would be to propagate the error and
>> change callers to handle it.
> 
> The only caller is bdrv_set_locked().  Which can't handle it, so it
> would have to pass it on.
> 
> The purpose of bdrv_set_locked() is to synchronize the physical lock (if
> any) with the virtual lock.  Worst that can happen is the physical lock
> fails to track the virtual one.
> 
> bdrv_set_locked()'s callers are device models: ide-cd and scsi-cd.  Each
> one calls it in four places (with all my patches applied):
> 
> * Device init: can return failure.  Failure makes QEMU refuse to start
>   (-device cold plug), or hot plug fail (device_add).
> 
> * Device exit: can't return failure.
> 
> * Device post migration: can return failure, makes migration fail.
>   Given the choice, I figure I'd pick an incorrect physical tray lock
>   over a failed migration.
> 
> * Guest PREVENT ALLOW MEDIUM REMOVAL: can send error reply to guest.
>   Recommended errors according to MMC-5 are "unit attention errors" (not
>   a good match), "CDB or parameter list validation errors" (worse) and
>   "hardware failures" (no good choices there either).
> 
> I doubt adding the error handling is worth our while, but feel free to
> send patches.

I agree with Luiz that it feels a bit strange to remove one of the rare
places in qemu that actually return errors, but if you say that the
callers can't handle it anyway, well... We can always add it back if we
find a device that could make use of it.

Kevin