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

[Luiz Capitulino]

On Wed, 25 Jan 2012 11:19:59 +0100
Markus Armbruster <address@hidden> wrote:

> Luiz Capitulino <address@hidden> writes:
> 
> > 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.
> >
> > Signed-off-by: Luiz Capitulino <address@hidden>
> > ---
> >
> > PS: Bugs are mine.
> >
> >  QMP/qmp-events.txt |   18 ++++++++++++++++++
> >  block.c            |   12 ++++++++++++
> >  block.h            |    5 +++++
> >  block_int.h        |    3 +++
> >  blockdev.c         |   13 +++++++++++++
> >  hw/ide/atapi.c     |    1 +
> >  hw/scsi-disk.c     |    1 +
> >  monitor.c          |    3 +++
> >  monitor.h          |    1 +
> >  9 files changed, 57 insertions(+), 0 deletions(-)
> >
> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > index af586ec..e414128 100644
> > --- a/QMP/qmp-events.txt
> > +++ b/QMP/qmp-events.txt
> > @@ -26,6 +26,24 @@ Example:
> >  Note: If action is "stop", a STOP event will eventually follow the
> >  BLOCK_IO_ERROR event.
> >  
> > +BLOCK_MEDIUM_EJECT
> > +------------------
> > +
> > +Emitted when the guest succeeds ejecting a medium. If the device has a 
> > tray,
> > +then this event is emitted whenever the guest opens or closes the tray.
> 
> This wording could be a bit confusing for devices where the physical
> device doesn't have a tray.  Our CD-ROMs have a tray.  Our floppy
> doesn't.  Our floppy device model doesn't let the guest eject, so it
> doesn't matter right now.  But trayless physical devices with removable
> media exist.
> 
> Suggest to say something like "Emitted whenever the guest successfully
> ejects or loads a medium, or opens or closes the tray."

Ok.

> > +
> > +Data:
> > +
> > +- "device": device name (json-string)
> 
> This is the backend name, i.e. the name that's shown in "info block".
> It's not the qdev ID.  Consistent with BLOCK_IO_ERROR.  Good.
> 
> We call too many things "device"...
> 
> > +- "ejected": true if the tray has been opened or false if it has been 
> > closed
> > +
> > +Example:
> > +
> > +{ "event": "BLOCK_MEDIUM_EJECT",
> > +    "data": { "device": "ide1-cd1",
> > +              "ejected": true },
> > +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> > +
> >  RESET
> >  -----
> >  
> > diff --git a/block.c b/block.c
> > index 3f072f6..b25932b 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1998,6 +1998,18 @@ BlockErrorAction bdrv_get_on_error(BlockDriverState 
> > *bs, int is_read)
> >      return is_read ? bs->on_read_error : bs->on_write_error;
> >  }
> >  
> > +void bdrv_dev_set_medium_eject_notify(BlockDriverState *bs,
> > +                                      bdrv_dev_medium_eject_notify_cb cb)
> > +{
> > +    bs->dev_medium_eject_notify_cb = cb;
> > +}
> > +
> > +void bdrv_dev_medium_eject_notify(BlockDriverState *bs, int is_ejected)
> > +{
> > +    assert(bs->dev_medium_eject_notify_cb);
> > +    bs->dev_medium_eject_notify_cb(bs, is_ejected);
> > +}
> > +
> >  int bdrv_is_read_only(BlockDriverState *bs)
> >  {
> >      return bs->read_only;
> > diff --git a/block.h b/block.h
> > index 3bd4398..344ca0d 100644
> > --- a/block.h
> > +++ b/block.h
> > @@ -247,6 +247,11 @@ int bdrv_get_translation_hint(BlockDriverState *bs);
> >  void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction 
> > on_read_error,
> >                         BlockErrorAction on_write_error);
> >  BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
> > +typedef void (*bdrv_dev_medium_eject_notify_cb)(BlockDriverState *bs,
> > +                                                int is_ejected);
> > +void bdrv_dev_set_medium_eject_notify(BlockDriverState *bs,
> > +                                      bdrv_dev_medium_eject_notify_cb cb);
> > +void bdrv_dev_medium_eject_notify(BlockDriverState *bs, int is_ejected);
> >  int bdrv_is_read_only(BlockDriverState *bs);
> >  int bdrv_is_sg(BlockDriverState *bs);
> >  int bdrv_enable_write_cache(BlockDriverState *bs);
> > diff --git a/block_int.h b/block_int.h
> > index 311bd2a..50c34f6 100644
> > --- a/block_int.h
> > +++ b/block_int.h
> > @@ -260,6 +260,9 @@ struct BlockDriverState {
> >      QTAILQ_ENTRY(BlockDriverState) list;
> >      void *private;
> >  
> > +    /* Callback for medium eject */
> > +    bdrv_dev_medium_eject_notify_cb dev_medium_eject_notify_cb;
> > +
> >      QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
> >  };
> >  
> 
> Why the indirection?  Isn't this always on_medium_eject()?
> bdrv_dev_medium_eject_notify_cb() asserts it's not null, and it's never
> set to anything else.
> 
> If the indirection is justified, why is it per object (in
> BlockDriverState) and not per driver (in BlockDriver)?
> 
> Update: I found a possible reason, see drive_init() below.

Let's discuss it below.

> block.h and block_int.h don't typedef their methods, suggest to stick to
> that.

Ok.

