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

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 20.07.2011 18:23, schrieb Markus Armbruster:
>> Callees always return 0, except for FreeBSD's cdrom_eject(), which
>> returns -ENOTSUP when the device is in a terminally wedged state.
>> 
>> The only caller is bdrv_eject(), and it maps -ENOTSUP to 0 since
>> commit 4be9762a.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> What about failed ioctls? Currently we only print an error message but
> still return 0. Is this the right behaviour? Could callers make use of
> an error return here or would we end up like with bdrv_set_locked()
> where we can't really communicate the error to the guest?

Note that I'm not removing any error handling.  I don't add any either.
The series is long enough...

bdrv_eject()'s purpose is similar to bdrv_set_locked()'s: synchronize
the physical tray to the virtual one.  Unsurprisingly, our error
handling options are also similar.

All bdrv_eject() can do with errors from the BlockDriver method is pass
them on.

At this point in the series, bdrv_eject() still returns an error of its
own: -EBUSY when bs->locked.  28/55 moves that check into callers.

bdrv_eject()'s callers are device models: ide-cd and scsi-cd.  Each one
calls it in three 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 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.

  Does send the appropriate error when the virtual tray is locked.

  What sense to send when the ioctl() fails?

  How can the ioctl() fail?  When the tray is locked, but for that to
  happen some other process must have interfered with the lock (Bad
  process!  No treat for you!  Go away!).  We could synchronize the lock
  again right before the eject, but that merely shrinks the race's
  window.

I doubt adding the error handling is worth our while, but feel free to
send patches.