Luiz Capitulino <lcapitul...@redhat.com> writes:
> On Sat, 28 May 2011 09:58:24 +0200
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitul...@redhat.com> writes:
>>
>> > Conforms to the event specification defined in the
>> > QMP/qmp-events.txt file.
>>
>> I'd squash PATCH 2+3.
>
> I agree this would be more logical, but people have complained that it's hard
> to review new commands/events when its documentation is hidden or mixed with
> code somewhere in the series. I think that make sense.
>
> Maybe, it would be better to copy & paste the documentation in patch 0/0...
>
>> > Please, note the following details:
>> >
>> > o The event should be emitted only by devices which support the
>> > eject operation, which currently are: CDROMs (IDE and SCSI)
>> > and floppies
>> >
>> > o Human monitor commands "eject" and "change" also cause the
>> > event to be emitted
>> >
>> > o The event is only emitted when there's a tray transition from
>> > closed to opened. To implement this in the human monitor, we
>> > only emit the event if the device is removable and a media is
>> > present
>>
>> Rationale?
>
> This implementation covers the basic use case, which is to let clients know
> that an already inserted media has been ejected. I was under the assumption
> that this is the most important thing a client wants to know, as it will
> have to update its internal state and that only has to be done for the media
> it knows about (ie. the ones inserted by the client itself).
>
> But...
>
>> Wouldn't it be easier if we just report tray status change, regardless
>> of media?
>
> Yes, this seems to make sense.
>
>> > Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
>> > ---
>> > block.c | 12 ++++++++++++
>> > block.h | 1 +
>> > blockdev.c | 5 +++++
>> > monitor.c | 3 +++
>> > monitor.h | 1 +
>> > 5 files changed, 22 insertions(+), 0 deletions(-)
>> >
>>
>> Copied from PATCH 2 for review purposes:
>>
>> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
>> index 0ce5d4e..d53c129 100644
>> --- a/QMP/qmp-events.txt
>> +++ b/QMP/qmp-events.txt
>> @@ -1,6 +1,24 @@
>> QEMU Monitor Protocol Events
>> ============================
>>
>> +BLOCK_MEDIA_EJECT
>> +-----------------
>> +
>> +Emitted when a removable disk media (such as a CDROM or floppy) is
>> ejected.
>> +
>> +Data:
>> +
>> +- "device": device name (json-string)
>> +
>> +Example:
>> +
>> +{ "event": "BLOCK_MEDIA_EJECT",
>> + "data": { "device": "ide1-cd0" },
>> + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>> +
>> +NOTE: A disk media can be ejected by the guest or by monitor commands
>> (such
>> +as "eject" and "change")
>> +
>> BLOCK_IO_ERROR
>> --------------
>>
>> The event reports "tray opening". Do we need one for "tray closing" as
>> well?
>
> At first I thought it wouldn't be needed (and maybe most use cases don't
> require it), but reporting the tray status is more general and easier to do,
> so I think it's what I'm going to do.
>
>> > diff --git a/block.c b/block.c
>> > index f5014cf..dbe813b 100644
>> > --- a/block.c
>> > +++ b/block.c
>> > @@ -1656,6 +1656,15 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t
>> > sector_num, int nb_sectors,
>> > return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
>> > }
>> >
>> > +void bdrv_eject_mon_event(const BlockDriverState *bdrv)
>> > +{
>> > + QObject *data;
>> > +
>> > + data = qobject_from_jsonf("{ 'device': %s }", bdrv->device_name);
>> > + monitor_protocol_event(QEVENT_BLOCK_MEDIA_EJECT, data);
>> > + qobject_decref(data);
>> > +}
>> > +
>> > void bdrv_error_mon_event(const BlockDriverState *bdrv,
>> > BlockMonEventAction action, int is_read)
>> > {
>> > @@ -2770,6 +2779,9 @@ int bdrv_eject(BlockDriverState *bs, int eject_flag)
>> > ret = 0;
>> > }
>> > if (ret >= 0) {
>> > + if (eject_flag && !bs->tray_open) {
>> > + bdrv_eject_mon_event(bs);
>> > + }
>> > bs->tray_open = eject_flag;
>> > }
>> >
>>
>> This covers guest-initiated eject.
>>
>>
>> The event is suppressed when the tray is already open.
>
> Yes, because there's no visible state change.
No objection to that, as long as it's consistently done.
>> The event is not suppressed when the tray is empty, is it? Contradicts
>> spec.
>
> Why does it contradicts the spec? No media is ejected if the tray is empty.
Yes. Nevertheless, the event is triggered.
> But this is probably not going to be an issue when we implement the more
> general BLOCK_TRAY_STATUS (or something like it).
No need to debate the fine points of "media eject" then.
>> > diff --git a/block.h b/block.h
>> > index 1f58eab..e4053dd 100644
>> > --- a/block.h
>> > +++ b/block.h
>> > @@ -50,6 +50,7 @@ typedef enum {
>> > BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>> > } BlockMonEventAction;
>> >
>> > +void bdrv_eject_mon_event(const BlockDriverState *bdrv);
>> > void bdrv_error_mon_event(const BlockDriverState *bdrv,
>> > BlockMonEventAction action, int is_read);
>> > void bdrv_info_print(Monitor *mon, const QObject *data);
>> > diff --git a/blockdev.c b/blockdev.c
>> > index 6e0eb83..5fd0043 100644
>> > --- a/blockdev.c
>> > +++ b/blockdev.c
>> > @@ -661,6 +661,11 @@ static int eject_device(Monitor *mon,
>> > BlockDriverState *bs, int force)
>> > return -1;
>> > }
>> > }
>> > +
>> > + if (bdrv_is_removable(bs) && bdrv_is_inserted(bs)) {
>> > + bdrv_eject_mon_event(bs);
>> > + }
>> > +
>> > bdrv_close(bs);
>> > return 0;
>> > }
>>
>> This covers monitor-initiated eject (commands eject and change).
>>
>> The event is not suppressed when the tray is already open (previous
>> guest-initiated eject), is it?. Contradicts spec.
>
> That's a bug.
>
>> The event is suppressed when the tray is empty.
>>
>> "eject -f" on a non-removable drive does not trigger an event. Why
>> treat it specially? I'm not saying you shouldn't, just wondering.
>
> Ejecting a non-removable drive is a qemu bug.
It's clearly intentional, so it's a (mis-)feature, not a bug.
[...]
