qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Luiz Capitulino
Subject: Re: [Qemu-devel] [PATCH 4/4] qmp: add BLOCK_MEDIUM_EJECT event
Date: Thu, 16 Feb 2012 11:14:47 -0200

On Thu, 16 Feb 2012 10:25:43 +0100
Markus Armbruster <address@hidden> wrote:

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

> > +
> > +{ "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).

> 
> > +        }
> > +        if (load) {
> > +            /* tray close */
> > +            bdrv_emit_qmp_eject_event(bs, false);
> 
> Likewise (!tray_was_closed && load)?
> 
> > +        }
> >      }
> >  }
> >  
> > @@ -3567,6 +3587,10 @@ void bdrv_eject(BlockDriverState *bs, bool 
> > eject_flag, bool tray_changed)
> >      if (drv && drv->bdrv_eject) {
> >          drv->bdrv_eject(bs, eject_flag);
> >      }
> > +
> > +    if (tray_changed) {
> > +        bdrv_emit_qmp_eject_event(bs, eject_flag);
> > +    }
> 
> I wonder why we bother to call bdrv_eject() with false tray_changed at
> all.  If there's no good reason for that, we could replace PATCH 3/4 by
> one that simply skips the call when the virtual tray stays put.

My understanding is that this is used for passthrough: we forward ejects,
independent of the state of the tray. Seems correct to me, but I'm not
against changing it.

> 
> >  }
> >  
> >  /**
> > diff --git a/monitor.c b/monitor.c
> > index aadbdcb..1758f03 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -485,6 +485,9 @@ void monitor_protocol_event(MonitorEvent event, QObject 
> > *data)
> >          case QEVENT_BLOCK_JOB_CANCELLED:
> >              event_name = "BLOCK_JOB_CANCELLED";
> >              break;
> > +        case QEVENT_BLOCK_MEDIUM_EJECT:
> > +             event_name = "BLOCK_MEDIUM_EJECT";
> > +            break;
> >          default:
> >              abort();
> >              break;
> > diff --git a/monitor.h b/monitor.h
> > index b72ea07..a2555e5 100644
> > --- a/monitor.h
> > +++ b/monitor.h
> > @@ -38,6 +38,7 @@ typedef enum MonitorEvent {
> >      QEVENT_SPICE_DISCONNECTED,
> >      QEVENT_BLOCK_JOB_COMPLETED,
> >      QEVENT_BLOCK_JOB_CANCELLED,
> > +    QEVENT_BLOCK_MEDIUM_EJECT,
> >      QEVENT_MAX,
> >  } MonitorEvent;
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]