Kevin Wolf <kw...@redhat.com> writes:
> Am 16.02.2012 14:14, schrieb Luiz Capitulino:
>> On Thu, 16 Feb 2012 10:25:43 +0100
>> Markus Armbruster <arm...@redhat.com> wrote:
>>
>>> Luiz Capitulino <lcapitul...@redhat.com> writes:
>>>
>>>> It's emitted whenever the tray is moved by the guest or by HMP/QMP
>>>> commands.
>>>
>>> I like the simplicity of this patch. A few remarks inline.
>>>
>>>> Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
>>>> ---
>>>> QMP/qmp-events.txt | 17 +++++++++++++++++
>>>> block.c | 24 ++++++++++++++++++++++++
>>>> monitor.c | 3 +++
>>>> monitor.h | 1 +
>>>> 4 files changed, 45 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
>>>> index 06cb404..c52e7fe 100644
>>>> --- a/QMP/qmp-events.txt
>>>> +++ b/QMP/qmp-events.txt
>>>> @@ -26,6 +26,23 @@ Example:
>>>> Note: If action is "stop", a STOP event will eventually follow the
>>>> BLOCK_IO_ERROR event.
>>>>
>>>> +BLOCK_MEDIUM_EJECT
>>>> +------------------
>>>> +
>>>> +It's emitted whenever the tray is moved by the guest or by HMP/QMP
>>>> +commands.
>>>> +
>>>> +Data:
>>>> +
>>>> +- "device": device name (json-string)
>>>> +- "ejected": true if the tray has been opened or false if it has been
>>>> closed
>>>
>>> I'd make this "load", because I find "true to load, false to eject" more
>>> pleasing, but that's strictly a matter of taste.
>>
>> I also think that in the end it's just matter of taste.
>>
>> I don't like "load" because (at least in my mind) it suggests a medium has
>> been
>> loaded and that might not be true.
>>
>> Another good option (and now I think I'll change to it) is "tray-open". That
>> matches with query-block's output and I think the meaning is more direct.
>> I chose "ejected" because that matches with the well-known eject program.
>
> I like tray-open.
>
>>>> +
>>>> +{ "event": "BLOCK_MEDIUM_EJECT",
>>>> + "data": { "device": "ide1-cd0",
>>>> + "ejected": true
>>>> + },
>>>> + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>>> +
>>>> RESET
>>>> -----
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 47f1823..e5e2a5f 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -970,10 +970,30 @@ void bdrv_emit_qmp_error_event(const
>>>> BlockDriverState *bdrv,
>>>> qobject_decref(data);
>>>> }
>>>>
>>>> +static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool ejected)
>>>> +{
>>>> + QObject *data;
>>>> +
>>>> + data = qobject_from_jsonf("{ 'device': %s, 'ejected': %i }",
>>>> + bdrv_get_device_name(bs), ejected);
>>>> + monitor_protocol_event(QEVENT_BLOCK_MEDIUM_EJECT, data);
>>>> +
>>>> + qobject_decref(data);
>>>> +}
>>>> +
>>>> static void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load)
>>>> {
>>>> if (bs->dev_ops && bs->dev_ops->change_media_cb) {
>>>> + bool tray_was_closed = !bdrv_dev_is_tray_open(bs);
>>>> bs->dev_ops->change_media_cb(bs->dev_opaque, load);
>>>> + if (tray_was_closed) {
>>>> + /* tray open */
>>>> + bdrv_emit_qmp_eject_event(bs, true);
>>>
>>> Emits event even when tray stays closed (tray_was_closed && !load),
>>> doesn't it?
>>
>> Yes, but that's on purpose. There are two approaches here:
>>
>> 1. Emit the event as the operations would be carried on real hw. On real
>> hw, your example would be the equivalent of: open the tray, remove the
>> medium if any, close the tray. This patch would report the opening and
>> closing of the tray
>>
>> 2. Emit the event only if the final state of the tray is different from
>> the previous state. In this case, we wouldn't emit any event when
>> change is issued on a closed tray
>>
>> I agree that item 2 matches better with the description "the event is emitted
>> whenever the tray moves", but then the change command would only emit the
>> event
>> when the tray is already open (ie. it will emit the event for tray close).
>
> What we want to have is the semantics of 1. which is how I understand
> "tray has moved". We don't observe the state before a 'change' monitor
> command and after it and emit an event if tray status isn't the same any
> more, but we actually observe the tray moves in the single steps that
> are done for implementing the monitor command.
Agreed.
> If you need magic like if (tray_was_closed) in bdrv_dev_change_media_cb,
> this seems to indicate that we're not doing things completely right in
> the internal model.
Agreed again.
> We're probably taking shortcuts so that this
> function really sees a closed -> closed transition when we really have
> closed -> open -> closed.
Let's figure out how this stuff really works.
1. Guest load/eject
Guest commands load or eject. Device model updates its state of virtual
tray (e.g. IDEState member tray_open) unless locked, then calls
bdrv_eject().
bdrv_eject() is a no-op except for pass-through backends such as
host_cdrom.
Note: device models call bdrv_eject() whether the state changed or not.
The only use for that I can see is syncing a wayward physical tray to
the virtual one. Shouldn't be necessary with Paolo's recent work,
should it?
Luiz's patch adds event emission to bdrv_eject(). Makes sense, except
when it's called even though the tray didn't move. Patch adds a
parameter to suppress the event then. I'd prefer to suppress the call
entirely then, if that's possible.
2. Monitor commands
2a. eject
run bdrv_close() if eject is permitted right now (not in use by
long-running block operations such as block migration and image
streaming, and force or not locked).
bdrv_close(bs) calls bdrv_dev_change_media_cb(bs, false).
bdrv_dev_change_media_cb() tells the device model the media went away.
Device model responds by opening its virtual tray, unless it's already
open.
Luiz's patch adds event emission to bdrv_dev_change_media_cb(). It
emits an "open" event when the tray is closed before telling the device
model. We assume the device model always opens the tray then, so this
emits the event only when the tray actually moves. A bit subtle, but
works.
Wart: bdrv_close(bs) is a no-op if bs isn't open. "bs not open"
represents "no media". Therefore, eject without media does nothing. In
particular, the virtual tray doesn't move, and no event is emitted.
Works.
2b. change
First do 2a. eject. If we had media, the virtual tray is now open.
Else, it could be open or closed, because of the wart.
Then open a new block backend. bdrv_dev_change_media_cb(bs, true) gets
called either right away by bdrv_open(), or later by bdrv_set_key().
bdrv_dev_change_media_cb() tells the device model new media appeared.
Device model responds by closing its virtual tray, unless it's already
closed[*].
Luiz's patch adds event emission to bdrv_dev_change_media_cb(). It
emits an "open" event when the tray is closed before telling the device
model, and a "closed" event unconditionally. As above, subtle, but
works,
3. Physical load/eject with pass-through device
The host_cdrom backend's tray handling is rudimentary. But let me
sketch how a real implementation could work.
3a. User closes tray, or opens unlocked tray
Backend gets notified, calls into block layer. Block layer notifies
device model. Device model notifies guest OS.
3b. User presses button while tray is closed and locked
Backend gets notified, calls into block layer. Block layer notifies
device model. Device model notifies guest OS. Guest OS may command
tray open. Goto 1.
We should be able to emit suitable events in the block layer.
> Depending on how hard it would be to fix I would suggest that either you
> fix the internal model, or that we check that the externally visible
> behaviour is the same as if we did it right and postpone the fix for the
> internal model.
Your suggestion makes sense to me.
I figure Luiz's patch works. But maybe it could be simplified some by
replacing bdrv_dev_change_media_cb() by a "open/close tray" callback
that returns whether it moved. bdrv_dev_change_media_cb() would then
simply open the tray, emit event if it moved, close the tray, emit event
if it moved.
I believe that should also do fine for my case 3.
[*] Can happen only in the warty case. Or when the guest closes the
tray before us. I'm not sure that works. The device model sees media
(because bs is open) even though we don't have the key, yet. No idea
what happens when it reads or writes it.
