Re: [Qemu-devel] [PATCH] qmp: add BLOCK_MEDIUM_EJECT event

[Kevin Wolf]

Am 26.01.2012 18:57, schrieb Luiz Capitulino:
> On Wed, 25 Jan 2012 10:42:04 -0200
> Luiz Capitulino <address@hidden> wrote:
> 
>> On Wed, 25 Jan 2012 09:41:20 +0100
>> Kevin Wolf <address@hidden> wrote:
>>
>>> Am 24.01.2012 21:03, schrieb Eric Blake:
>>>> On 01/24/2012 11:16 AM, Luiz Capitulino wrote:
>>>>> Libvirt wants to be notified when the guest ejects a medium, so that
>>>>> it can update its view of the guest.
>>>>>
>>>>> This code has been originally written by Daniel Berrange. It adds
>>>>> the event to IDE and SCSI emulation.
>>>>>
>>>>> Please, note that this only covers guest initiated ejects, that's,
>>>>> the QMP/HMP commands 'eject' and 'change' are not covered.
>>>
>>> What's the reason for this behaviour? It feels inconsistent.
>>
>> I don't think it's inconsistent because we're limiting it to guest initiated
>> actions. Also, the mngt app knows when it sends a 'eject' or 'change' 
>> command.
>> The exception is if it allows HMP to run in parallel with QMP, but I don't 
>> think
>> this is recommended (at least not for commands that change any VM state).
> 
> Let me elaborate more. We have two options:
> 
>  1. Emit the event for guest initiated ejects (this patch, although I think
>     the event should be renamed to GUEST_MEDIUM_EJECT)
> 
>  2. Emit the event for guest & QMP initiated ejects, that's, also emit the
>     event for the eject and change commands
> 
> The first thing to note is that, they're not mutually exclusive. If we do
> item 1 now, we can add 2 later (as a different event).
> 
> But my point is that doing 2 is problematic and we should avoid it. The 
> problem
> is that the semantics of eject and change are complex and/or buggy. And 
> something
> I've learned about confusing semantics in QMP is that, they will give you 
> headaches
> soon or later.

But I'm not really interested in the semantics of QMP commands, because
they are irrelevant for the events.

My view is that a device generates an event each time its try status
changes. If it opens the tray, it does so by calling bdrv_eject - an
obvious place to send an event. If it closes the tray or the eject
monitor command is used, we go through bdrv_dev_change_media_cb - not
quite as obvious, but I think this despite its name this is really a
"tray status changed externally" callback.

QMP commands may cause any of these actions to occur. But the event is
tied to the actions and not to the QMP commands that may or may not
cause them according to their confusing semantics.

> The main problem with eject is that it's inconsistent with the handling of
> the tray status. Try this:
> 
> 1. Eject with no medium inserted
> 
> (qemu) info block
> ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted]
> (qemu) eject ide1-cd0
> (qemu) info block
> ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted]
> (qemu) 
> 
> Conclusion: the tray didn't move, we shouldn't emit the event.

Fails before bdrv_close, so bdrv_dev_change_media_cb is never called.
Works as it is today.

> 
> 2. Eject with medium inserted
> 
> (qemu) info block
> ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok file=/tmp/vl.iKvBAF 
> backing_file=/mnt/fernando/isos/linux/Fedora-14-x86_64-DVD.iso ro=0 drv=qcow2 
> encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0
> (qemu) eject ide1-cd0
> (qemu) info block
> ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok [not inserted]
> (qemu) 
> 
> Conclusion: the tray opened and the media was purged. We should emit the 
> event.

bdrv_dev_change_media_cb is called, we can emit an event there.

> 
> 3. Eject with medium inserted and locked
> 
> (qemu) info block
> ide1-cd0: removable=1 locked=1 tray-open=0 io-status=ok file=/tmp/vl.2LHApn 
> backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 
> encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0
> (qemu) eject ide1-cd0
> Device 'ide1-cd0' is locked
> (qemu) info block
> ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok file=/tmp/vl.2LHApn 
> backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 
> encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0
> (qemu) 
> 
> Conclusion: eject returned an error, but a few seconds later the tray opened 
> and
>             the media wasn't purged. What happened here is that, the _guest_
>             opened the tray. The code in this patch would trigger the event, 
> but
>             we shouldn't emit it twice if we cover eject & change (ie. 
> special case)

bdrv_dev_change_media_cb is not called because media cannot be ejected
with a locked drive. Instead bdrv_dev_eject_request is called which
doesn't emit an event.

If the guest happens to initiate an eject itself after receiving the
eject request, it calls bdrv_eject, where we can emit an event.

If we had force=true in the initial eject command, bdrv_close is called,
which in turn goes through bdrv_dev_change_media_cb where an event is
emitted.

> 
> Each of the three cases has different semantics wrt to the tray. We could try
> to fix them, but this seems to require some surgery... Is it worth it?
> 
> Now, the change command. Basically, it opens the tray, purges the medium,
> inserts another medium and closes the tray. Should we generate the event 
> twice?

change is eject followed by bdrv_open. We don't teleport media into a
device with closed tray, so yes, this is an open event followed by a
close event.

> So, I'd just avoid all this and have only GUEST_MEDIUM_EJECT, which has clear
> semantics and satisfies libvirt needs...

I think it's rather complex semantics compared to doing it always: Two
more or less obvious places in block.c and no modification of the device
emulation needed. For me that's a clear winner.

Kevin