Patchwork [3/3] QMP: Introduce the BLOCK_MEDIA_EJECT event

login
register
mail settings
Submitter Luiz Capitulino
Date May 27, 2011, 7:31 p.m.
Message ID <1306524712-13050-4-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/97743/
State New
Headers show

Comments

Luiz Capitulino - May 27, 2011, 7:31 p.m.
Conforms to the event specification defined in the
QMP/qmp-events.txt file.

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

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c    |   12 ++++++++++++
 block.h    |    1 +
 blockdev.c |    5 +++++
 monitor.c  |    3 +++
 monitor.h  |    1 +
 5 files changed, 22 insertions(+), 0 deletions(-)
Markus Armbruster - May 28, 2011, 7:58 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> Conforms to the event specification defined in the
> QMP/qmp-events.txt file.

I'd squash PATCH 2+3.

>
> 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?

Wouldn't it be easier if we just report tray status change, regardless
of media?

>
> Signed-off-by: Luiz Capitulino <lcapitulino@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?

> 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.

The event is not suppressed when the tray is empty, is it?  Contradicts
spec.

> 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.

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.

> diff --git a/monitor.c b/monitor.c
> index f63cce0..4a81467 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -450,6 +450,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>          case QEVENT_VNC_DISCONNECTED:
>              event_name = "VNC_DISCONNECTED";
>              break;
> +        case QEVENT_BLOCK_MEDIA_EJECT:
> +            event_name = "BLOCK_MEDIA_EJECT";
> +            break;
>          case QEVENT_BLOCK_IO_ERROR:
>              event_name = "BLOCK_IO_ERROR";
>              break;
> diff --git a/monitor.h b/monitor.h
> index 4f2d328..7a04137 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -29,6 +29,7 @@ typedef enum MonitorEvent {
>      QEVENT_VNC_CONNECTED,
>      QEVENT_VNC_INITIALIZED,
>      QEVENT_VNC_DISCONNECTED,
> +    QEVENT_BLOCK_MEDIA_EJECT,
>      QEVENT_BLOCK_IO_ERROR,
>      QEVENT_RTC_CHANGE,
>      QEVENT_WATCHDOG,
Luiz Capitulino - May 30, 2011, 2:09 p.m.
On Sat, 28 May 2011 09:58:24 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@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 <lcapitulino@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.

> 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.
But this is probably not going to be an issue when we implement the more
general BLOCK_TRAY_STATUS (or something like it).

> > 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.

> > diff --git a/monitor.c b/monitor.c
> > index f63cce0..4a81467 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -450,6 +450,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
> >          case QEVENT_VNC_DISCONNECTED:
> >              event_name = "VNC_DISCONNECTED";
> >              break;
> > +        case QEVENT_BLOCK_MEDIA_EJECT:
> > +            event_name = "BLOCK_MEDIA_EJECT";
> > +            break;
> >          case QEVENT_BLOCK_IO_ERROR:
> >              event_name = "BLOCK_IO_ERROR";
> >              break;
> > diff --git a/monitor.h b/monitor.h
> > index 4f2d328..7a04137 100644
> > --- a/monitor.h
> > +++ b/monitor.h
> > @@ -29,6 +29,7 @@ typedef enum MonitorEvent {
> >      QEVENT_VNC_CONNECTED,
> >      QEVENT_VNC_INITIALIZED,
> >      QEVENT_VNC_DISCONNECTED,
> > +    QEVENT_BLOCK_MEDIA_EJECT,
> >      QEVENT_BLOCK_IO_ERROR,
> >      QEVENT_RTC_CHANGE,
> >      QEVENT_WATCHDOG,
>
Markus Armbruster - May 30, 2011, 2:49 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Sat, 28 May 2011 09:58:24 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@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 <lcapitulino@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.

[...]
Kevin Wolf - May 31, 2011, 8:12 a.m.
Am 30.05.2011 16:49, schrieb Markus Armbruster:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
>> On Sat, 28 May 2011 09:58:24 +0200
>> Markus Armbruster <armbru@redhat.com> wrote:
>>
>>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>>> 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.

Is there really a use case for it? The closest thing to a specification
that we have is the help text and it says:

        .help       = "eject a removable medium (use -f to force it)",

QMP describes it like this:

        Eject a removable medium.

So I start tending to agree that this whole trouble around the 'eject'
monitor command is in fact a long standing bug rather than overloaded
semantics. Nowhere is stated that it disconnects a BlockDriverState from
the image, and I can't imagine a use case for this semantics either.

Do we break anything if we make eject really eject the medium (we have a
virtual tray status now) instead of just closing the image? I think the
most visible change is that we'll eject the host medium when using
pass-through. I consider this an additional bugfix.

Kevin
Luiz Capitulino - May 31, 2011, 1:35 p.m.
On Tue, 31 May 2011 10:12:17 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 30.05.2011 16:49, schrieb Markus Armbruster:
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> > 
> >> On Sat, 28 May 2011 09:58:24 +0200
> >> Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >>> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >>>> 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.

Calling it "eject" is a bug, as it's not exactly what the command does.

> Is there really a use case for it? The closest thing to a specification
> that we have is the help text and it says:
> 
>         .help       = "eject a removable medium (use -f to force it)",
> 
> QMP describes it like this:
> 
>         Eject a removable medium.
> 
> So I start tending to agree that this whole trouble around the 'eject'
> monitor command is in fact a long standing bug rather than overloaded
> semantics. Nowhere is stated that it disconnects a BlockDriverState from
> the image, and I can't imagine a use case for this semantics either.

That's my thinking too.

> Do we break anything if we make eject really eject the medium (we have a
> virtual tray status now) instead of just closing the image?

I don't think so. I guess users/clients really have the expectation that
the only result is to get the media ejected.

Now, "-f" can be used with non-removable media. There's some risk of
breakage here if clients are using this to "unplug" devices. But I think
this a case where we'll have to pay the price for the breakage (if any).

> most visible change is that we'll eject the host medium when using
> pass-through. I consider this an additional bugfix.

Yes.
Anthony Liguori - May 31, 2011, 1:40 p.m.
On 05/31/2011 08:35 AM, Luiz Capitulino wrote:
> On Tue, 31 May 2011 10:12:17 +0200
>> Do we break anything if we make eject really eject the medium (we have a
>> virtual tray status now) instead of just closing the image?
>
> I don't think so. I guess users/clients really have the expectation that
> the only result is to get the media ejected.
>
> Now, "-f" can be used with non-removable media. There's some risk of
> breakage here if clients are using this to "unplug" devices. But I think
> this a case where we'll have to pay the price for the breakage (if any).

Only badness can ensue from doing that today so I see no reason to 
preserve this "functionality".

Regards,

Anthony Liguori

>
>> most visible change is that we'll eject the host medium when using
>> pass-through. I consider this an additional bugfix.
>
> Yes.
>

Patch

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;
     }
 
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;
 }
diff --git a/monitor.c b/monitor.c
index f63cce0..4a81467 100644
--- a/monitor.c
+++ b/monitor.c
@@ -450,6 +450,9 @@  void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_VNC_DISCONNECTED:
             event_name = "VNC_DISCONNECTED";
             break;
+        case QEVENT_BLOCK_MEDIA_EJECT:
+            event_name = "BLOCK_MEDIA_EJECT";
+            break;
         case QEVENT_BLOCK_IO_ERROR:
             event_name = "BLOCK_IO_ERROR";
             break;
diff --git a/monitor.h b/monitor.h
index 4f2d328..7a04137 100644
--- a/monitor.h
+++ b/monitor.h
@@ -29,6 +29,7 @@  typedef enum MonitorEvent {
     QEVENT_VNC_CONNECTED,
     QEVENT_VNC_INITIALIZED,
     QEVENT_VNC_DISCONNECTED,
+    QEVENT_BLOCK_MEDIA_EJECT,
     QEVENT_BLOCK_IO_ERROR,
     QEVENT_RTC_CHANGE,
     QEVENT_WATCHDOG,