Re: [Qemu-devel] [PATCH 3/3] QMP: Introduce the BLOCK_MEDIA_EJECT event

[Luiz Capitulino]

On Tue, 31 May 2011 10:12:17 +0200
Kevin Wolf <address@hidden> wrote:

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