> 
> > diff --git a/blockdev.c b/blockdev.c
> > index 1f83c88..71b50fa 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -11,6 +11,7 @@
> >  #include "blockdev.h"
> >  #include "monitor.h"
> >  #include "qerror.h"
> > +#include "qjson.h"
> >  #include "qemu-option.h"
> >  #include "qemu-config.h"
> >  #include "sysemu.h"
> > @@ -237,6 +238,17 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
> >      return true;
> >  }
> >  
> > +static void on_medium_eject(BlockDriverState *bs, int is_ejected)
> > +{
> > +    QObject *data;
> > +
> > +    data = qobject_from_jsonf("{ 'device': %s, 'ejected': %i }",
> > +                              bdrv_get_device_name(bs), is_ejected);
> > +    monitor_protocol_event(QEVENT_BLOCK_MEDIUM_EJECT, data);
> > +
> > +    qobject_decref(data);
> > +}
> > +
> >  DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> >  {
> >      const char *buf;
> > @@ -503,6 +515,7 @@ DriveInfo *drive_init(QemuOpts *opts, int 
> > default_to_scsi)
> >      QTAILQ_INSERT_TAIL(&drives, dinfo, next);
> >  
> >      bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
> > +    bdrv_dev_set_medium_eject_notify(dinfo->bdrv, on_medium_eject);
> >  
> >      /* disk I/O throttling */
> >      bdrv_set_io_limits(dinfo->bdrv, &io_limits);
> 
> Aha.  The callback is set in BlockDriverStates created by drive_init()
> only.  I.e. it's set for block backends created by the user, with -drive
> / drive_add, or its sugared forms.
> 
> I figure you do this because you want the event emitted only for the
> "top" block backend (the the device model connects to), not the backends
> "below".
> 
> Example: -drive if=none,id=foo,file=foo.img gives us a raw backend named
> "foo" on top of an anonymous file backend.  The device model connects to
> "foo".  You want the event emitted for raw "foo", but not the anonymous
> file backend.
> 
> Example: -drive if=none,id=bar,file=bar.qcow2 gives us a qcow2 backend
> named "bar" on top of an anonymous file backend and another anonymous
> backend for the backing image (possibly on top of still more backends).
> The device model connects to "bar".  You want the event emitted for
> "bar", but not the anonymous backends below.
>
> Device models are always connected to a backend created by drive_init()
> right now.  That's why your assertion in
> bdrv_dev_set_medium_eject_notify() holds.

Thanks for the explanation. I have to admit that all this theory wasn't
clear to me. I based my review of the original code on what on_read_error
and on_write_error do.

> However, the envisaged block backend configuration revamp hopefully
> relegates DriveInfo to legacy status, i.e. only block backends created
> in legacy ways have a DriveInfo.  That'll break this code's assumption
> "the top block backend was created by drive_init()".
> 
> There's a cleaner way to detect the top backend: it has a name, and is
> in bdrv_states.  See bdrv_new().  But is it necessary?  Why can't we
> just call on_medium_eject() and be done with it?

Well, we can. I think that one benefit of the indirection is to avoid
direct dependencies between modules that are unrelated in principle (like
core ide & qmp). But calling on_medium_eject() directly works for me.

> Perhaps management applications eventually develop an interest in how
> the eject is propagated down.  Then we'll have to report it for all
> block backends, not just the top one.  And we'll also have to give the
> non-top ones names.  We'll cross that bridge when we come to it.

Yes.

> > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> > index 0adb27b..4b4bc61 100644
> > --- a/hw/ide/atapi.c
> > +++ b/hw/ide/atapi.c
> > @@ -885,6 +885,7 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* 
> > buf)
> >          }
> >          bdrv_eject(s->bs, !start);
> >          s->tray_open = !start;
> > +        bdrv_dev_medium_eject_notify(s->bs, !start);
> >      }
> >  
> >      ide_atapi_cmd_ok(s);
> > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> > index 5d8bf53..35e55f4 100644
> > --- a/hw/scsi-disk.c
> > +++ b/hw/scsi-disk.c
> > @@ -1061,6 +1061,7 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq 
> > *r)
> >          }
> >          bdrv_eject(s->qdev.conf.bs, !start);
> >          s->tray_open = !start;
> > +        bdrv_dev_medium_eject_notify(s->qdev.conf.bs, !start);
> >      }
> >      return 0;
> >  }
> 
> Would it be possible to emit the event automatically from bdrv_eject()?
> 
> We'd have to suppress it for anonymous block backends, of course.
> 
> Hmm, if we do that, then ide_tray_state_post_load() emits the event,
> too.  Would that be bad or good?

If we do eject the media, then it's probably good. But it's not clear to me
why we do this, ie. why do we eject & lock after we load the state?

> 
> > diff --git a/monitor.c b/monitor.c
> > index 187083c..df6b475 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -479,6 +479,9 @@ void monitor_protocol_event(MonitorEvent event, QObject 
> > *data)
> >          case QEVENT_SPICE_DISCONNECTED:
> >              event_name = "SPICE_DISCONNECTED";
> >              break;
> > +        case QEVENT_BLOCK_MEDIUM_EJECT:
> > +            event_name = "BLOCK_MEDIUM_EJECT";
> > +            break;
> >          default:
> >              abort();
> >              break;
> > diff --git a/monitor.h b/monitor.h
> > index 887c472..16fab50 100644
> > --- a/monitor.h
> > +++ b/monitor.h
> > @@ -36,6 +36,7 @@ typedef enum MonitorEvent {
> >      QEVENT_SPICE_CONNECTED,
> >      QEVENT_SPICE_INITIALIZED,
> >      QEVENT_SPICE_DISCONNECTED,
> > +    QEVENT_BLOCK_MEDIUM_EJECT,
> >      QEVENT_MAX,
> >  } MonitorEvent;
>