[Qemu-devel] [PATCH 0/6] qapi: generate event defines automatically

[Qemu-devel] [PATCH 0/6] qapi: generate event defines automatically

[Wenchao Xia]

This series move the event define to qapi code, so later other components
could use it easily, it also make monitor code less and easier to decouple
with other code.

Original patch comes from my series titles as
[PATCH 00/13] trivial patches for event, error and monitor
To focus more on one item, pick up the event patches alone.

patch 6 was not merged to 4 since doc may be a bit much, so make it
separate to focus on doc's correctness.

Note:
please pay a bit attention on patch 6's doc for shutdown, powerdown,
stop, resume, suspend, suspend_disk, wakeup, to see if it is correct.
I paid some time to find the difference, and doced it from my understanding.
Esp shutdown and powerdown is not very clear, from qemu online doc it says
shutdown is gracefully operation and powerdown is brutely one, but the code
shows shutdown event is generated when qemu get signal from "kill", which is
correct? I hope to state those clearly in the doc, so I would be appreciate if
you have comments on the doc.

Wenchao Xia (6):
  1 block: use type MonitorEvent directly
  2 qapi: rename MonitorEvent to QEvent
  3 qapi: rename prefix QEVENT to Q_EVENT
  4 qapi: move event defines to qapi-schema.json
  5 qapi: remove var monitor_event_names[]
  6 qapi: add doc for QEvent

 balloon.c                  |    2 +-
 block.c                    |    6 +-
 block/qcow2-refcount.c     |    2 +-
 blockdev.c                 |    4 +-
 blockjob.c                 |    5 +-
 cpus.c                     |    2 +-
 hw/acpi/core.c             |    2 +-
 hw/core/qdev.c             |    2 +-
 hw/misc/pvpanic.c          |    2 +-
 hw/net/virtio-net.c        |    2 +-
 hw/watchdog/watchdog.c     |    2 +-
 include/block/block_int.h  |    2 +-
 include/monitor/monitor.h  |   39 +------------------
 monitor.c                  |   64 ++++++++----------------------
 qapi-schema.json           |   93 ++++++++++++++++++++++++++++++++++++++++++++
 stubs/mon-protocol-event.c |    2 +-
 target-s390x/kvm.c         |    2 +-
 ui/spice-core.c            |    8 ++--
 ui/vnc.c                   |    8 ++--
 vl.c                       |   14 +++---
 20 files changed, 144 insertions(+), 119 deletions(-)

[Qemu-devel] [PATCH 1/6] block: use type MonitorEvent directly

[Wenchao Xia]

block_int.h included monitor.h, so it knows the typedef.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                   |    2 +-
 include/block/block_int.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index fd05a80..2c15e5d 100644
--- a/block.c
+++ b/block.c
@@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
 }
 
 void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
-                               enum MonitorEvent ev,
+                               MonitorEvent ev,
                                BlockErrorAction action, bool is_read)
 {
     QObject *data;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a48731d..bcc72e2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
 int is_windows_drive(const char *filename);
 #endif
 void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
-                               enum MonitorEvent ev,
+                               MonitorEvent ev,
                                BlockErrorAction action, bool is_read);
 
 /**
-- 
1.7.1

[Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c                    |    2 +-
 include/block/block_int.h  |    2 +-
 include/monitor/monitor.h  |    6 +++---
 monitor.c                  |   12 ++++++------
 stubs/mon-protocol-event.c |    2 +-
 ui/vnc.c                   |    2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 2c15e5d..458a4f8 100644
--- a/block.c
+++ b/block.c
@@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
 }
 
 void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
-                               MonitorEvent ev,
+                               QEvent ev,
                                BlockErrorAction action, bool is_read)
 {
     QObject *data;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index bcc72e2..bfdaf84 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
 int is_windows_drive(const char *filename);
 #endif
 void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
-                               MonitorEvent ev,
+                               QEvent ev,
                                BlockErrorAction action, bool is_read);
 
 /**
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 10fa0e3..8b14a6f 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -20,7 +20,7 @@ extern Monitor *default_mon;
 #define MONITOR_CMD_ASYNC       0x0001
 
 /* QMP events */
-typedef enum MonitorEvent {
+typedef enum QEvent {
     QEVENT_SHUTDOWN,
     QEVENT_RESET,
     QEVENT_POWERDOWN,
@@ -54,11 +54,11 @@ typedef enum MonitorEvent {
      * defining new events here */
 
     QEVENT_MAX,
-} MonitorEvent;
+} QEvent;
 
 int monitor_cur_is_qmp(void);
 
-void monitor_protocol_event(MonitorEvent event, QObject *data);
+void monitor_protocol_event(QEvent event, QObject *data);
 void monitor_init(CharDriverState *chr, int flags);
 
 int monitor_suspend(Monitor *mon);
diff --git a/monitor.c b/monitor.c
index 74f3f1b..9377834 100644
--- a/monitor.c
+++ b/monitor.c
@@ -175,7 +175,7 @@ typedef struct MonitorControl {
  * instance.
  */
 typedef struct MonitorEventState {
-    MonitorEvent event; /* Event being tracked */
+    QEvent event;       /* Event being tracked */
     int64_t rate;       /* Period over which to throttle. 0 to disable */
     int64_t last;       /* Time at which event was last emitted */
     QEMUTimer *timer;   /* Timer for handling delayed events */
@@ -517,7 +517,7 @@ QemuMutex monitor_event_state_lock;
  * Emits the event to every monitor instance
  */
 static void
-monitor_protocol_event_emit(MonitorEvent event,
+monitor_protocol_event_emit(QEvent event,
                             QObject *data)
 {
     Monitor *mon;
@@ -536,7 +536,7 @@ monitor_protocol_event_emit(MonitorEvent event,
  * applying any rate limiting if required.
  */
 static void
-monitor_protocol_event_queue(MonitorEvent event,
+monitor_protocol_event_queue(QEvent event,
                              QObject *data)
 {
     MonitorEventState *evstate;
@@ -614,7 +614,7 @@ static void monitor_protocol_event_handler(void *opaque)
  * milliseconds
  */
 static void
-monitor_protocol_event_throttle(MonitorEvent event,
+monitor_protocol_event_throttle(QEvent event,
                                 int64_t rate)
 {
     MonitorEventState *evstate;
@@ -650,7 +650,7 @@ static void monitor_protocol_event_init(void)
  *
  * Event-specific data can be emitted through the (optional) 'data' parameter.
  */
-void monitor_protocol_event(MonitorEvent event, QObject *data)
+void monitor_protocol_event(QEvent event, QObject *data)
 {
     QDict *qmp;
     const char *event_name;
@@ -1067,7 +1067,7 @@ CommandInfoList *qmp_query_commands(Error **errp)
 EventInfoList *qmp_query_events(Error **errp)
 {
     EventInfoList *info, *ev_list = NULL;
-    MonitorEvent e;
+    QEvent e;
 
     for (e = 0 ; e < QEVENT_MAX ; e++) {
         const char *event_name = monitor_event_names[e];
diff --git a/stubs/mon-protocol-event.c b/stubs/mon-protocol-event.c
index 0946e94..e769729 100644
--- a/stubs/mon-protocol-event.c
+++ b/stubs/mon-protocol-event.c
@@ -1,6 +1,6 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 
-void monitor_protocol_event(MonitorEvent event, QObject *data)
+void monitor_protocol_event(QEvent event, QObject *data)
 {
 }
diff --git a/ui/vnc.c b/ui/vnc.c
index 5601cc3..47fda54 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -275,7 +275,7 @@ static void vnc_client_cache_addr(VncState *client)
     client->info = QOBJECT(qdict);
 }
 
-static void vnc_qmp_event(VncState *vs, MonitorEvent event)
+static void vnc_qmp_event(VncState *vs, QEvent event)
 {
     QDict *server;
     QObject *data;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

On 10/21/2013 03:16 AM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block.c                    |    2 +-
>  include/block/block_int.h  |    2 +-
>  include/monitor/monitor.h  |    6 +++---
>  monitor.c                  |   12 ++++++------
>  stubs/mon-protocol-event.c |    2 +-
>  ui/vnc.c                   |    2 +-
>  6 files changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Luiz Capitulino]

On Mon, 21 Oct 2013 10:16:01 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block.c                    |    2 +-
>  include/block/block_int.h  |    2 +-
>  include/monitor/monitor.h  |    6 +++---
>  monitor.c                  |   12 ++++++------
>  stubs/mon-protocol-event.c |    2 +-
>  ui/vnc.c                   |    2 +-
>  6 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 2c15e5d..458a4f8 100644
> --- a/block.c
> +++ b/block.c
> @@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
>  }
>  
>  void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
> -                               MonitorEvent ev,
> +                               QEvent ev,
>                                 BlockErrorAction action, bool is_read)
>  {
>      QObject *data;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index bcc72e2..bfdaf84 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>  int is_windows_drive(const char *filename);
>  #endif
>  void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
> -                               MonitorEvent ev,
> +                               QEvent ev,
>                                 BlockErrorAction action, bool is_read);
>  
>  /**
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 10fa0e3..8b14a6f 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -20,7 +20,7 @@ extern Monitor *default_mon;
>  #define MONITOR_CMD_ASYNC       0x0001
>  
>  /* QMP events */
> -typedef enum MonitorEvent {
> +typedef enum QEvent {

Qt has a QEvent class, so QEvent is not a good name for us if we're
considering making it public in the schema (which could become an
external library in the distant future).

I suggest calling it QMPEvent.

>      QEVENT_SHUTDOWN,
>      QEVENT_RESET,
>      QEVENT_POWERDOWN,
> @@ -54,11 +54,11 @@ typedef enum MonitorEvent {
>       * defining new events here */
>  
>      QEVENT_MAX,
> -} MonitorEvent;
> +} QEvent;
>  
>  int monitor_cur_is_qmp(void);
>  
> -void monitor_protocol_event(MonitorEvent event, QObject *data);
> +void monitor_protocol_event(QEvent event, QObject *data);
>  void monitor_init(CharDriverState *chr, int flags);
>  
>  int monitor_suspend(Monitor *mon);
> diff --git a/monitor.c b/monitor.c
> index 74f3f1b..9377834 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -175,7 +175,7 @@ typedef struct MonitorControl {
>   * instance.
>   */
>  typedef struct MonitorEventState {
> -    MonitorEvent event; /* Event being tracked */
> +    QEvent event;       /* Event being tracked */
>      int64_t rate;       /* Period over which to throttle. 0 to disable */
>      int64_t last;       /* Time at which event was last emitted */
>      QEMUTimer *timer;   /* Timer for handling delayed events */
> @@ -517,7 +517,7 @@ QemuMutex monitor_event_state_lock;
>   * Emits the event to every monitor instance
>   */
>  static void
> -monitor_protocol_event_emit(MonitorEvent event,
> +monitor_protocol_event_emit(QEvent event,
>                              QObject *data)
>  {
>      Monitor *mon;
> @@ -536,7 +536,7 @@ monitor_protocol_event_emit(MonitorEvent event,
>   * applying any rate limiting if required.
>   */
>  static void
> -monitor_protocol_event_queue(MonitorEvent event,
> +monitor_protocol_event_queue(QEvent event,
>                               QObject *data)
>  {
>      MonitorEventState *evstate;
> @@ -614,7 +614,7 @@ static void monitor_protocol_event_handler(void *opaque)
>   * milliseconds
>   */
>  static void
> -monitor_protocol_event_throttle(MonitorEvent event,
> +monitor_protocol_event_throttle(QEvent event,
>                                  int64_t rate)
>  {
>      MonitorEventState *evstate;
> @@ -650,7 +650,7 @@ static void monitor_protocol_event_init(void)
>   *
>   * Event-specific data can be emitted through the (optional) 'data' parameter.
>   */
> -void monitor_protocol_event(MonitorEvent event, QObject *data)
> +void monitor_protocol_event(QEvent event, QObject *data)
>  {
>      QDict *qmp;
>      const char *event_name;
> @@ -1067,7 +1067,7 @@ CommandInfoList *qmp_query_commands(Error **errp)
>  EventInfoList *qmp_query_events(Error **errp)
>  {
>      EventInfoList *info, *ev_list = NULL;
> -    MonitorEvent e;
> +    QEvent e;
>  
>      for (e = 0 ; e < QEVENT_MAX ; e++) {
>          const char *event_name = monitor_event_names[e];
> diff --git a/stubs/mon-protocol-event.c b/stubs/mon-protocol-event.c
> index 0946e94..e769729 100644
> --- a/stubs/mon-protocol-event.c
> +++ b/stubs/mon-protocol-event.c
> @@ -1,6 +1,6 @@
>  #include "qemu-common.h"
>  #include "monitor/monitor.h"
>  
> -void monitor_protocol_event(MonitorEvent event, QObject *data)
> +void monitor_protocol_event(QEvent event, QObject *data)
>  {
>  }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 5601cc3..47fda54 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -275,7 +275,7 @@ static void vnc_client_cache_addr(VncState *client)
>      client->info = QOBJECT(qdict);
>  }
>  
> -static void vnc_qmp_event(VncState *vs, MonitorEvent event)
> +static void vnc_qmp_event(VncState *vs, QEvent event)
>  {
>      QDict *server;
>      QObject *data;

Re: [Qemu-devel] [libvirt] QEMU 1.6 and drive discard parameter

[Gareth Bult]

Hey guys,

I've just rolled out Qemu 1.6 to fix problems I've been having, which worked fine .. but I've now
lost discard support which is a problem. Is there an easy / quick fix for this without digging through
other people's code? I'm happy to compile up whatever is necessary, I just need the "discard" option 
to work for Libvirt / Qemu 1.6 ...

tia
Gareth.


On Thu, Oct 31, 2013 at 04:35:43PM +0800, Amos Kong wrote:
> On Thu, Oct 31, 2013 at 04:07:15PM +0800, Osier Yang wrote:
> > CC to Amos.
> > 
> > On 30/10/13 16:19, whitearchey wrote:
> > >
> > >In QEMU 1.6 parameters of 'drive' option were removed:
> > >
> > >QemuOptsList qemu_drive_opts = {
> > >    .name = "drive",
> > >    .head = QTAILQ_HEAD_INITIALIZER(qemu_drive_opts.head),
> > >    .desc = {
> > >        /*
> > >         * no elements => accept any params
> > >         * validation will happen later
> > >         */
> > >        { /* end of list */ }
> > >    },
> > >};
> > >
> > >But libvirt still checks for QEMU_CAPS_DRIVE_DISCARD using QMP
> > >query-command-line-options:
> > >
> > >static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = {
> > >    { "machine", "mem-merge", QEMU_CAPS_MEM_MERGE },
> > >    { "drive", "discard", QEMU_CAPS_DRIVE_DISCARD },
> > >    { "realtime", "mlock", QEMU_CAPS_MLOCK },
> > >};
> > >...
> > >qemuMonitorGetCommandLineOptionParameters(mon,
> > >virQEMUCapsCommandLine[i].option, &values)
> > >
> > >So, when I try to use discard option in domain xml I get this error:
> > >
> > >error : qemuBuildDriveStr:3986 : unsupported configuration:
> > >discard is not supported by this QEMU binary
> > >
> > 
> > It's a qemu problem, the command "query-command-line-options" should
> > keep working
> > after the structures were changed for any option, in this case, all
> > the option descs were
> > moved to "qemu_common_drive_opts" instead.
> 
> { 'execute': 'query-command-line-options', 'arguments': { 'option': 'drive' } 
> }
>  
> {
>     "return": [
>         {
>             "parameters": [
>             ],
>             "option": "drive"
>         }
>     ]
> }
> 
> It returns a NULL parameters list, that's true, some error handling
> should be done by libvirt.

Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Wenchao Xia]

于 2013/11/1 22:02, Luiz Capitulino 写道:
> On Mon, 21 Oct 2013 10:16:01 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block.c                    |    2 +-
>>   include/block/block_int.h  |    2 +-
>>   include/monitor/monitor.h  |    6 +++---
>>   monitor.c                  |   12 ++++++------
>>   stubs/mon-protocol-event.c |    2 +-
>>   ui/vnc.c                   |    2 +-
>>   6 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 2c15e5d..458a4f8 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
>>   }
>>
>>   void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
>> -                               MonitorEvent ev,
>> +                               QEvent ev,
>>                                  BlockErrorAction action, bool is_read)
>>   {
>>       QObject *data;
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index bcc72e2..bfdaf84 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>>   int is_windows_drive(const char *filename);
>>   #endif
>>   void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
>> -                               MonitorEvent ev,
>> +                               QEvent ev,
>>                                  BlockErrorAction action, bool is_read);
>>
>>   /**
>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> index 10fa0e3..8b14a6f 100644
>> --- a/include/monitor/monitor.h
>> +++ b/include/monitor/monitor.h
>> @@ -20,7 +20,7 @@ extern Monitor *default_mon;
>>   #define MONITOR_CMD_ASYNC       0x0001
>>
>>   /* QMP events */
>> -typedef enum MonitorEvent {
>> +typedef enum QEvent {
>
> Qt has a QEvent class, so QEvent is not a good name for us if we're
> considering making it public in the schema (which could become an
> external library in the distant future).
>
> I suggest calling it QMPEvent.
>

   Maybe QMPEventType, since QMPEvent should be used an union?

{ 'Union': 'QMPEvent',
   'base': 'QMPEventBase',
   'discriminator': 'event',
   'data': {
       'SHUTDOWN'               : 'EventShutdown',
       'POWERDOWN'              : 'EventPowerdown',
       'RESET'                  : 'EventReset'
    } }

   I used "Event" as union name before, but I am afraid it is too
generic, so changed it as QMPEvent.

>>       QEVENT_SHUTDOWN,
>>       QEVENT_RESET,
>>       QEVENT_POWERDOWN,
>> @@ -54,11 +54,11 @@ typedef enum MonitorEvent {
>>        * defining new events here */
>>
>>       QEVENT_MAX,
>> -} MonitorEvent;
>> +} QEvent;
>>
>>   int monitor_cur_is_qmp(void);
>>
>> -void monitor_protocol_event(MonitorEvent event, QObject *data);
>> +void monitor_protocol_event(QEvent event, QObject *data);
>>   void monitor_init(CharDriverState *chr, int flags);
>>
>>   int monitor_suspend(Monitor *mon);
>> diff --git a/monitor.c b/monitor.c
>> index 74f3f1b..9377834 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -175,7 +175,7 @@ typedef struct MonitorControl {
>>    * instance.
>>    */
>>   typedef struct MonitorEventState {
>> -    MonitorEvent event; /* Event being tracked */
>> +    QEvent event;       /* Event being tracked */
>>       int64_t rate;       /* Period over which to throttle. 0 to disable */
>>       int64_t last;       /* Time at which event was last emitted */
>>       QEMUTimer *timer;   /* Timer for handling delayed events */
>> @@ -517,7 +517,7 @@ QemuMutex monitor_event_state_lock;
>>    * Emits the event to every monitor instance
>>    */
>>   static void
>> -monitor_protocol_event_emit(MonitorEvent event,
>> +monitor_protocol_event_emit(QEvent event,
>>                               QObject *data)
>>   {
>>       Monitor *mon;
>> @@ -536,7 +536,7 @@ monitor_protocol_event_emit(MonitorEvent event,
>>    * applying any rate limiting if required.
>>    */
>>   static void
>> -monitor_protocol_event_queue(MonitorEvent event,
>> +monitor_protocol_event_queue(QEvent event,
>>                                QObject *data)
>>   {
>>       MonitorEventState *evstate;
>> @@ -614,7 +614,7 @@ static void monitor_protocol_event_handler(void *opaque)
>>    * milliseconds
>>    */
>>   static void
>> -monitor_protocol_event_throttle(MonitorEvent event,
>> +monitor_protocol_event_throttle(QEvent event,
>>                                   int64_t rate)
>>   {
>>       MonitorEventState *evstate;
>> @@ -650,7 +650,7 @@ static void monitor_protocol_event_init(void)
>>    *
>>    * Event-specific data can be emitted through the (optional) 'data' parameter.
>>    */
>> -void monitor_protocol_event(MonitorEvent event, QObject *data)
>> +void monitor_protocol_event(QEvent event, QObject *data)
>>   {
>>       QDict *qmp;
>>       const char *event_name;
>> @@ -1067,7 +1067,7 @@ CommandInfoList *qmp_query_commands(Error **errp)
>>   EventInfoList *qmp_query_events(Error **errp)
>>   {
>>       EventInfoList *info, *ev_list = NULL;
>> -    MonitorEvent e;
>> +    QEvent e;
>>
>>       for (e = 0 ; e < QEVENT_MAX ; e++) {
>>           const char *event_name = monitor_event_names[e];
>> diff --git a/stubs/mon-protocol-event.c b/stubs/mon-protocol-event.c
>> index 0946e94..e769729 100644
>> --- a/stubs/mon-protocol-event.c
>> +++ b/stubs/mon-protocol-event.c
>> @@ -1,6 +1,6 @@
>>   #include "qemu-common.h"
>>   #include "monitor/monitor.h"
>>
>> -void monitor_protocol_event(MonitorEvent event, QObject *data)
>> +void monitor_protocol_event(QEvent event, QObject *data)
>>   {
>>   }
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 5601cc3..47fda54 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -275,7 +275,7 @@ static void vnc_client_cache_addr(VncState *client)
>>       client->info = QOBJECT(qdict);
>>   }
>>
>> -static void vnc_qmp_event(VncState *vs, MonitorEvent event)
>> +static void vnc_qmp_event(VncState *vs, QEvent event)
>>   {
>>       QDict *server;
>>       QObject *data;
>

Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Luiz Capitulino]

On Mon, 04 Nov 2013 09:59:50 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013/11/1 22:02, Luiz Capitulino 写道:
> > On Mon, 21 Oct 2013 10:16:01 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >> ---
> >>   block.c                    |    2 +-
> >>   include/block/block_int.h  |    2 +-
> >>   include/monitor/monitor.h  |    6 +++---
> >>   monitor.c                  |   12 ++++++------
> >>   stubs/mon-protocol-event.c |    2 +-
> >>   ui/vnc.c                   |    2 +-
> >>   6 files changed, 13 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index 2c15e5d..458a4f8 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
> >>   }
> >>
> >>   void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
> >> -                               MonitorEvent ev,
> >> +                               QEvent ev,
> >>                                  BlockErrorAction action, bool is_read)
> >>   {
> >>       QObject *data;
> >> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >> index bcc72e2..bfdaf84 100644
> >> --- a/include/block/block_int.h
> >> +++ b/include/block/block_int.h
> >> @@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
> >>   int is_windows_drive(const char *filename);
> >>   #endif
> >>   void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
> >> -                               MonitorEvent ev,
> >> +                               QEvent ev,
> >>                                  BlockErrorAction action, bool is_read);
> >>
> >>   /**
> >> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> >> index 10fa0e3..8b14a6f 100644
> >> --- a/include/monitor/monitor.h
> >> +++ b/include/monitor/monitor.h
> >> @@ -20,7 +20,7 @@ extern Monitor *default_mon;
> >>   #define MONITOR_CMD_ASYNC       0x0001
> >>
> >>   /* QMP events */
> >> -typedef enum MonitorEvent {
> >> +typedef enum QEvent {
> >
> > Qt has a QEvent class, so QEvent is not a good name for us if we're
> > considering making it public in the schema (which could become an
> > external library in the distant future).
> >
> > I suggest calling it QMPEvent.
> >
> 
>    Maybe QMPEventType, since QMPEvent should be used an union?

If we add the 'event' type, like:

{ 'event': 'BLOCK_IO_ERROR', 'data': { ... } }

Then we don't need an union.

Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Wenchao Xia]

于 2013/11/4 21:33, Luiz Capitulino 写道:
> On Mon, 04 Nov 2013 09:59:50 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013/11/1 22:02, Luiz Capitulino 写道:
>>> On Mon, 21 Oct 2013 10:16:01 +0800
>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> ---
>>>>    block.c                    |    2 +-
>>>>    include/block/block_int.h  |    2 +-
>>>>    include/monitor/monitor.h  |    6 +++---
>>>>    monitor.c                  |   12 ++++++------
>>>>    stubs/mon-protocol-event.c |    2 +-
>>>>    ui/vnc.c                   |    2 +-
>>>>    6 files changed, 13 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 2c15e5d..458a4f8 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
>>>>    }
>>>>
>>>>    void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
>>>> -                               MonitorEvent ev,
>>>> +                               QEvent ev,
>>>>                                   BlockErrorAction action, bool is_read)
>>>>    {
>>>>        QObject *data;
>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>>> index bcc72e2..bfdaf84 100644
>>>> --- a/include/block/block_int.h
>>>> +++ b/include/block/block_int.h
>>>> @@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>>>>    int is_windows_drive(const char *filename);
>>>>    #endif
>>>>    void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
>>>> -                               MonitorEvent ev,
>>>> +                               QEvent ev,
>>>>                                   BlockErrorAction action, bool is_read);
>>>>
>>>>    /**
>>>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>>>> index 10fa0e3..8b14a6f 100644
>>>> --- a/include/monitor/monitor.h
>>>> +++ b/include/monitor/monitor.h
>>>> @@ -20,7 +20,7 @@ extern Monitor *default_mon;
>>>>    #define MONITOR_CMD_ASYNC       0x0001
>>>>
>>>>    /* QMP events */
>>>> -typedef enum MonitorEvent {
>>>> +typedef enum QEvent {
>>>
>>> Qt has a QEvent class, so QEvent is not a good name for us if we're
>>> considering making it public in the schema (which could become an
>>> external library in the distant future).
>>>
>>> I suggest calling it QMPEvent.
>>>
>>
>>     Maybe QMPEventType, since QMPEvent should be used an union?
>
> If we add the 'event' type, like:
>
> { 'event': 'BLOCK_IO_ERROR', 'data': { ... } }
>
> Then we don't need an union.
>

   It would bring a little trouble to C code caller, for example the
event generate function(just like monitor_protocol_event) would be:
event_generate(QMPEvent *e);
   We may need to make *e as opaque and handly writing code for "switch
", but with union, the generated code would do that for us, so I think
union is better.

Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Luiz Capitulino]

On Tue, 05 Nov 2013 10:17:28 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013/11/4 21:33, Luiz Capitulino 写道:
> > On Mon, 04 Nov 2013 09:59:50 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >> 于 2013/11/1 22:02, Luiz Capitulino 写道:
> >>> On Mon, 21 Oct 2013 10:16:01 +0800
> >>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>
> >>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>> ---
> >>>>    block.c                    |    2 +-
> >>>>    include/block/block_int.h  |    2 +-
> >>>>    include/monitor/monitor.h  |    6 +++---
> >>>>    monitor.c                  |   12 ++++++------
> >>>>    stubs/mon-protocol-event.c |    2 +-
> >>>>    ui/vnc.c                   |    2 +-
> >>>>    6 files changed, 13 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/block.c b/block.c
> >>>> index 2c15e5d..458a4f8 100644
> >>>> --- a/block.c
> >>>> +++ b/block.c
> >>>> @@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
> >>>>    }
> >>>>
> >>>>    void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
> >>>> -                               MonitorEvent ev,
> >>>> +                               QEvent ev,
> >>>>                                   BlockErrorAction action, bool is_read)
> >>>>    {
> >>>>        QObject *data;
> >>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >>>> index bcc72e2..bfdaf84 100644
> >>>> --- a/include/block/block_int.h
> >>>> +++ b/include/block/block_int.h
> >>>> @@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
> >>>>    int is_windows_drive(const char *filename);
> >>>>    #endif
> >>>>    void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
> >>>> -                               MonitorEvent ev,
> >>>> +                               QEvent ev,
> >>>>                                   BlockErrorAction action, bool is_read);
> >>>>
> >>>>    /**
> >>>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> >>>> index 10fa0e3..8b14a6f 100644
> >>>> --- a/include/monitor/monitor.h
> >>>> +++ b/include/monitor/monitor.h
> >>>> @@ -20,7 +20,7 @@ extern Monitor *default_mon;
> >>>>    #define MONITOR_CMD_ASYNC       0x0001
> >>>>
> >>>>    /* QMP events */
> >>>> -typedef enum MonitorEvent {
> >>>> +typedef enum QEvent {
> >>>
> >>> Qt has a QEvent class, so QEvent is not a good name for us if we're
> >>> considering making it public in the schema (which could become an
> >>> external library in the distant future).
> >>>
> >>> I suggest calling it QMPEvent.
> >>>
> >>
> >>     Maybe QMPEventType, since QMPEvent should be used an union?
> >
> > If we add the 'event' type, like:
> >
> > { 'event': 'BLOCK_IO_ERROR', 'data': { ... } }
> >
> > Then we don't need an union.
> >
> 
>    It would bring a little trouble to C code caller, for example the
> event generate function(just like monitor_protocol_event) would be:
> event_generate(QMPEvent *e);

Hmm, no.

Today, events are open-coded and an event's definition lives in the code,
like the DEVICE_TRAY_MOVED event:

static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool ejected)
{
    QObject *data;

    data = qobject_from_jsonf("{ 'device': %s, 'tray-open': %i }",
                              bdrv_get_device_name(bs), ejected);
    monitor_protocol_event(QEVENT_DEVICE_TRAY_MOVED, data);

    qobject_decref(data);
}

Having an union for the event's names saves some typing elsewhere, but
it doesn't solve the problem of having the event definition in the code.
Adding event type support to the QAPI does solve this problem.

Taking the DEVICE_TRAY_MOVED event as an example, we would have the
following entry in the qapi-schema.json file:

{ 'event': 'DEVICE_TRAY_MOVED', 'data': { 'device': 'str',
                                          'tray-open': 'bool' } }

Then the QAPI generator would generate this function:

void qmp_send_event_device_tray_moved(const char *device, bool tray_open);

This function uses visitors to build a qobject which is then passed
to monitor_protocol_event(), along with the event name. Also note that
monitor_protocol_event() could take the event name as a string, which
completely eliminates the current enum MonitorEvent.

I believe this is the direction we want to go wrt events.

Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Wenchao Xia]

于 2013/11/5 10:51, Luiz Capitulino 写道:
> On Tue, 05 Nov 2013 10:17:28 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013/11/4 21:33, Luiz Capitulino 写道:
>>> On Mon, 04 Nov 2013 09:59:50 +0800
>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>
>>>> 于 2013/11/1 22:02, Luiz Capitulino 写道:
>>>>> On Mon, 21 Oct 2013 10:16:01 +0800
>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>     block.c                    |    2 +-
>>>>>>     include/block/block_int.h  |    2 +-
>>>>>>     include/monitor/monitor.h  |    6 +++---
>>>>>>     monitor.c                  |   12 ++++++------
>>>>>>     stubs/mon-protocol-event.c |    2 +-
>>>>>>     ui/vnc.c                   |    2 +-
>>>>>>     6 files changed, 13 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/block.c b/block.c
>>>>>> index 2c15e5d..458a4f8 100644
>>>>>> --- a/block.c
>>>>>> +++ b/block.c
>>>>>> @@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
>>>>>>     }
>>>>>>
>>>>>>     void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
>>>>>> -                               MonitorEvent ev,
>>>>>> +                               QEvent ev,
>>>>>>                                    BlockErrorAction action, bool is_read)
>>>>>>     {
>>>>>>         QObject *data;
>>>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>>>>> index bcc72e2..bfdaf84 100644
>>>>>> --- a/include/block/block_int.h
>>>>>> +++ b/include/block/block_int.h
>>>>>> @@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>>>>>>     int is_windows_drive(const char *filename);
>>>>>>     #endif
>>>>>>     void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
>>>>>> -                               MonitorEvent ev,
>>>>>> +                               QEvent ev,
>>>>>>                                    BlockErrorAction action, bool is_read);
>>>>>>
>>>>>>     /**
>>>>>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>>>>>> index 10fa0e3..8b14a6f 100644
>>>>>> --- a/include/monitor/monitor.h
>>>>>> +++ b/include/monitor/monitor.h
>>>>>> @@ -20,7 +20,7 @@ extern Monitor *default_mon;
>>>>>>     #define MONITOR_CMD_ASYNC       0x0001
>>>>>>
>>>>>>     /* QMP events */
>>>>>> -typedef enum MonitorEvent {
>>>>>> +typedef enum QEvent {
>>>>>
>>>>> Qt has a QEvent class, so QEvent is not a good name for us if we're
>>>>> considering making it public in the schema (which could become an
>>>>> external library in the distant future).
>>>>>
>>>>> I suggest calling it QMPEvent.
>>>>>
>>>>
>>>>      Maybe QMPEventType, since QMPEvent should be used an union?
>>>
>>> If we add the 'event' type, like:
>>>
>>> { 'event': 'BLOCK_IO_ERROR', 'data': { ... } }
>>>
>>> Then we don't need an union.
>>>
>>
>>     It would bring a little trouble to C code caller, for example the
>> event generate function(just like monitor_protocol_event) would be:
>> event_generate(QMPEvent *e);
>
> Hmm, no.
>
> Today, events are open-coded and an event's definition lives in the code,
> like the DEVICE_TRAY_MOVED event:
>
> static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool ejected)
> {
>      QObject *data;
>
>      data = qobject_from_jsonf("{ 'device': %s, 'tray-open': %i }",
>                                bdrv_get_device_name(bs), ejected);
>      monitor_protocol_event(QEVENT_DEVICE_TRAY_MOVED, data);
>
>      qobject_decref(data);
> }
>
> Having an union for the event's names saves some typing elsewhere, but
> it doesn't solve the problem of having the event definition in the code.
> Adding event type support to the QAPI does solve this problem.
>
> Taking the DEVICE_TRAY_MOVED event as an example, we would have the
> following entry in the qapi-schema.json file:
>
> { 'event': 'DEVICE_TRAY_MOVED', 'data': { 'device': 'str',
>                                            'tray-open': 'bool' } }
>
> Then the QAPI generator would generate this function:
>
> void qmp_send_event_device_tray_moved(const char *device, bool tray_open);
>
> This function uses visitors to build a qobject which is then passed
> to monitor_protocol_event(), along with the event name. Also note that
> monitor_protocol_event() could take the event name as a string, which
> completely eliminates the current enum MonitorEvent.
>
> I believe this is the direction we want to go wrt events.
>

   It seems a different direction: let QAPI generator recognize
keyword 'event', and generate emit functions.

My draft is a bit different:
caller:

   QMPEvent *e = g_malloc0(sizeof(QMPEvent));
   e->kind = QMP_EVENT_TYPE_DEVICE_TRAY_MOVED;
   e->device_tray_moved = g_malloc0(sizeof(QMPEventDeviceTrayMoved));
   e->device_tray_moved->device = g_strdup("ide0-hd");
   e->device_tray_moved->tray_open = false;
   qmp_event_generate(e);
   qapi_free_QMPEvent(e);

   Then qmp_event_generate() will use visitors to build qobject and then
emit it.

   Compared with above, I think qmp_send_event_device_tray_moved()
method saves malloc and free calls, other things are similar(My draft
tells the caller how to set value by structure define, while your way
tells it by function define), but it may require more modification
for genrator scripts which I need to rework on, so wonder whether the
easier way is acceptable.

Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Luiz Capitulino]

On Tue, 05 Nov 2013 13:31:09 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013/11/5 10:51, Luiz Capitulino 写道:
> > On Tue, 05 Nov 2013 10:17:28 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >> 于 2013/11/4 21:33, Luiz Capitulino 写道:
> >>> On Mon, 04 Nov 2013 09:59:50 +0800
> >>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>
> >>>> 于 2013/11/1 22:02, Luiz Capitulino 写道:
> >>>>> On Mon, 21 Oct 2013 10:16:01 +0800
> >>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>>
> >>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>>>> ---
> >>>>>>     block.c                    |    2 +-
> >>>>>>     include/block/block_int.h  |    2 +-
> >>>>>>     include/monitor/monitor.h  |    6 +++---
> >>>>>>     monitor.c                  |   12 ++++++------
> >>>>>>     stubs/mon-protocol-event.c |    2 +-
> >>>>>>     ui/vnc.c                   |    2 +-
> >>>>>>     6 files changed, 13 insertions(+), 13 deletions(-)
> >>>>>>
> >>>>>> diff --git a/block.c b/block.c
> >>>>>> index 2c15e5d..458a4f8 100644
> >>>>>> --- a/block.c
> >>>>>> +++ b/block.c
> >>>>>> @@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
> >>>>>>     }
> >>>>>>
> >>>>>>     void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
> >>>>>> -                               MonitorEvent ev,
> >>>>>> +                               QEvent ev,
> >>>>>>                                    BlockErrorAction action, bool is_read)
> >>>>>>     {
> >>>>>>         QObject *data;
> >>>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >>>>>> index bcc72e2..bfdaf84 100644
> >>>>>> --- a/include/block/block_int.h
> >>>>>> +++ b/include/block/block_int.h
> >>>>>> @@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
> >>>>>>     int is_windows_drive(const char *filename);
> >>>>>>     #endif
> >>>>>>     void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
> >>>>>> -                               MonitorEvent ev,
> >>>>>> +                               QEvent ev,
> >>>>>>                                    BlockErrorAction action, bool is_read);
> >>>>>>
> >>>>>>     /**
> >>>>>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> >>>>>> index 10fa0e3..8b14a6f 100644
> >>>>>> --- a/include/monitor/monitor.h
> >>>>>> +++ b/include/monitor/monitor.h
> >>>>>> @@ -20,7 +20,7 @@ extern Monitor *default_mon;
> >>>>>>     #define MONITOR_CMD_ASYNC       0x0001
> >>>>>>
> >>>>>>     /* QMP events */
> >>>>>> -typedef enum MonitorEvent {
> >>>>>> +typedef enum QEvent {
> >>>>>
> >>>>> Qt has a QEvent class, so QEvent is not a good name for us if we're
> >>>>> considering making it public in the schema (which could become an
> >>>>> external library in the distant future).
> >>>>>
> >>>>> I suggest calling it QMPEvent.
> >>>>>
> >>>>
> >>>>      Maybe QMPEventType, since QMPEvent should be used an union?
> >>>
> >>> If we add the 'event' type, like:
> >>>
> >>> { 'event': 'BLOCK_IO_ERROR', 'data': { ... } }
> >>>
> >>> Then we don't need an union.
> >>>
> >>
> >>     It would bring a little trouble to C code caller, for example the
> >> event generate function(just like monitor_protocol_event) would be:
> >> event_generate(QMPEvent *e);
> >
> > Hmm, no.
> >
> > Today, events are open-coded and an event's definition lives in the code,
> > like the DEVICE_TRAY_MOVED event:
> >
> > static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool ejected)
> > {
> >      QObject *data;
> >
> >      data = qobject_from_jsonf("{ 'device': %s, 'tray-open': %i }",
> >                                bdrv_get_device_name(bs), ejected);
> >      monitor_protocol_event(QEVENT_DEVICE_TRAY_MOVED, data);
> >
> >      qobject_decref(data);
> > }
> >
> > Having an union for the event's names saves some typing elsewhere, but
> > it doesn't solve the problem of having the event definition in the code.
> > Adding event type support to the QAPI does solve this problem.
> >
> > Taking the DEVICE_TRAY_MOVED event as an example, we would have the
> > following entry in the qapi-schema.json file:
> >
> > { 'event': 'DEVICE_TRAY_MOVED', 'data': { 'device': 'str',
> >                                            'tray-open': 'bool' } }
> >
> > Then the QAPI generator would generate this function:
> >
> > void qmp_send_event_device_tray_moved(const char *device, bool tray_open);
> >
> > This function uses visitors to build a qobject which is then passed
> > to monitor_protocol_event(), along with the event name. Also note that
> > monitor_protocol_event() could take the event name as a string, which
> > completely eliminates the current enum MonitorEvent.
> >
> > I believe this is the direction we want to go wrt events.
> >
> 
>    It seems a different direction: let QAPI generator recognize
> keyword 'event', and generate emit functions.

Yes, the same way we have it for QMP commands.

> My draft is a bit different:
> caller:
> 
>    QMPEvent *e = g_malloc0(sizeof(QMPEvent));
>    e->kind = QMP_EVENT_TYPE_DEVICE_TRAY_MOVED;
>    e->device_tray_moved = g_malloc0(sizeof(QMPEventDeviceTrayMoved));
>    e->device_tray_moved->device = g_strdup("ide0-hd");
>    e->device_tray_moved->tray_open = false;
>    qmp_event_generate(e);
>    qapi_free_QMPEvent(e);
> 
>    Then qmp_event_generate() will use visitors to build qobject and then
> emit it.

Seems a lot more complex to me, we're going to write a lot of code if
we do this.

>    Compared with above, I think qmp_send_event_device_tray_moved()
> method saves malloc and free calls, other things are similar(My draft
> tells the caller how to set value by structure define, while your way
> tells it by function define), but it may require more modification
> for genrator scripts which I need to rework on, so wonder whether the
> easier way is acceptable.

We'll add event support to the code generator only once, then all
a programmer has to do to add a new event is to add an new entry
in the qapi-schema.json and call the generated qmp_send_event_()
function from the appropriate place. That's trivial compared to
what you showed above. Besides, there are other advantages. One of
them is adding support to allow for C code to listen for events,
which we might want to have in the future and builds nicely on
top of having the QAPI event type.

Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Wenchao Xia]

于 2013/11/5 22:06, Luiz Capitulino 写道:
> On Tue, 05 Nov 2013 13:31:09 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013/11/5 10:51, Luiz Capitulino 写道:
>>> On Tue, 05 Nov 2013 10:17:28 +0800
>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>
>>>> 于 2013/11/4 21:33, Luiz Capitulino 写道:
>>>>> On Mon, 04 Nov 2013 09:59:50 +0800
>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>> 于 2013/11/1 22:02, Luiz Capitulino 写道:
>>>>>>> On Mon, 21 Oct 2013 10:16:01 +0800
>>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>>>
>>>>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>>>>> ---
>>>>>>>>      block.c                    |    2 +-
>>>>>>>>      include/block/block_int.h  |    2 +-
>>>>>>>>      include/monitor/monitor.h  |    6 +++---
>>>>>>>>      monitor.c                  |   12 ++++++------
>>>>>>>>      stubs/mon-protocol-event.c |    2 +-
>>>>>>>>      ui/vnc.c                   |    2 +-
>>>>>>>>      6 files changed, 13 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/block.c b/block.c
>>>>>>>> index 2c15e5d..458a4f8 100644
>>>>>>>> --- a/block.c
>>>>>>>> +++ b/block.c
>>>>>>>> @@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
>>>>>>>>      }
>>>>>>>>
>>>>>>>>      void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
>>>>>>>> -                               MonitorEvent ev,
>>>>>>>> +                               QEvent ev,
>>>>>>>>                                     BlockErrorAction action, bool is_read)
>>>>>>>>      {
>>>>>>>>          QObject *data;
>>>>>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>>>>>>> index bcc72e2..bfdaf84 100644
>>>>>>>> --- a/include/block/block_int.h
>>>>>>>> +++ b/include/block/block_int.h
>>>>>>>> @@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>>>>>>>>      int is_windows_drive(const char *filename);
>>>>>>>>      #endif
>>>>>>>>      void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
>>>>>>>> -                               MonitorEvent ev,
>>>>>>>> +                               QEvent ev,
>>>>>>>>                                     BlockErrorAction action, bool is_read);
>>>>>>>>
>>>>>>>>      /**
>>>>>>>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>>>>>>>> index 10fa0e3..8b14a6f 100644
>>>>>>>> --- a/include/monitor/monitor.h
>>>>>>>> +++ b/include/monitor/monitor.h
>>>>>>>> @@ -20,7 +20,7 @@ extern Monitor *default_mon;
>>>>>>>>      #define MONITOR_CMD_ASYNC       0x0001
>>>>>>>>
>>>>>>>>      /* QMP events */
>>>>>>>> -typedef enum MonitorEvent {
>>>>>>>> +typedef enum QEvent {
>>>>>>>
>>>>>>> Qt has a QEvent class, so QEvent is not a good name for us if we're
>>>>>>> considering making it public in the schema (which could become an
>>>>>>> external library in the distant future).
>>>>>>>
>>>>>>> I suggest calling it QMPEvent.
>>>>>>>
>>>>>>
>>>>>>       Maybe QMPEventType, since QMPEvent should be used an union?
>>>>>
>>>>> If we add the 'event' type, like:
>>>>>
>>>>> { 'event': 'BLOCK_IO_ERROR', 'data': { ... } }
>>>>>
>>>>> Then we don't need an union.
>>>>>
>>>>
>>>>      It would bring a little trouble to C code caller, for example the
>>>> event generate function(just like monitor_protocol_event) would be:
>>>> event_generate(QMPEvent *e);
>>>
>>> Hmm, no.
>>>
>>> Today, events are open-coded and an event's definition lives in the code,
>>> like the DEVICE_TRAY_MOVED event:
>>>
>>> static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool ejected)
>>> {
>>>       QObject *data;
>>>
>>>       data = qobject_from_jsonf("{ 'device': %s, 'tray-open': %i }",
>>>                                 bdrv_get_device_name(bs), ejected);
>>>       monitor_protocol_event(QEVENT_DEVICE_TRAY_MOVED, data);
>>>
>>>       qobject_decref(data);
>>> }
>>>
>>> Having an union for the event's names saves some typing elsewhere, but
>>> it doesn't solve the problem of having the event definition in the code.
>>> Adding event type support to the QAPI does solve this problem.
>>>
>>> Taking the DEVICE_TRAY_MOVED event as an example, we would have the
>>> following entry in the qapi-schema.json file:
>>>
>>> { 'event': 'DEVICE_TRAY_MOVED', 'data': { 'device': 'str',
>>>                                             'tray-open': 'bool' } }
>>>
>>> Then the QAPI generator would generate this function:
>>>
>>> void qmp_send_event_device_tray_moved(const char *device, bool tray_open);
>>>
>>> This function uses visitors to build a qobject which is then passed
>>> to monitor_protocol_event(), along with the event name. Also note that
>>> monitor_protocol_event() could take the event name as a string, which
>>> completely eliminates the current enum MonitorEvent.
>>>
>>> I believe this is the direction we want to go wrt events.
>>>
>>
>>     It seems a different direction: let QAPI generator recognize
>> keyword 'event', and generate emit functions.
>
> Yes, the same way we have it for QMP commands.
>
>> My draft is a bit different:
>> caller:
>>
>>     QMPEvent *e = g_malloc0(sizeof(QMPEvent));
>>     e->kind = QMP_EVENT_TYPE_DEVICE_TRAY_MOVED;
>>     e->device_tray_moved = g_malloc0(sizeof(QMPEventDeviceTrayMoved));
>>     e->device_tray_moved->device = g_strdup("ide0-hd");
>>     e->device_tray_moved->tray_open = false;
>>     qmp_event_generate(e);
>>     qapi_free_QMPEvent(e);
>>
>>     Then qmp_event_generate() will use visitors to build qobject and then
>> emit it.
>
> Seems a lot more complex to me, we're going to write a lot of code if
> we do this.
>

   OK, it is a good enough reason, I'll write qapi-event.py to get
qapi-event.h and qapi-event.c.

>>     Compared with above, I think qmp_send_event_device_tray_moved()
>> method saves malloc and free calls, other things are similar(My draft
>> tells the caller how to set value by structure define, while your way
>> tells it by function define), but it may require more modification
>> for genrator scripts which I need to rework on, so wonder whether the
>> easier way is acceptable.
>
> We'll add event support to the code generator only once, then all
> a programmer has to do to add a new event is to add an new entry
> in the qapi-schema.json and call the generated qmp_send_event_()
> function from the appropriate place. That's trivial compared to
> what you showed above. Besides, there are other advantages. One of
> them is adding support to allow for C code to listen for events,
> which we might want to have in the future and builds nicely on
> top of having the QAPI event type.
>

[Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Wenchao Xia]

The define will be moved to qapi-schema.json later, so rename the
prefix to match its naming style.

Also fixed code style error reported in spice-core.c.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 balloon.c                 |    2 +-
 block.c                   |    4 +-
 block/qcow2-refcount.c    |    2 +-
 blockdev.c                |    4 +-
 blockjob.c                |    5 ++-
 cpus.c                    |    2 +-
 hw/acpi/core.c            |    2 +-
 hw/core/qdev.c            |    2 +-
 hw/misc/pvpanic.c         |    2 +-
 hw/net/virtio-net.c       |    2 +-
 hw/watchdog/watchdog.c    |    2 +-
 include/monitor/monitor.h |   58 +++++++++++++++++-----------------
 monitor.c                 |   74 ++++++++++++++++++++++----------------------
 target-s390x/kvm.c        |    2 +-
 ui/spice-core.c           |    8 ++--
 ui/vnc.c                  |    6 ++--
 vl.c                      |   14 ++++----
 17 files changed, 96 insertions(+), 95 deletions(-)

diff --git a/balloon.c b/balloon.c
index e321f2c..16ea2e1 100644
--- a/balloon.c
+++ b/balloon.c
@@ -88,7 +88,7 @@ void qemu_balloon_changed(int64_t actual)
     data = qobject_from_jsonf("{ 'actual': %" PRId64 " }",
                               actual);
 
-    monitor_protocol_event(QEVENT_BALLOON_CHANGE, data);
+    monitor_protocol_event(Q_EVENT_BALLOON_CHANGE, data);
 
     qobject_decref(data);
 }
diff --git a/block.c b/block.c
index 458a4f8..002405f 100644
--- a/block.c
+++ b/block.c
@@ -1795,7 +1795,7 @@ static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool ejected)
 
     data = qobject_from_jsonf("{ 'device': %s, 'tray-open': %i }",
                               bdrv_get_device_name(bs), ejected);
-    monitor_protocol_event(QEVENT_DEVICE_TRAY_MOVED, data);
+    monitor_protocol_event(Q_EVENT_DEVICE_TRAY_MOVED, data);
 
     qobject_decref(data);
 }
@@ -2926,7 +2926,7 @@ void bdrv_error_action(BlockDriverState *bs, BlockErrorAction action,
                        bool is_read, int error)
 {
     assert(error >= 0);
-    bdrv_emit_qmp_error_event(bs, QEVENT_BLOCK_IO_ERROR, action, is_read);
+    bdrv_emit_qmp_error_event(bs, Q_EVENT_BLOCK_IO_ERROR, action, is_read);
     if (action == BDRV_ACTION_STOP) {
         vm_stop(RUN_STATE_IO_ERROR);
         bdrv_iostatus_set_err(bs, error);
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 1ff43d0..f31faa9 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1787,7 +1787,7 @@ int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
         data = qobject_from_jsonf("{ 'device': %s, 'msg': %s, 'offset': %"
                 PRId64 ", 'size': %" PRId64 " }", bs->device_name, message,
                 offset, size);
-        monitor_protocol_event(QEVENT_BLOCK_IMAGE_CORRUPTED, data);
+        monitor_protocol_event(Q_EVENT_BLOCK_IMAGE_CORRUPTED, data);
         g_free(message);
         qobject_decref(data);
 
diff --git a/blockdev.c b/blockdev.c
index b260477..ed30f3a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1725,9 +1725,9 @@ static void block_job_cb(void *opaque, int ret)
     }
 
     if (block_job_is_cancelled(bs->job)) {
-        monitor_protocol_event(QEVENT_BLOCK_JOB_CANCELLED, obj);
+        monitor_protocol_event(Q_EVENT_BLOCK_JOB_CANCELLED, obj);
     } else {
-        monitor_protocol_event(QEVENT_BLOCK_JOB_COMPLETED, obj);
+        monitor_protocol_event(Q_EVENT_BLOCK_JOB_COMPLETED, obj);
     }
     qobject_decref(obj);
 
diff --git a/blockjob.c b/blockjob.c
index 9e5fd5c..77940de 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -246,7 +246,7 @@ QObject *qobject_from_block_job(BlockJob *job)
 void block_job_ready(BlockJob *job)
 {
     QObject *data = qobject_from_block_job(job);
-    monitor_protocol_event(QEVENT_BLOCK_JOB_READY, data);
+    monitor_protocol_event(Q_EVENT_BLOCK_JOB_READY, data);
     qobject_decref(data);
 }
 
@@ -272,7 +272,8 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
     default:
         abort();
     }
-    bdrv_emit_qmp_error_event(job->bs, QEVENT_BLOCK_JOB_ERROR, action, is_read);
+    bdrv_emit_qmp_error_event(job->bs, Q_EVENT_BLOCK_JOB_ERROR,
+                              action, is_read);
     if (action == BDRV_ACTION_STOP) {
         block_job_pause(job);
         block_job_iostatus_set_err(job, error);
diff --git a/cpus.c b/cpus.c
index 398229e..1fe67d3 100644
--- a/cpus.c
+++ b/cpus.c
@@ -530,7 +530,7 @@ static int do_vm_stop(RunState state)
         pause_all_vcpus();
         runstate_set(state);
         vm_state_notify(0, state);
-        monitor_protocol_event(QEVENT_STOP, NULL);
+        monitor_protocol_event(Q_EVENT_STOP, NULL);
     }
 
     bdrv_drain_all();
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 7138139..fbb8efb 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -511,7 +511,7 @@ static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
             break;
         default:
             if (sus_typ == ar->pm1.cnt.s4_val) { /* S4 request */
-                monitor_protocol_event(QEVENT_SUSPEND_DISK, NULL);
+                monitor_protocol_event(Q_EVENT_SUSPEND_DISK, NULL);
                 qemu_system_shutdown_request();
             }
             break;
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 533f6dd..9dc1ac7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -821,7 +821,7 @@ static void device_unparent(Object *obj)
         } else {
             event_data = qobject_from_jsonf("{ 'path': %s }", path);
         }
-        monitor_protocol_event(QEVENT_DEVICE_DELETED, event_data);
+        monitor_protocol_event(Q_EVENT_DEVICE_DELETED, event_data);
         qobject_decref(event_data);
         g_free(path);
     }
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index b64e3bb..192b60a 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -36,7 +36,7 @@ static void panicked_mon_event(const char *action)
     QObject *data;
 
     data = qobject_from_jsonf("{ 'action': %s }", action);
-    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
+    monitor_protocol_event(Q_EVENT_GUEST_PANICKED, data);
     qobject_decref(data);
 }
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 22dbd05..c35cc8e 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -208,7 +208,7 @@ static void rxfilter_notify(NetClientState *nc)
             event_data = qobject_from_jsonf("{ 'path': %s }",
                                     object_get_canonical_path(OBJECT(n->qdev)));
         }
-        monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data);
+        monitor_protocol_event(Q_EVENT_NIC_RX_FILTER_CHANGED, event_data);
         qobject_decref(event_data);
 
         /* disable event notification to avoid events flooding */
diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 387962e..4501685 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -105,7 +105,7 @@ static void watchdog_mon_event(const char *action)
     QObject *data;
 
     data = qobject_from_jsonf("{ 'action': %s }", action);
-    monitor_protocol_event(QEVENT_WATCHDOG, data);
+    monitor_protocol_event(Q_EVENT_WATCHDOG, data);
     qobject_decref(data);
 }
 
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 8b14a6f..b273c5b 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -21,39 +21,39 @@ extern Monitor *default_mon;
 
 /* QMP events */
 typedef enum QEvent {
-    QEVENT_SHUTDOWN,
-    QEVENT_RESET,
-    QEVENT_POWERDOWN,
-    QEVENT_STOP,
-    QEVENT_RESUME,
-    QEVENT_VNC_CONNECTED,
-    QEVENT_VNC_INITIALIZED,
-    QEVENT_VNC_DISCONNECTED,
-    QEVENT_BLOCK_IO_ERROR,
-    QEVENT_RTC_CHANGE,
-    QEVENT_WATCHDOG,
-    QEVENT_SPICE_CONNECTED,
-    QEVENT_SPICE_INITIALIZED,
-    QEVENT_SPICE_DISCONNECTED,
-    QEVENT_BLOCK_JOB_COMPLETED,
-    QEVENT_BLOCK_JOB_CANCELLED,
-    QEVENT_BLOCK_JOB_ERROR,
-    QEVENT_BLOCK_JOB_READY,
-    QEVENT_DEVICE_DELETED,
-    QEVENT_DEVICE_TRAY_MOVED,
-    QEVENT_NIC_RX_FILTER_CHANGED,
-    QEVENT_SUSPEND,
-    QEVENT_SUSPEND_DISK,
-    QEVENT_WAKEUP,
-    QEVENT_BALLOON_CHANGE,
-    QEVENT_SPICE_MIGRATE_COMPLETED,
-    QEVENT_GUEST_PANICKED,
-    QEVENT_BLOCK_IMAGE_CORRUPTED,
+    Q_EVENT_SHUTDOWN,
+    Q_EVENT_RESET,
+    Q_EVENT_POWERDOWN,
+    Q_EVENT_STOP,
+    Q_EVENT_RESUME,
+    Q_EVENT_VNC_CONNECTED,
+    Q_EVENT_VNC_INITIALIZED,
+    Q_EVENT_VNC_DISCONNECTED,
+    Q_EVENT_BLOCK_IO_ERROR,
+    Q_EVENT_RTC_CHANGE,
+    Q_EVENT_WATCHDOG,
+    Q_EVENT_SPICE_CONNECTED,
+    Q_EVENT_SPICE_INITIALIZED,
+    Q_EVENT_SPICE_DISCONNECTED,
+    Q_EVENT_BLOCK_JOB_COMPLETED,
+    Q_EVENT_BLOCK_JOB_CANCELLED,
+    Q_EVENT_BLOCK_JOB_ERROR,
+    Q_EVENT_BLOCK_JOB_READY,
+    Q_EVENT_DEVICE_DELETED,
+    Q_EVENT_DEVICE_TRAY_MOVED,
+    Q_EVENT_NIC_RX_FILTER_CHANGED,
+    Q_EVENT_SUSPEND,
+    Q_EVENT_SUSPEND_DISK,
+    Q_EVENT_WAKEUP,
+    Q_EVENT_BALLOON_CHANGE,
+    Q_EVENT_SPICE_MIGRATE_COMPLETED,
+    Q_EVENT_GUEST_PANICKED,
+    Q_EVENT_BLOCK_IMAGE_CORRUPTED,
 
     /* Add to 'monitor_event_names' array in monitor.c when
      * defining new events here */
 
-    QEVENT_MAX,
+    Q_EVENT_MAX,
 } QEvent;
 
 int monitor_cur_is_qmp(void);
diff --git a/monitor.c b/monitor.c
index 9377834..b2c64de 100644
--- a/monitor.c
+++ b/monitor.c
@@ -479,38 +479,38 @@ static void timestamp_put(QDict *qdict)
 
 
 static const char *monitor_event_names[] = {
-    [QEVENT_SHUTDOWN] = "SHUTDOWN",
-    [QEVENT_RESET] = "RESET",
-    [QEVENT_POWERDOWN] = "POWERDOWN",
-    [QEVENT_STOP] = "STOP",
-    [QEVENT_RESUME] = "RESUME",
-    [QEVENT_VNC_CONNECTED] = "VNC_CONNECTED",
-    [QEVENT_VNC_INITIALIZED] = "VNC_INITIALIZED",
-    [QEVENT_VNC_DISCONNECTED] = "VNC_DISCONNECTED",
-    [QEVENT_BLOCK_IO_ERROR] = "BLOCK_IO_ERROR",
-    [QEVENT_RTC_CHANGE] = "RTC_CHANGE",
-    [QEVENT_WATCHDOG] = "WATCHDOG",
-    [QEVENT_SPICE_CONNECTED] = "SPICE_CONNECTED",
-    [QEVENT_SPICE_INITIALIZED] = "SPICE_INITIALIZED",
-    [QEVENT_SPICE_DISCONNECTED] = "SPICE_DISCONNECTED",
-    [QEVENT_BLOCK_JOB_COMPLETED] = "BLOCK_JOB_COMPLETED",
-    [QEVENT_BLOCK_JOB_CANCELLED] = "BLOCK_JOB_CANCELLED",
-    [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
-    [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
-    [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED",
-    [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
-    [QEVENT_NIC_RX_FILTER_CHANGED] = "NIC_RX_FILTER_CHANGED",
-    [QEVENT_SUSPEND] = "SUSPEND",
-    [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK",
-    [QEVENT_WAKEUP] = "WAKEUP",
-    [QEVENT_BALLOON_CHANGE] = "BALLOON_CHANGE",
-    [QEVENT_SPICE_MIGRATE_COMPLETED] = "SPICE_MIGRATE_COMPLETED",
-    [QEVENT_GUEST_PANICKED] = "GUEST_PANICKED",
-    [QEVENT_BLOCK_IMAGE_CORRUPTED] = "BLOCK_IMAGE_CORRUPTED",
+    [Q_EVENT_SHUTDOWN] = "SHUTDOWN",
+    [Q_EVENT_RESET] = "RESET",
+    [Q_EVENT_POWERDOWN] = "POWERDOWN",
+    [Q_EVENT_STOP] = "STOP",
+    [Q_EVENT_RESUME] = "RESUME",
+    [Q_EVENT_VNC_CONNECTED] = "VNC_CONNECTED",
+    [Q_EVENT_VNC_INITIALIZED] = "VNC_INITIALIZED",
+    [Q_EVENT_VNC_DISCONNECTED] = "VNC_DISCONNECTED",
+    [Q_EVENT_BLOCK_IO_ERROR] = "BLOCK_IO_ERROR",
+    [Q_EVENT_RTC_CHANGE] = "RTC_CHANGE",
+    [Q_EVENT_WATCHDOG] = "WATCHDOG",
+    [Q_EVENT_SPICE_CONNECTED] = "SPICE_CONNECTED",
+    [Q_EVENT_SPICE_INITIALIZED] = "SPICE_INITIALIZED",
+    [Q_EVENT_SPICE_DISCONNECTED] = "SPICE_DISCONNECTED",
+    [Q_EVENT_BLOCK_JOB_COMPLETED] = "BLOCK_JOB_COMPLETED",
+    [Q_EVENT_BLOCK_JOB_CANCELLED] = "BLOCK_JOB_CANCELLED",
+    [Q_EVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
+    [Q_EVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
+    [Q_EVENT_DEVICE_DELETED] = "DEVICE_DELETED",
+    [Q_EVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
+    [Q_EVENT_NIC_RX_FILTER_CHANGED] = "NIC_RX_FILTER_CHANGED",
+    [Q_EVENT_SUSPEND] = "SUSPEND",
+    [Q_EVENT_SUSPEND_DISK] = "SUSPEND_DISK",
+    [Q_EVENT_WAKEUP] = "WAKEUP",
+    [Q_EVENT_BALLOON_CHANGE] = "BALLOON_CHANGE",
+    [Q_EVENT_SPICE_MIGRATE_COMPLETED] = "SPICE_MIGRATE_COMPLETED",
+    [Q_EVENT_GUEST_PANICKED] = "GUEST_PANICKED",
+    [Q_EVENT_BLOCK_IMAGE_CORRUPTED] = "BLOCK_IMAGE_CORRUPTED",
 };
-QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
+QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != Q_EVENT_MAX)
 
-MonitorEventState monitor_event_state[QEVENT_MAX];
+MonitorEventState monitor_event_state[Q_EVENT_MAX];
 QemuMutex monitor_event_state_lock;
 
 /*
@@ -541,7 +541,7 @@ monitor_protocol_event_queue(QEvent event,
 {
     MonitorEventState *evstate;
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-    assert(event < QEVENT_MAX);
+    assert(event < Q_EVENT_MAX);
 
     qemu_mutex_lock(&monitor_event_state_lock);
     evstate = &(monitor_event_state[event]);
@@ -618,7 +618,7 @@ monitor_protocol_event_throttle(QEvent event,
                                 int64_t rate)
 {
     MonitorEventState *evstate;
-    assert(event < QEVENT_MAX);
+    assert(event < Q_EVENT_MAX);
 
     evstate = &(monitor_event_state[event]);
 
@@ -640,9 +640,9 @@ static void monitor_protocol_event_init(void)
 {
     qemu_mutex_init(&monitor_event_state_lock);
     /* Limit RTC & BALLOON events to 1 per second */
-    monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000);
-    monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000);
-    monitor_protocol_event_throttle(QEVENT_WATCHDOG, 1000);
+    monitor_protocol_event_throttle(Q_EVENT_RTC_CHANGE, 1000);
+    monitor_protocol_event_throttle(Q_EVENT_BALLOON_CHANGE, 1000);
+    monitor_protocol_event_throttle(Q_EVENT_WATCHDOG, 1000);
 }
 
 /**
@@ -655,7 +655,7 @@ void monitor_protocol_event(QEvent event, QObject *data)
     QDict *qmp;
     const char *event_name;
 
-    assert(event < QEVENT_MAX);
+    assert(event < Q_EVENT_MAX);
 
     event_name = monitor_event_names[event];
     assert(event_name != NULL);
@@ -1069,7 +1069,7 @@ EventInfoList *qmp_query_events(Error **errp)
     EventInfoList *info, *ev_list = NULL;
     QEvent e;
 
-    for (e = 0 ; e < QEVENT_MAX ; e++) {
+    for (e = 0 ; e < Q_EVENT_MAX ; e++) {
         const char *event_name = monitor_event_names[e];
         assert(event_name != NULL);
         info = g_malloc0(sizeof(*info));
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 02ac4ba..b09a7d2 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -736,7 +736,7 @@ static int handle_intercept(S390CPU *cpu)
                     QObject *data;
 
                     data = qobject_from_jsonf("{ 'action': %s }", "pause");
-                    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
+                    monitor_protocol_event(Q_EVENT_GUEST_PANICKED, data);
                     qobject_decref(data);
                     vm_stop(RUN_STATE_GUEST_PANICKED);
                 }
diff --git a/ui/spice-core.c b/ui/spice-core.c
index e4d533d..f3c6795 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -199,9 +199,9 @@ static void add_channel_info(QDict *dict, SpiceChannelEventInfo *info)
 static void channel_event(int event, SpiceChannelEventInfo *info)
 {
     static const int qevent[] = {
-        [ SPICE_CHANNEL_EVENT_CONNECTED    ] = QEVENT_SPICE_CONNECTED,
-        [ SPICE_CHANNEL_EVENT_INITIALIZED  ] = QEVENT_SPICE_INITIALIZED,
-        [ SPICE_CHANNEL_EVENT_DISCONNECTED ] = QEVENT_SPICE_DISCONNECTED,
+        [SPICE_CHANNEL_EVENT_CONNECTED]    = Q_EVENT_SPICE_CONNECTED,
+        [SPICE_CHANNEL_EVENT_INITIALIZED]  = Q_EVENT_SPICE_INITIALIZED,
+        [SPICE_CHANNEL_EVENT_DISCONNECTED] = Q_EVENT_SPICE_DISCONNECTED,
     };
     QDict *server, *client;
     QObject *data;
@@ -303,7 +303,7 @@ static void migrate_connect_complete_cb(SpiceMigrateInstance *sin)
 
 static void migrate_end_complete_cb(SpiceMigrateInstance *sin)
 {
-    monitor_protocol_event(QEVENT_SPICE_MIGRATE_COMPLETED, NULL);
+    monitor_protocol_event(Q_EVENT_SPICE_MIGRATE_COMPLETED, NULL);
     spice_migration_completed = true;
 }
 
diff --git a/ui/vnc.c b/ui/vnc.c
index 47fda54..b7c6f63 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1033,7 +1033,7 @@ void vnc_disconnect_finish(VncState *vs)
     vnc_jobs_join(vs); /* Wait encoding jobs */
 
     vnc_lock_output(vs);
-    vnc_qmp_event(vs, QEVENT_VNC_DISCONNECTED);
+    vnc_qmp_event(vs, Q_EVENT_VNC_DISCONNECTED);
 
     buffer_free(&vs->input);
     buffer_free(&vs->output);
@@ -2315,7 +2315,7 @@ static int protocol_client_init(VncState *vs, uint8_t *data, size_t len)
     vnc_flush(vs);
 
     vnc_client_cache_auth(vs);
-    vnc_qmp_event(vs, QEVENT_VNC_INITIALIZED);
+    vnc_qmp_event(vs, Q_EVENT_VNC_INITIALIZED);
 
     vnc_read_when(vs, protocol_client_msg, 1);
 
@@ -2819,7 +2819,7 @@ static void vnc_connect(VncDisplay *vd, int csock,
     }
 
     vnc_client_cache_addr(vs);
-    vnc_qmp_event(vs, QEVENT_VNC_CONNECTED);
+    vnc_qmp_event(vs, Q_EVENT_VNC_CONNECTED);
     vnc_set_share_mode(vs, VNC_SHARE_MODE_CONNECTING);
 
     vs->vd = vd;
diff --git a/vl.c b/vl.c
index b42ac67..6586072 100644
--- a/vl.c
+++ b/vl.c
@@ -746,7 +746,7 @@ void rtc_change_mon_event(struct tm *tm)
     QObject *data;
 
     data = qobject_from_jsonf("{ 'offset': %d }", qemu_timedate_diff(tm));
-    monitor_protocol_event(QEVENT_RTC_CHANGE, data);
+    monitor_protocol_event(Q_EVENT_RTC_CHANGE, data);
     qobject_decref(data);
 }
 
@@ -1699,7 +1699,7 @@ void vm_start(void)
         runstate_set(RUN_STATE_RUNNING);
         vm_state_notify(1, RUN_STATE_RUNNING);
         resume_all_vcpus();
-        monitor_protocol_event(QEVENT_RESUME, NULL);
+        monitor_protocol_event(Q_EVENT_RESUME, NULL);
     }
 }
 
@@ -1847,7 +1847,7 @@ void qemu_system_reset(bool report)
         qemu_devices_reset();
     }
     if (report) {
-        monitor_protocol_event(QEVENT_RESET, NULL);
+        monitor_protocol_event(Q_EVENT_RESET, NULL);
     }
     cpu_synchronize_all_post_reset();
 }
@@ -1868,7 +1868,7 @@ static void qemu_system_suspend(void)
     pause_all_vcpus();
     notifier_list_notify(&suspend_notifiers, NULL);
     runstate_set(RUN_STATE_SUSPENDED);
-    monitor_protocol_event(QEVENT_SUSPEND, NULL);
+    monitor_protocol_event(Q_EVENT_SUSPEND, NULL);
 }
 
 void qemu_system_suspend_request(void)
@@ -1929,7 +1929,7 @@ void qemu_system_shutdown_request(void)
 
 static void qemu_system_powerdown(void)
 {
-    monitor_protocol_event(QEVENT_POWERDOWN, NULL);
+    monitor_protocol_event(Q_EVENT_POWERDOWN, NULL);
     notifier_list_notify(&powerdown_notifiers, NULL);
 }
 
@@ -1967,7 +1967,7 @@ static bool main_loop_should_exit(void)
     }
     if (qemu_shutdown_requested()) {
         qemu_kill_report();
-        monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
+        monitor_protocol_event(Q_EVENT_SHUTDOWN, NULL);
         if (no_shutdown) {
             vm_stop(RUN_STATE_SHUTDOWN);
         } else {
@@ -1990,7 +1990,7 @@ static bool main_loop_should_exit(void)
         notifier_list_notify(&wakeup_notifiers, &wakeup_reason);
         wakeup_reason = QEMU_WAKEUP_REASON_NONE;
         resume_all_vcpus();
-        monitor_protocol_event(QEVENT_WAKEUP, NULL);
+        monitor_protocol_event(Q_EVENT_WAKEUP, NULL);
     }
     if (qemu_powerdown_requested()) {
         qemu_system_powerdown();
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

On 10/21/2013 03:16 AM, Wenchao Xia wrote:
> The define will be moved to qapi-schema.json later, so rename the
> prefix to match its naming style.

Wouldn't it be simpler to fix the code generator to special case QEvent
to turn into QEVENT, instead of having to go through this churn?  But if
we _like_ the Q_EVENT_ prefix, then this looks fairly mechanical:

> 
> Also fixed code style error reported in spice-core.c.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  balloon.c                 |    2 +-
>  block.c                   |    4 +-
>  block/qcow2-refcount.c    |    2 +-
>  blockdev.c                |    4 +-
>  blockjob.c                |    5 ++-
>  cpus.c                    |    2 +-
>  hw/acpi/core.c            |    2 +-
>  hw/core/qdev.c            |    2 +-
>  hw/misc/pvpanic.c         |    2 +-
>  hw/net/virtio-net.c       |    2 +-
>  hw/watchdog/watchdog.c    |    2 +-
>  include/monitor/monitor.h |   58 +++++++++++++++++-----------------
>  monitor.c                 |   74 ++++++++++++++++++++++----------------------
>  target-s390x/kvm.c        |    2 +-
>  ui/spice-core.c           |    8 ++--
>  ui/vnc.c                  |    6 ++--
>  vl.c                      |   14 ++++----
>  17 files changed, 96 insertions(+), 95 deletions(-)

If no one else is as opposed to the Q_EVENT naming as I seem to be, then
you can add my reluctant

Reviewed-by: Eric Blake <eblake@redhat.com>

in that your change is mechanical and correct.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Wenchao Xia]

于 2013/10/22 4:41, Eric Blake 写道:
> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
>> The define will be moved to qapi-schema.json later, so rename the
>> prefix to match its naming style.
> Wouldn't it be simpler to fix the code generator to special case QEvent
> to turn into QEVENT, instead of having to go through this churn?  But if
> we _like_ the Q_EVENT_ prefix, then this looks fairly mechanical:
>
>> Also fixed code style error reported in spice-core.c.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   balloon.c                 |    2 +-
>>   block.c                   |    4 +-
>>   block/qcow2-refcount.c    |    2 +-
>>   blockdev.c                |    4 +-
>>   blockjob.c                |    5 ++-
>>   cpus.c                    |    2 +-
>>   hw/acpi/core.c            |    2 +-
>>   hw/core/qdev.c            |    2 +-
>>   hw/misc/pvpanic.c         |    2 +-
>>   hw/net/virtio-net.c       |    2 +-
>>   hw/watchdog/watchdog.c    |    2 +-
>>   include/monitor/monitor.h |   58 +++++++++++++++++-----------------
>>   monitor.c                 |   74 ++++++++++++++++++++++----------------------
>>   target-s390x/kvm.c        |    2 +-
>>   ui/spice-core.c           |    8 ++--
>>   ui/vnc.c                  |    6 ++--
>>   vl.c                      |   14 ++++----
>>   17 files changed, 96 insertions(+), 95 deletions(-)
> If no one else is as opposed to the Q_EVENT naming as I seem to be, then
> you can add my reluctant
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> in that your change is mechanical and correct.
>
   Personally I think less special case is better, and there is no harm
to do a mechanical prefix change in code. Markus, can I have your opinion?
also cc qapi-types.py author Anthony.

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 21/10/2013 22:41, Eric Blake ha scritto:
> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
>> The define will be moved to qapi-schema.json later, so rename
>> the prefix to match its naming style.
> 
> Wouldn't it be simpler to fix the code generator to special case
> QEvent to turn into QEVENT, instead of having to go through this
> churn?  But if we _like_ the Q_EVENT_ prefix, then this looks
> fairly mechanical:

No, I disagree.  However, since something _is_ being renamed, we might
as well rename every QEVENT_ to MONITOR_EVENT_ and avoid patch 2.

Paolo

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSbj/xAAoJEBvWZb6bTYbyNkQP/R0l+qwakF5zBfwoiEc7Ru4r
YnG6B2HkCn7Bc5juDnZXYIvPdX1+n3x9tvGTcw2eP+8hiUQBmVGQEWNIlvOkPQoW
EtW7wzf0uRvROeYy0aqr5P+xU/0uDLonr3wONxv3aMc99CwZvJ918X889is5z4ps
P7CrMrnh9Ng99SgmkFgYS9KxwM004iIKyGyjKbYC+BYTKrJd7chzGfb41r687AbR
7CMb3gBCUvFRU2E2+yCdRI/cRVgRDSCMZ4Ic5VNK4DFKa6eq+yzP4zEhILkStl1o
yvPYlxpfHNLLle83iOb4rJFmBYz6mrAkVhfUOXdrKPegU6l+hod0vpuXcm3tMIO8
YJ9iMxeFrI2tFsIXVzQgugz4jaSY0BDapCP15aL9zqIzACiTpYZeaVden/gWu1Oy
iupuue9xzT2l40FCQwNXrGULOiOVRuiry8JstJ0A/valQi/xRy28cky+FI+PglNt
3eaj3GYK0oF1UyG3e22cFensIUOTWE1Znbm7/5ON12xcFOTE1e5bzvuPWUDOJ1wh
2I/p7Bn7uewQLstL14++gN1iMP+GCXJ0ce7xc0QtTP7jvtYCeTt+yBDC7jlan4Zb
N/CEISOti9obFYlDKiQL+Hdk4ivQsFjT2B1p7feGSd5JAZtLthNxBOSQrS4wg5fd
WW6UX7otKyj6MADQl3Jk
=Q+3Z
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Wenchao Xia]

于 2013/10/28 18:44, Paolo Bonzini 写道:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Il 21/10/2013 22:41, Eric Blake ha scritto:
>> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
>>> The define will be moved to qapi-schema.json later, so rename
>>> the prefix to match its naming style.
>> Wouldn't it be simpler to fix the code generator to special case
>> QEvent to turn into QEVENT, instead of having to go through this
>> churn?  But if we _like_ the Q_EVENT_ prefix, then this looks
>> fairly mechanical:
> No, I disagree.  However, since something _is_ being renamed, we might
> as well rename every QEVENT_ to MONITOR_EVENT_ and avoid patch 2.
>
> Paolo
>
   MONITOR_EVENT seems tide to monitor too much, since it will be present
in qapi-schema, I think Q_EVENT_ or QMP_EVENT_KIND would be better?

   I am coding v2, which fully support event define in qapi-schema. There
is another thing I hope to know your opinion:
   Should we support use enum as discriminator?

{ 'enum': 'QEvent',
   'data': [ 'SHUTDOWN', 'POWERDOWN']

{ 'type': 'QmpEventBase',
   'data': { 'event': 'QEvent', 'timestamp': 'EventTimestamp' } }

{ 'Union': 'QmpEvent',
   'base': 'QmpEventBase',
   'discriminator': 'event',
   'data': {
       'SHUTDOWN'               : 'EventShutdown',
       'POWERDOWN'              : 'EventPowerdown'
    }
}

   By default 'QmpEvent' will generate a hidden enum type 'QmpEventKind'.
but the hidden type define is needed by query-event, so there is two way
to archieve it:
1 just use the hidden type in qapi-schema.json.
2 modified the script to support use predefined enum type.
   In my draft code, both can work, but which one do you prefer?

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]

On 10/28/2013 11:22 PM, Wenchao Xia wrote:

>>
>   MONITOR_EVENT seems tide to monitor too much, since it will be present
> in qapi-schema, I think Q_EVENT_ or QMP_EVENT_KIND would be better?

I don't have a strong enough opinion on the bikeshed color.
MONITOR_EVENT implies the event is always associated with delivery over
a monitor; but how else would you receive an event without a monitor?

> 
>   I am coding v2, which fully support event define in qapi-schema. There
> is another thing I hope to know your opinion:
>   Should we support use enum as discriminator?
> 
> { 'enum': 'QEvent',
>   'data': [ 'SHUTDOWN', 'POWERDOWN']
> 
> { 'type': 'QmpEventBase',
>   'data': { 'event': 'QEvent', 'timestamp': 'EventTimestamp' } }
> 
> { 'Union': 'QmpEvent',
>   'base': 'QmpEventBase',
>   'discriminator': 'event',

I raised that question when Kevin first added discriminated unions; if I
recall, the answer was along the lines: "yes, it would be a nice
addition, but someone would have to code it, and we'd have to teach the
generator.py to loudly fail if all branches of the enum are not covered
in the union's data".  So go for it!

>   'data': {
>       'SHUTDOWN'               : 'EventShutdown',
>       'POWERDOWN'              : 'EventPowerdown'
>    }
> }
> 
>   By default 'QmpEvent' will generate a hidden enum type 'QmpEventKind'.
> but the hidden type define is needed by query-event, so there is two way
> to archieve it:
> 1 just use the hidden type in qapi-schema.json.
> 2 modified the script to support use predefined enum type.
>   In my draft code, both can work, but which one do you prefer?

I'd like to see the addition of an enum-typed discriminator, rather than
forcing the discriminator to be string-only.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Wenchao Xia]

于 2013/10/30 0:09, Eric Blake 写道:
> On 10/28/2013 11:22 PM, Wenchao Xia wrote:
>
>>    MONITOR_EVENT seems tide to monitor too much, since it will be present
>> in qapi-schema, I think Q_EVENT_ or QMP_EVENT_KIND would be better?
> I don't have a strong enough opinion on the bikeshed color.
> MONITOR_EVENT implies the event is always associated with delivery over
> a monitor; but how else would you receive an event without a monitor?
   Block code emit event already, when it is packaged as a library
it may need emit to other place. I think event doesn't have to be
bond to monitor, I can modify the qapi script to generate name correctly,
to avoid patch 2, Since it seems not right to have '_' between two capitals.


>>    I am coding v2, which fully support event define in qapi-schema. There
>> is another thing I hope to know your opinion:
>>    Should we support use enum as discriminator?
>>
>> { 'enum': 'QEvent',
>>    'data': [ 'SHUTDOWN', 'POWERDOWN']
>>
>> { 'type': 'QmpEventBase',
>>    'data': { 'event': 'QEvent', 'timestamp': 'EventTimestamp' } }
>>
>> { 'Union': 'QmpEvent',
>>    'base': 'QmpEventBase',
>>    'discriminator': 'event',
> I raised that question when Kevin first added discriminated unions; if I
> recall, the answer was along the lines: "yes, it would be a nice
> addition, but someone would have to code it, and we'd have to teach the
> generator.py to loudly fail if all branches of the enum are not covered
> in the union's data".  So go for it!
   OK, will send it soon.

>
>>    'data': {
>>        'SHUTDOWN'               : 'EventShutdown',
>>        'POWERDOWN'              : 'EventPowerdown'
>>     }
>> }
>>
>>    By default 'QmpEvent' will generate a hidden enum type 'QmpEventKind'.
>> but the hidden type define is needed by query-event, so there is two way
>> to archieve it:
>> 1 just use the hidden type in qapi-schema.json.
>> 2 modified the script to support use predefined enum type.
>>    In my draft code, both can work, but which one do you prefer?
> I'd like to see the addition of an enum-typed discriminator, rather than
> forcing the discriminator to be string-only.
>

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Luiz Capitulino]

On Tue, 29 Oct 2013 10:09:40 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 10/28/2013 11:22 PM, Wenchao Xia wrote:
> 
> >>
> >   MONITOR_EVENT seems tide to monitor too much, since it will be present
> > in qapi-schema, I think Q_EVENT_ or QMP_EVENT_KIND would be better?
> 
> I don't have a strong enough opinion on the bikeshed color.
> MONITOR_EVENT implies the event is always associated with delivery over
> a monitor; but how else would you receive an event without a monitor?

Today QMP is tied to the Monitor, but one of the goals of the QAPI is
to untangle them. This would allow for a separate QMP server, which
could have QMP only features like session support.

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 792 bytes --]

Am 21.10.2013 um 22:41 hat Eric Blake geschrieben:
> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
> > The define will be moved to qapi-schema.json later, so rename the
> > prefix to match its naming style.
> 
> Wouldn't it be simpler to fix the code generator to special case QEvent
> to turn into QEVENT, instead of having to go through this churn?  But if
> we _like_ the Q_EVENT_ prefix, then this looks fairly mechanical:

Or rather, instead of special casing QEvent, it shouldn't insert
underscores if there is nothing between the two capital letters.

I've had a similar case with AIO in the blockdev-add series; and while
renaming it to Aio worked, this kind of thing doesn't seem to be a rare
exception in practice, so it might be worth adjusting the generator.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Wenchao Xia]

于 2013/10/30 2:18, Kevin Wolf 写道:
> Am 21.10.2013 um 22:41 hat Eric Blake geschrieben:
>> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
>>> The define will be moved to qapi-schema.json later, so rename the
>>> prefix to match its naming style.
>> Wouldn't it be simpler to fix the code generator to special case QEvent
>> to turn into QEVENT, instead of having to go through this churn?  But if
>> we _like_ the Q_EVENT_ prefix, then this looks fairly mechanical:
> Or rather, instead of special casing QEvent, it shouldn't insert
> underscores if there is nothing between the two capital letters.
>
> I've had a similar case with AIO in the blockdev-add series; and while
> renaming it to Aio worked, this kind of thing doesn't seem to be a rare
> exception in practice, so it might be worth adjusting the generator.
>
> Kevin
It seems the right way, will adjust the generator.

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Paolo Bonzini]

Il 30/10/2013 08:27, Wenchao Xia ha scritto:
> 于 2013/10/30 2:18, Kevin Wolf 写道:
>> Am 21.10.2013 um 22:41 hat Eric Blake geschrieben:
>>> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
>>>> The define will be moved to qapi-schema.json later, so rename the
>>>> prefix to match its naming style.
>>> Wouldn't it be simpler to fix the code generator to special case QEvent
>>> to turn into QEVENT, instead of having to go through this churn?  But if
>>> we _like_ the Q_EVENT_ prefix, then this looks fairly mechanical:
>> Or rather, instead of special casing QEvent, it shouldn't insert
>> underscores if there is nothing between the two capital letters.

... but it should still insert one before the very last capital letter.
 For example AIOType should become AIO_TYPE, not AIOTYPE.

>> I've had a similar case with AIO in the blockdev-add series; and while
>> renaming it to Aio worked, this kind of thing doesn't seem to be a rare
>> exception in practice, so it might be worth adjusting the generator.
>
> It seems the right way, will adjust the generator.

I think even if you adjusted it, it would be a no-op in this case.  For
example:

    AIOType => AIO_TYPE
    QEvent  => Q_EVENT

Paolo

Re: [Qemu-devel] [PATCH 3/6] qapi: rename prefix QEVENT to Q_EVENT

[Wenchao Xia]

于 2013/10/30 19:55, Paolo Bonzini 写道:
> Il 30/10/2013 08:27, Wenchao Xia ha scritto:
>> 于 2013/10/30 2:18, Kevin Wolf 写道:
>>> Am 21.10.2013 um 22:41 hat Eric Blake geschrieben:
>>>> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
>>>>> The define will be moved to qapi-schema.json later, so rename the
>>>>> prefix to match its naming style.
>>>> Wouldn't it be simpler to fix the code generator to special case QEvent
>>>> to turn into QEVENT, instead of having to go through this churn?  But if
>>>> we _like_ the Q_EVENT_ prefix, then this looks fairly mechanical:
>>> Or rather, instead of special casing QEvent, it shouldn't insert
>>> underscores if there is nothing between the two capital letters.
>
> ... but it should still insert one before the very last capital letter.
>   For example AIOType should become AIO_TYPE, not AIOTYPE.
>
>>> I've had a similar case with AIO in the blockdev-add series; and while
>>> renaming it to Aio worked, this kind of thing doesn't seem to be a rare
>>> exception in practice, so it might be worth adjusting the generator.
>>
>> It seems the right way, will adjust the generator.
>
> I think even if you adjusted it, it would be a no-op in this case.  For
> example:
>
>      AIOType => AIO_TYPE
>      QEvent  => Q_EVENT
>
> Paolo
>

   Maybe check whether there is more than two continous capitalized char?

[Qemu-devel] [PATCH 4/6] qapi: move event defines to qapi-schema.json

[Wenchao Xia]

They are defined with capitals for compatibility, see monitor_event_names
in monitor.c, which was used to set the event strings.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 include/monitor/monitor.h |   37 -------------------------------------
 qapi-schema.json          |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index b273c5b..cdbb5b2 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -19,43 +19,6 @@ extern Monitor *default_mon;
 /* flags for monitor commands */
 #define MONITOR_CMD_ASYNC       0x0001
 
-/* QMP events */
-typedef enum QEvent {
-    Q_EVENT_SHUTDOWN,
-    Q_EVENT_RESET,
-    Q_EVENT_POWERDOWN,
-    Q_EVENT_STOP,
-    Q_EVENT_RESUME,
-    Q_EVENT_VNC_CONNECTED,
-    Q_EVENT_VNC_INITIALIZED,
-    Q_EVENT_VNC_DISCONNECTED,
-    Q_EVENT_BLOCK_IO_ERROR,
-    Q_EVENT_RTC_CHANGE,
-    Q_EVENT_WATCHDOG,
-    Q_EVENT_SPICE_CONNECTED,
-    Q_EVENT_SPICE_INITIALIZED,
-    Q_EVENT_SPICE_DISCONNECTED,
-    Q_EVENT_BLOCK_JOB_COMPLETED,
-    Q_EVENT_BLOCK_JOB_CANCELLED,
-    Q_EVENT_BLOCK_JOB_ERROR,
-    Q_EVENT_BLOCK_JOB_READY,
-    Q_EVENT_DEVICE_DELETED,
-    Q_EVENT_DEVICE_TRAY_MOVED,
-    Q_EVENT_NIC_RX_FILTER_CHANGED,
-    Q_EVENT_SUSPEND,
-    Q_EVENT_SUSPEND_DISK,
-    Q_EVENT_WAKEUP,
-    Q_EVENT_BALLOON_CHANGE,
-    Q_EVENT_SPICE_MIGRATE_COMPLETED,
-    Q_EVENT_GUEST_PANICKED,
-    Q_EVENT_BLOCK_IMAGE_CORRUPTED,
-
-    /* Add to 'monitor_event_names' array in monitor.c when
-     * defining new events here */
-
-    Q_EVENT_MAX,
-} QEvent;
-
 int monitor_cur_is_qmp(void);
 
 void monitor_protocol_event(QEvent event, QObject *data);
diff --git a/qapi-schema.json b/qapi-schema.json
index 60f3fd1..fbc1fab 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -29,6 +29,43 @@
             'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap' ] }
 
 ##
+# @QEvent
+#
+# QEMU event types
+#
+# Since: 1.8
+##
+{ 'enum': 'QEvent',
+  'data': [ 'SHUTDOWN',
+            'RESET',
+            'POWERDOWN',
+            'STOP',
+            'RESUME',
+            'VNC_CONNECTED',
+            'VNC_INITIALIZED',
+            'VNC_DISCONNECTED',
+            'BLOCK_IO_ERROR',
+            'RTC_CHANGE',
+            'WATCHDOG',
+            'SPICE_CONNECTED',
+            'SPICE_INITIALIZED',
+            'SPICE_DISCONNECTED',
+            'BLOCK_JOB_COMPLETED',
+            'BLOCK_JOB_CANCELLED',
+            'BLOCK_JOB_ERROR',
+            'BLOCK_JOB_READY',
+            'DEVICE_DELETED',
+            'DEVICE_TRAY_MOVED',
+            'NIC_RX_FILTER_CHANGED',
+            'SUSPEND',
+            'SUSPEND_DISK',
+            'WAKEUP',
+            'BALLOON_CHANGE',
+            'SPICE_MIGRATE_COMPLETED',
+            'GUEST_PANICKED',
+            'BLOCK_IMAGE_CORRUPTED' ] }
+
+##
 # @add_client
 #
 # Allow client connections for VNC, Spice and socket based
-- 
1.7.1

Re: [Qemu-devel] [PATCH 4/6] qapi: move event defines to qapi-schema.json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]

On 10/21/2013 03:16 AM, Wenchao Xia wrote:
> They are defined with capitals for compatibility, see monitor_event_names
> in monitor.c, which was used to set the event strings.

Agree; the existing 'query-events' commands returns all-capital strings.

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  include/monitor/monitor.h |   37 -------------------------------------
>  qapi-schema.json          |   37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 37 deletions(-)
> 
> -typedef enum QEvent {
> -    Q_EVENT_SHUTDOWN,
> -    Q_EVENT_RESET,
> -    Q_EVENT_POWERDOWN,
> -    Q_EVENT_STOP,
> -    Q_EVENT_RESUME,
> -    Q_EVENT_VNC_CONNECTED,
> -    Q_EVENT_VNC_INITIALIZED,
> -    Q_EVENT_VNC_DISCONNECTED,
> -    Q_EVENT_BLOCK_IO_ERROR,
> -    Q_EVENT_RTC_CHANGE,
> -    Q_EVENT_WATCHDOG,
> -    Q_EVENT_SPICE_CONNECTED,
> -    Q_EVENT_SPICE_INITIALIZED,
> -    Q_EVENT_SPICE_DISCONNECTED,
> -    Q_EVENT_BLOCK_JOB_COMPLETED,
> -    Q_EVENT_BLOCK_JOB_CANCELLED,
> -    Q_EVENT_BLOCK_JOB_ERROR,
> -    Q_EVENT_BLOCK_JOB_READY,
> -    Q_EVENT_DEVICE_DELETED,
> -    Q_EVENT_DEVICE_TRAY_MOVED,
> -    Q_EVENT_NIC_RX_FILTER_CHANGED,
> -    Q_EVENT_SUSPEND,
> -    Q_EVENT_SUSPEND_DISK,
> -    Q_EVENT_WAKEUP,
> -    Q_EVENT_BALLOON_CHANGE,
> -    Q_EVENT_SPICE_MIGRATE_COMPLETED,
> -    Q_EVENT_GUEST_PANICKED,
> -    Q_EVENT_BLOCK_IMAGE_CORRUPTED,
> -
> -    /* Add to 'monitor_event_names' array in monitor.c when
> -     * defining new events here */
> -
> -    Q_EVENT_MAX,
> -} QEvent;
> -
>  int monitor_cur_is_qmp(void);
>  
>  void monitor_protocol_event(QEvent event, QObject *data);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 60f3fd1..fbc1fab 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -29,6 +29,43 @@
>              'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap' ] }
>  
>  ##
> +# @QEvent

Again, see my comment in 3/6 wondering wheter we should fix the code
generation python to special-case QEVENT when converting this enum into
C code, rather than adding lots of churn.

I see why you split the documentation patch into 6/6 - to let this one
focus on just minimal code motion.
-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH 5/6] qapi: remove var monitor_event_names[]

[Wenchao Xia]

Use the generated table in qapi-types.h.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 monitor.c |   36 ++----------------------------------
 1 files changed, 2 insertions(+), 34 deletions(-)

diff --git a/monitor.c b/monitor.c
index b2c64de..7b8bd5e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -478,38 +478,6 @@ static void timestamp_put(QDict *qdict)
 }
 
 
-static const char *monitor_event_names[] = {
-    [Q_EVENT_SHUTDOWN] = "SHUTDOWN",
-    [Q_EVENT_RESET] = "RESET",
-    [Q_EVENT_POWERDOWN] = "POWERDOWN",
-    [Q_EVENT_STOP] = "STOP",
-    [Q_EVENT_RESUME] = "RESUME",
-    [Q_EVENT_VNC_CONNECTED] = "VNC_CONNECTED",
-    [Q_EVENT_VNC_INITIALIZED] = "VNC_INITIALIZED",
-    [Q_EVENT_VNC_DISCONNECTED] = "VNC_DISCONNECTED",
-    [Q_EVENT_BLOCK_IO_ERROR] = "BLOCK_IO_ERROR",
-    [Q_EVENT_RTC_CHANGE] = "RTC_CHANGE",
-    [Q_EVENT_WATCHDOG] = "WATCHDOG",
-    [Q_EVENT_SPICE_CONNECTED] = "SPICE_CONNECTED",
-    [Q_EVENT_SPICE_INITIALIZED] = "SPICE_INITIALIZED",
-    [Q_EVENT_SPICE_DISCONNECTED] = "SPICE_DISCONNECTED",
-    [Q_EVENT_BLOCK_JOB_COMPLETED] = "BLOCK_JOB_COMPLETED",
-    [Q_EVENT_BLOCK_JOB_CANCELLED] = "BLOCK_JOB_CANCELLED",
-    [Q_EVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
-    [Q_EVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
-    [Q_EVENT_DEVICE_DELETED] = "DEVICE_DELETED",
-    [Q_EVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
-    [Q_EVENT_NIC_RX_FILTER_CHANGED] = "NIC_RX_FILTER_CHANGED",
-    [Q_EVENT_SUSPEND] = "SUSPEND",
-    [Q_EVENT_SUSPEND_DISK] = "SUSPEND_DISK",
-    [Q_EVENT_WAKEUP] = "WAKEUP",
-    [Q_EVENT_BALLOON_CHANGE] = "BALLOON_CHANGE",
-    [Q_EVENT_SPICE_MIGRATE_COMPLETED] = "SPICE_MIGRATE_COMPLETED",
-    [Q_EVENT_GUEST_PANICKED] = "GUEST_PANICKED",
-    [Q_EVENT_BLOCK_IMAGE_CORRUPTED] = "BLOCK_IMAGE_CORRUPTED",
-};
-QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != Q_EVENT_MAX)
-
 MonitorEventState monitor_event_state[Q_EVENT_MAX];
 QemuMutex monitor_event_state_lock;
 
@@ -657,7 +625,7 @@ void monitor_protocol_event(QEvent event, QObject *data)
 
     assert(event < Q_EVENT_MAX);
 
-    event_name = monitor_event_names[event];
+    event_name = QEvent_lookup[event];
     assert(event_name != NULL);
 
     qmp = qdict_new();
@@ -1070,7 +1038,7 @@ EventInfoList *qmp_query_events(Error **errp)
     QEvent e;
 
     for (e = 0 ; e < Q_EVENT_MAX ; e++) {
-        const char *event_name = monitor_event_names[e];
+        const char *event_name = QEvent_lookup[e];
         assert(event_name != NULL);
         info = g_malloc0(sizeof(*info));
         info->value = g_malloc0(sizeof(*info->value));
-- 
1.7.1

Re: [Qemu-devel] [PATCH 5/6] qapi: remove var monitor_event_names[]

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

On 10/21/2013 03:16 AM, Wenchao Xia wrote:
> Use the generated table in qapi-types.h.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  monitor.c |   36 ++----------------------------------
>  1 files changed, 2 insertions(+), 34 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

(but may change if you rebase things to avoid churn in 3/5)

> @@ -1070,7 +1038,7 @@ EventInfoList *qmp_query_events(Error **errp)
>      QEvent e;
>  
>      for (e = 0 ; e < Q_EVENT_MAX ; e++) {
> -        const char *event_name = monitor_event_names[e];
> +        const char *event_name = QEvent_lookup[e];
>          assert(event_name != NULL);
>          info = g_malloc0(sizeof(*info));
>          info->value = g_malloc0(sizeof(*info->value));
> 

Remember this spot in patch 6/6...


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qapi-schema.json |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index fbc1fab..0f966ab 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -33,6 +33,62 @@
 #
 # QEMU event types
 #
+# @SHUTDOWN: system shutdown
+#
+# @RESET: system resets
+#
+# @POWERDOWN: system power down, if it is suppoted
+#
+# @STOP: stops the emulation
+#
+# @RESUME: resumes the emulation, typically after system stop
+#
+# @VNC_CONNECTED: a vnc client has connected to system
+#
+# @VNC_INITIALIZED: system has initialized for a vnc client
+#
+# @VNC_DISCONNECTED: a vnc client has disconnected from system
+#
+# @BLOCK_IO_ERROR: block layer meets I/O error
+#
+# @RTC_CHANGE: rtc changes
+#
+# @WATCHDOG: watch dog performs a action
+#
+# @SPICE_CONNECTED: a spice client has connected to system
+#
+# @SPICE_INITIALIZED: system has initialized for a spice client
+#
+# @SPICE_DISCONNECTED: a spice client has disconnected from system
+#
+# @BLOCK_JOB_COMPLETED: a block job has been completed
+#
+# @BLOCK_JOB_CANCELLED: a block job has been cancelled
+#
+# @BLOCK_JOB_ERROR: a block job meets error
+#
+# @BLOCK_JOB_READY: a block job is ready
+#
+# @DEVICE_DELETED: a device has been deleted
+#
+# @DEVICE_TRAY_MOVED: a device tray's status has changed
+#
+# @NIC_RX_FILTER_CHANGED: the filter for receiving on a nic has been changed
+#
+# @SUSPEND: system suspends, typically request comes from guest
+#
+# @SUSPEND_DISK: system suspends to disk, typically request comes from guest
+#
+# @WAKEUP: system wakes up from suspend
+#
+# @BALLOON_CHANGE: system resource balloon status changes
+#
+# @SPICE_MIGRATE_COMPLETED: spice migration has been completed
+#
+# @GUEST_PANICKED: guest has panicked
+#
+# @BLOCK_IMAGE_CORRUPTED: block image has been corrupted
+#
 # Since: 1.8
 ##
 { 'enum': 'QEvent',
-- 
1.7.1

Re: [Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3678 bytes --]

On 10/21/2013 03:16 AM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  qapi-schema.json |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)

Incomplete.  Now that you are actually using the enum (see the spot I
pointed out in 5/6), you ALSO need to change:

-{ 'type': 'EventInfo', 'data': {'name': 'str'} }
+{ 'type': 'EventInfo', 'data': {'name': 'QEvent'} }

and make use of the enum in the QAPI documentation.

>  #
> +# @SHUTDOWN: system shutdown
> +#
> +# @RESET: system resets

s/resets/has reset/

> +#
> +# @POWERDOWN: system power down, if it is suppoted

s/suppoted/supported/

Events aren't issued if they aren't supported, so that phrase is pointless.

> +#
> +# @STOP: stops the emulation
> +#

Your use of present tense makes it sounds like this is a causal command
("issuing STOP will stop the emulation"), but you really want it to
sound like a notification of an effect ("STOP is issued after emulation
is stopped).  That is:

s/stops the emulation/emulation stopped/

> +# @RESUME: resumes the emulation, typically after system stop

Again, tense matters; I suggest:

@RESUME: emulation resumed

> +#
> +# @VNC_CONNECTED: a vnc client has connected to system
> +#
> +# @VNC_INITIALIZED: system has initialized for a vnc client
> +#
> +# @VNC_DISCONNECTED: a vnc client has disconnected from system
> +#
> +# @BLOCK_IO_ERROR: block layer meets I/O error

s/meets/encountered an/

> +#
> +# @RTC_CHANGE: rtc changes

s/changes/changed/

> +#
> +# @WATCHDOG: watch dog performs a action

I suggest:

@WATCHDOG: watchdog expired

> +#
> +# @SPICE_CONNECTED: a spice client has connected to system
> +#
> +# @SPICE_INITIALIZED: system has initialized for a spice client
> +#
> +# @SPICE_DISCONNECTED: a spice client has disconnected from system
> +#
> +# @BLOCK_JOB_COMPLETED: a block job has been completed
> +#
> +# @BLOCK_JOB_CANCELLED: a block job has been cancelled
> +#
> +# @BLOCK_JOB_ERROR: a block job meets error

s/meets/encountered an/

> +#
> +# @BLOCK_JOB_READY: a block job is ready
> +#
> +# @DEVICE_DELETED: a device has been deleted
> +#
> +# @DEVICE_TRAY_MOVED: a device tray's status has changed
> +#
> +# @NIC_RX_FILTER_CHANGED: the filter for receiving on a nic has been changed
> +#
> +# @SUSPEND: system suspends, typically request comes from guest

No need to say the typical cause for an event here; I'd much rather see
us give that extra detail in the place where we further describe each
event (more on that later).  I suggest:

@SUSPEND: system has suspended to memory (S3 power state)

> +#
> +# @SUSPEND_DISK: system suspends to disk, typically request comes from guest

I suggest:

@SUSPEND_DISK: system has suspended to disk (S4 power state)

> +#
> +# @WAKEUP: system wakes up from suspend

s/wakes/woke/

> +#
> +# @BALLOON_CHANGE: system resource balloon status changes

s/changes/changed/

> +#
> +# @SPICE_MIGRATE_COMPLETED: spice migration has been completed
> +#
> +# @GUEST_PANICKED: guest has panicked
> +#
> +# @BLOCK_IMAGE_CORRUPTED: block image has been corrupted
> +#
>  # Since: 1.8
>  ##
>  { 'enum': 'QEvent',
> 

Good start, but this series needs more.  Ultimately, I'd like to get rid
of docs/qmp/qmp-events.txt, and inline that content into
qapi-schema.json.  We already documented how to do it:

https://lists.gnu.org/archive/html/qemu-devel/2013-09/msg02164.html

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Wenchao Xia]

于 2013/10/22 5:00, Eric Blake 写道:
> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   qapi-schema.json |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 56 insertions(+), 0 deletions(-)
> Incomplete.  Now that you are actually using the enum (see the spot I
> pointed out in 5/6), you ALSO need to change:
>
> -{ 'type': 'EventInfo', 'data': {'name': 'str'} }
> +{ 'type': 'EventInfo', 'data': {'name': 'QEvent'} }
>
> and make use of the enum in the QAPI documentation.
>
  Will add this part, thanks for tipping it.


>>   #
>> +# @SHUTDOWN: system shutdown
>> +#
>> +# @RESET: system resets
> s/resets/has reset/
>
>> +#
>> +# @POWERDOWN: system power down, if it is suppoted
> s/suppoted/supported/
>
> Events aren't issued if they aren't supported, so that phrase is pointless.
   Ok, I will skip that phrase. The point here is that, many people are 
confused
about shutdown and powerdown, and it seems POWERDOWN item is not present
in doc/qmp/qapi-events.txt? I want to add doc tips the difference:
How about: It will set the system power control unit to notify guest, 
such as
ACPI chips.(This is where I am not sure, the qemu online doc says, 
shutdown is
gracefully....).

>> +#
>> +# @STOP: stops the emulation
>> +#
> Your use of present tense makes it sounds like this is a causal command
> ("issuing STOP will stop the emulation"), but you really want it to
> sound like a notification of an effect ("STOP is issued after emulation
> is stopped).  That is:
>
> s/stops the emulation/emulation stopped/
   Do you mean all tense in the doc should use past tense?
I hesitated before about tense usage, it seems not all event
is emitted after it happens, for example, powerdown emitted before
it call notifier to set the states.
   Take another think, I think I may use past tense through the doc,
but with more carefully meaning, such as:
the system has enter powerdown state.
   If you agree with the tense, I'd like sent the reformed doc
in the following, before respin.

>> +# @RESUME: resumes the emulation, typically after system stop
> Again, tense matters; I suggest:
>
> @RESUME: emulation resumed
>
>> +#
>> +# @VNC_CONNECTED: a vnc client has connected to system
>> +#
>> +# @VNC_INITIALIZED: system has initialized for a vnc client
>> +#
>> +# @VNC_DISCONNECTED: a vnc client has disconnected from system
>> +#
>> +# @BLOCK_IO_ERROR: block layer meets I/O error
> s/meets/encountered an/
>
>> +#
>> +# @RTC_CHANGE: rtc changes
> s/changes/changed/
>
>> +#
>> +# @WATCHDOG: watch dog performs a action
> I suggest:
>
> @WATCHDOG: watchdog expired
   OK.

>> +#
>> +# @SPICE_CONNECTED: a spice client has connected to system
>> +#
>> +# @SPICE_INITIALIZED: system has initialized for a spice client
>> +#
>> +# @SPICE_DISCONNECTED: a spice client has disconnected from system
>> +#
>> +# @BLOCK_JOB_COMPLETED: a block job has been completed
>> +#
>> +# @BLOCK_JOB_CANCELLED: a block job has been cancelled
>> +#
>> +# @BLOCK_JOB_ERROR: a block job meets error
> s/meets/encountered an/
>
>> +#
>> +# @BLOCK_JOB_READY: a block job is ready
>> +#
>> +# @DEVICE_DELETED: a device has been deleted
>> +#
>> +# @DEVICE_TRAY_MOVED: a device tray's status has changed
>> +#
>> +# @NIC_RX_FILTER_CHANGED: the filter for receiving on a nic has been changed
>> +#
>> +# @SUSPEND: system suspends, typically request comes from guest
> No need to say the typical cause for an event here; I'd much rather see
> us give that extra detail in the place where we further describe each
> event (more on that later).  I suggest:
>
> @SUSPEND: system has suspended to memory (S3 power state)
>
>> +#
>> +# @SUSPEND_DISK: system suspends to disk, typically request comes from guest
> I suggest:
>
> @SUSPEND_DISK: system has suspended to disk (S4 power state)
>
>> +#
>> +# @WAKEUP: system wakes up from suspend
> s/wakes/woke/
>
>> +#
>> +# @BALLOON_CHANGE: system resource balloon status changes
> s/changes/changed/
>
>> +#
>> +# @SPICE_MIGRATE_COMPLETED: spice migration has been completed
>> +#
>> +# @GUEST_PANICKED: guest has panicked
>> +#
>> +# @BLOCK_IMAGE_CORRUPTED: block image has been corrupted
>> +#
>>   # Since: 1.8
>>   ##
>>   { 'enum': 'QEvent',
>>
> Good start, but this series needs more.  Ultimately, I'd like to get rid
> of docs/qmp/qmp-events.txt, and inline that content into
> qapi-schema.json.  We already documented how to do it:
>
> https://lists.gnu.org/archive/html/qemu-devel/2013-09/msg02164.html
   I'll take a look to find a good way inline those content.

Re: [Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2512 bytes --]

On 10/22/2013 04:19 AM, Wenchao Xia wrote:

>>> +#
>>> +# @POWERDOWN: system power down, if it is suppoted
>> s/suppoted/supported/
>>
>> Events aren't issued if they aren't supported, so that phrase is
>> pointless.
>   Ok, I will skip that phrase. The point here is that, many people are
> confused
> about shutdown and powerdown, and it seems POWERDOWN item is not present
> in doc/qmp/qapi-events.txt?

That's a bug in the existing docs, then :)

> I want to add doc tips the difference:
> How about: It will set the system power control unit to notify guest,
> such as
> ACPI chips.(This is where I am not sure, the qemu online doc says,
> shutdown is
> gracefully....).

Based on patch 3/6, the SHUTDOWN event is triggered if
qemu_shutdown_requested() is called, and the POWERDOWN event is issued
if qemu_shutdown_requested() is issued.  I'm fuzzy myself on the
conditions behind those two requests, maybe someone else can chime in.

>>> +#
>>> +# @STOP: stops the emulation
>>> +#
>> Your use of present tense makes it sounds like this is a causal command
>> ("issuing STOP will stop the emulation"), but you really want it to
>> sound like a notification of an effect ("STOP is issued after emulation
>> is stopped).  That is:
>>
>> s/stops the emulation/emulation stopped/
>   Do you mean all tense in the doc should use past tense?

Yes.

> I hesitated before about tense usage, it seems not all event
> is emitted after it happens, for example, powerdown emitted before
> it call notifier to set the states.

Implementation wise, we may issue some events before the action.  But
events are asynchronous by nature, we cannot guarantee that management
reads the event from QMP in any particular order, and in MOST cases,
management won't know about the event until AFTER we have done the
action, even if we queued the event for delivery on the front end of the
action.  It's better to just document ALL events as generically being
past tense.

>   Take another think, I think I may use past tense through the doc,
> but with more carefully meaning, such as:
> the system has enter powerdown state.
>   If you agree with the tense, I'd like sent the reformed doc
> in the following, before respin.
> 

Indeed, which is why separating the docs from the refactoring made sense
in your series, so that we could hammer out good docs.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Wenchao Xia]

>>    Take another think, I think I may use past tense through the doc,
>> but with more carefully meaning, such as:
>> the system has enter powerdown state.
>>    If you agree with the tense, I'd like sent the reformed doc
>> in the following, before respin.
>>
> Indeed, which is why separating the docs from the refactoring made sense
> in your series, so that we could hammer out good docs.
>
>
Hi, here is my draft for qapi-schema.json, please have a look.
Note:
1 it requires directly support of 'base', so I will sent additonal patch
support it by key word '_base' in 'data' contents.
2 some define not labeled with "since 1.8', are code move.
3 reordered.


##
# @QEvent
#
# QEMU event types
#
# Since: 1.8
##
{ 'enum': 'QEvent',
   'data': [
# system related
             'SHUTDOWN',
             'POWERDOWN',
             'RESET',
             'STOP',
             'RESUME',
             'SUSPEND',
             'SUSPEND_DISK',
             'WAKEUP',

# system virtual device related
             'RTC_CHANGE',
             'WATCHDOG',
             'DEVICE_DELETED',
             'DEVICE_TRAY_MOVED',

# block related
             'BLOCK_IO_ERROR',
             'BLOCK_IMAGE_CORRUPTED',

# block job related
             'BLOCK_JOB_COMPLETED',
             'BLOCK_JOB_CANCELLED',
             'BLOCK_JOB_ERROR',
             'BLOCK_JOB_READY',

# network related
             'NIC_RX_FILTER_CHANGED',

# vnc display related
             'VNC_CONNECTED',
             'VNC_INITIALIZED',
             'VNC_DISCONNECTED',

# spice display related
             'SPICE_CONNECTED',
             'SPICE_INITIALIZED',
             'SPICE_DISCONNECTED',
             'SPICE_MIGRATE_COMPLETED',

# guest related
             'BALLOON_CHANGE',
             'GUEST_PANICKED' ] }

##
# @EventTimestamp
#
# Reflect the time when event happens
#
# @seconds: the seconds have passed on host system
#
# @microsecnds: the microseconds have passed on host system
#
# Since: 1.8
##
{ 'type': 'EventTimestamp',
   'data': { 'seconds': 'int', 'microsecnds': 'int' } }

##
# @EventBase
#
# Base type containing properties that are available for all kind of events
#
# @event: type of the event
#
# @timestamp: the time stamp of the event, which is got from host system
#
# Since: 1.8
##
{ 'type': 'EventBase',
   'data': { 'event': 'QEvent', 'timestamp': 'EventTimestamp' } }


##
# @EventShutdown
#
# Emitted when the virtual machine shutdown, qemu terminates the 
emulation and
# exit. If the command-line option "-no-shutdown" has been specified, 
qemu will
# not exit, and a STOP event will eventually follow the SHUTDOWN event
#
# Since: 1.8
##
{ 'type': 'EventShutdown',
   'data': { } }

##
# @EventPowerdown
#
# Emitted when the virtual machine is powered down, qemu tries to set the
# system power control system, such as ACPI related virtual chips
#
# Since: 1.8
##
{ 'type': 'EventPowerdown',
   'data': { } }

##
# @EventReset
#
# Emitted when the virtual machine is reseted
#
# Since: 1.8
##
{ 'type': 'EventReset',
   'data': { } }

##
# @EventStop
#
# Emitted when the virtual machine is stopped
#
# Since: 1.8
##
{ 'type': 'EventStop',
   'data': { } }

##
# @EventResume
#
# Emitted when the virtual machine resumes execution
#
# Since: 1.8
##
{ 'type': 'EventResume',
   'data': { } }


##
# @EventSuspend
#
# Emitted when guest enters S3 state
#
# Since: 1.8
##
{ 'type': 'EventSuspend',
   'data': { } }

##
# @EventSuspendDisk
#
# Emitted when the guest makes a request to enter S4 state. Note QEMU shuts
# down (similar to @shutdown) when entering S4 state
#
# Since: 1.8
##
{ 'type': 'EventSuspendDisk',
   'data': { } }

##
# @EventWakeup
#
# Emitted when the guest has woken up from S3 and is running
#
# Since: 1.8
##
{ 'type': 'EventWakeup',
   'data': { } }


##
# @EventRtcChange
#
# Emitted when the guest changes the RTC time
#
# @offset: Offset between base RTC clock (as specified by -rtc base), and
#          new RTC clock value
#
# Since: 1.8
##
{ 'type': 'EventRtcChange',
   'data': {
       'data': {
           'offset': 'int'
       } } }

##
# @ActionTypeOnWatchdogExpired
#
# An enumeration of the actions taken when the watchdog device's timer is
# expired
#
# @reset: system resets
#
# @shutdown: system shutdown, note that it is similar to @powerdown, which
#            tries to set to system status and notify guest
#
# @poweroff: system poweroff, the emulator program exits
#
# @pause: system pauses, similar to @stop
#
# @pause: system enters debug state
#
# @none: nothing is done
#
# Since: 1.8
##
{ 'enum': 'ActionTypeOnWatchdogExpired',
   'data': [ 'reset', 'shutdown', 'poweroff', 'pause', 'debug', 'none' ] }

##
# @EventWatchdog
#
# Emitted when the watchdog device's timer is expired
#
# @action: Action that has been taken
#
# Since: 1.8
##
{ 'type': 'Watchdog',
   'data': {
       'data': {
           'action': 'ActionTypeOnWatchdogExpired'
       } } }

##
# @EventDeviceDeleted
#
# Emitted whenever the device removal completion is acknowledged by the 
guest.
# At this point, it's safe to reuse the specified device ID. Device 
removal can
# be initiated by the guest or by HMP/QMP commands.
#
# @device: #optional, device name
#
# @path: device path
#
# Since: 1.8
##
{ 'type': 'EventDeviceDeleted',
   'data': {
       'data': {
           '*device': 'str',
           'path'   : 'str'
       } } }

##
# @EventTrayMoved
#
# It's emitted whenever the tray of a removable device is moved by the guest
# or by HMP/QMP commands
#
# @device: device name
#
# @tray-open: true if the tray has been opened or false if it has been 
closed
#
# Since: 1.8
##
{ 'type': 'EventTrayMoved',
   'data': {
       'data': {
           'device'   : 'str',
           'tray-open': 'bool'
       } } }


##
# @IoOperationType
#
# An enumeration of the I/O operation types
#
# @read: read operation
#
# @write: write operation
#
# Since: 1.8
##
{ 'enum': 'IoOperationType',
   'data': [ 'read', 'write' ] }

##
# @BlockdevOnError:
#
# An enumeration of possible behaviors for errors on I/O operations.
# The exact meaning depends on whether the I/O was initiated by a guest
# or by a block job
#
# @report: for guest operations, report the error to the guest;
#          for jobs, cancel the job
#
# @ignore: ignore the error, only report a QMP event (BLOCK_IO_ERROR
#          or BLOCK_JOB_ERROR)
#
# @enospc: same as @stop on ENOSPC, same as @report otherwise.
#
# @stop: for guest operations, stop the virtual machine;
#        for jobs, pause the job
#
# Since: 1.3
##
{ 'enum': 'BlockdevOnError',
   'data': ['report', 'ignore', 'enospc', 'stop'] }

##
# @BlockErrorInfo
#
# The error info for a block error
#
# @device: device name
#
# @operation: I/O operation
#
# @action: action that has been taken
#
# Since: 1.8
##
{ 'type': 'BlockErrorInfo',
   'data': { 'device': 'str', 'operation': 'IoOperationType',
             'action': 'BlockdevOnError' } }

##
# @EventBlockIoError
#
# Emitted when a disk I/O error occurs
#
# Since: 1.8
##
{ 'type': 'EventBlockIoError',
   'data': {
       'data': {
           '_base': 'BlockErrorInfo'
       } } }

##
# @EventBlockImageCorrupted
#
# Emitted when a disk image is being marked corrupt
#
# @device: Device name
#
# @msg: Informative message, for example, reason for the corruption
#
# @offset: If the corruption resulted from an image access, this is the 
access
#          offset into the image
# @size: If the corruption resulted from an image access, this is the access
#        size
#
# Since: 1.8
##
{ 'type': 'EventBlockImageCorrupted',
   'data': {
       'data': {
           'device': 'str',
           'msg'   : 'str',
           'offset': 'int',
           'size'  : 'int'
       } } }


##
# @BlockJobType:
#
# Type of a block job.
#
# @commit: block commit job type, see "block-commit"
#
# @stream: block stream job type, see "block-stream"
#
# @mirror: drive mirror job type, see "drive-mirror"
#
# @backup: drive backup job type, see "drive-backup"
#
# Since: 1.7
##
{ 'enum': 'BlockJobType',
   'data': ['commit', 'stream', 'mirror', 'backup'] }

##
# @BlockJobInfoBase
#
# Basic info of a block job
#
# @type: Job type
#
# @device: Device name
#
# @len: Maximum progress value
#
# @offset: Current progress value. On success this is equal to len.
#          On failure this is less than len
#
# @speed: Rate limit, bytes per second
#
# Since: 1.8
##
{ 'type': 'BlockJobInfoBase',
   'data': { 'type': 'BlockJobType', 'device': 'str', 'len': 'int',
             'offset': 'int', 'speed': 'int' } }

##
# @EventBlockJobCompleted
#
# Emitted when a block job has completed
#
# @error: #optional, error message. Only present on failure. This field
#         contains a human-readable error message. There are no semantics
#         other than that streaming has failed and clients should not try to
#         interpret the error string
#
# Since: 1.8
##
{ 'type': 'EventBlockJobCompleted',
   'data': {
       'data': {
           '_base' : 'BlockJobInfoBase',
           '*error': 'str'
       } } }

##
# @EventBlockJobCancelled
#
# Emitted when a block job has been cancelled
#
# @error: #optional, error message. Only present on failure. This field
#         contains a human-readable error message. There are no semantics
#         other than that streaming has failed and clients should not try to
#         interpret the error string
#
# Since: 1.8
##
{ 'type': 'EventBlockJobCancelled',
   'data': {
       'data': {
           '_base': 'BlockJobInfoBase'
       } } }

##
# @EventBlockJobError
#
# Emitted when a block job encounters an error
#
# Since: 1.8
##
{ 'type': 'EventBlockJobError',
   'data': {
       'data': {
           '_base': 'BlockErrorInfo'
       } } }

##
# @EventBlockJobReady
#
# Emitted when a block job is ready to complete
#
# @device: device name
#
# Since: 1.8
##
{ 'type': 'EventBlockJobReady',
   'data': {
       'data': {
           'device': 'str'
       } } }

##
# @EventNicRxFilterChanged
#
# The event is emitted once until the query command is executed, the first
# event will always be emitted
#
# @name: net client name
#
# @path: device path
#
# Since: 1.8
##
{ 'type': 'EventNicRxFilterChanged',
   'data': {
       'data': {
           'name': 'str',
           'path': 'str'
       } } }


##
# @NetworkConnectionInfo
#
# The information for network connection
#
# @host: IP address
#
# @service: port number
#
# @family: address family, "ipv4" or "ipv6"
#
# Since: 1.8
##
{ 'type': 'NetworkConnectionInfo',
   'data': { 'host': 'str', 'service': 'str', 'family': 'str' } }

##
# @EventVncConnected
#
# Emitted when a VNC client establishes a connection
#
# @server: Server information
#
#   @auth: #optional, authentication method
#
# @client: Client information
#
# Since: 1.8
##
{ 'type': 'EventVncConnected',
   'data': {
       'data': {
           'server': {
               '_base': 'NetworkConnectionInfo',
               '*auth': 'str' },
           'client': 'NetworkConnectionInfo'
       } } }

##
# @EventVncInitialized
#
# Emitted after authentication takes place (if any) and the VNC session is
# made active
#
# @server: Server information
#
#   @auth: #optional, authentication method
#
# @client: Client information
#
#   @x509_dname: #optional, TLS dname
#
#   @sasl_username: #optional, SASL username
#
# Since: 1.8
##
{ 'type': 'EventVncInitialized',
   'data': {
       'data': {
           'server': {
               '_base': 'NetworkConnectionInfo',
               '*auth': 'str' },
           'client': {
               '_base'         : 'NetworkConnectionInfo',
               '*x509_dname'   : 'str',
               '*sasl_username': 'str' }
       } } }

##
# @EventVncDisconnected
#
# Emitted when the connection is closed
#
# @server: Server information
#
#   @auth: #optional, authentication method
#
# @client: Client information
#
#   @x509_dname: #optional, TLS dname
#
#   @sasl_username: #optional, SASL username
#
# Since: 1.8
##
{ 'type': 'EventVncDisconnected',
   'data': {
       'data': {
           'server': {
               '_base': 'NetworkConnectionInfo',
               '*auth': 'str' },
           'client': {
               '_base'         : 'NetworkConnectionInfo',
               '*x509_dname'   : 'str',
               '*sasl_username': 'str' }
       } } }


##
# @EventSpiceConnected
#
# Emitted when a Spice client establishes a connection
#
# @server: Server information
#
# @client: Client information
#
# Since: 1.8
##
{ 'type': 'EventSpiceConnected',
   'data': {
       'data': {
           'server': 'NetworkConnectionInfo',
           'client': 'NetworkConnectionInfo'
       } } }

##
# @EventSpiceInitialized
#
# Emitted after initial handshake and authentication takes place (if any)
# and the SPICE channel is up'n'running
#
# @server: Server information
#
#   @auth: #optional, authentication method
#
# @client: Client information
#
#   @connection-id: spice connection id. All channels with the same id
#                   belong to the same spice session
#
#   @channel-type: channel type. "1" is the main control channel, filter for
#                  this one if you want track spice sessions only
#
#   @channel-id: channel id. Usually "0", might be different needed when
#                multiple channels of the same type exist, such as multiple
#                display channels in a multihead setup
#
#   @tls: whevener the channel is encrypted
#
# Since: 1.8
##
{ 'type': 'EventSpiceInitialized',
   'data': {
       'data': {
           'server': {
               '_base': 'NetworkConnectionInfo',
               '*auth': 'str' },
           'client': {
               '_base'        : 'NetworkConnectionInfo',
               'connection-id': 'int',
               'channel-type' : 'int',
               'channel-id'   : 'int',
               'tls'          : 'bool' }
       } } }

##
# @EventSpiceDisconnected
#
# Emitted when the spice connection is closed
#
# @server: Server information
#
# @client: Client information
#
# Since: 1.8
##
{ 'type': 'EventSpiceDisconnected',
   'data': {
       'data': {
           'server': 'NetworkConnectionInfo',
           'client': 'NetworkConnectionInfo'
       } } }

##
# @EventSpiceMigrateCompleted
#
# Emitted when spice migration has completed
#
# Since: 1.8
##
{ 'type': 'EventSpiceMigrateCompleted',
   'data': { } }


##
# @EventBalloonChange
#
# Emitted when the guest changes the actual BALLOON level. This value is
# equivalent to the @actual field return by the 'query-balloon' command
#
# @actual: actual level of the guest memory balloon in bytes
#
# Since: 1.8
##
{ 'type': 'EventBalloonChange',
   'data': {
       'data': {
           'actual': 'int'
       } } }

##
# @EventGuestPanicked
#
# Emitted when guest OS panic is detected
#
# @action: Action that has been taken, currently always "pause"
#
# Since: 1.8
##
{ 'type': 'EventGuestPanicked',
   'data': {
       'data': {
           'action': 'str'
       } } }


##
# @Event
#
# Emitted when an event happens
#
# Since: 1.8
##
{ 'Union': 'Event',
   'base': 'EventBase',
   'discriminator': 'event',
   'data': {
       'SHUTDOWN'               : 'EventShutdown',
       'POWERDOWN'              : 'EventPowerdown',
       'RESET'                  : 'EventReset',
       'STOP'                   : 'EventStop',
       'RESUME'                 : 'EventResume',
       'SUSPEND'                : 'EventSuspend',
       'SUSPEND_DISK'           : 'EventSuspendDisk',
       'WAKEUP'                 : 'EventWakeup',

       'RTC_CHANGE'             : 'EventRtcChange',
       'WATCHDOG'               : 'EventWatchdog',
       'DEVICE_DELETED'         : 'EventDeviceDeleted',
       'DEVICE_TRAY_MOVED'      : 'EventDeviceTrayMoved',

       'BLOCK_IO_ERROR'         : 'EventBlockIoError',
       'BLOCK_IMAGE_CORRUPTED'  : 'EventBlockImageCorrupted',

       'BLOCK_JOB_COMPLETED'    : 'EventBlockJobCompleted',
       'BLOCK_JOB_CANCELLED'    : 'EventBlockJobCancelled',
       'BLOCK_JOB_ERROR'        : 'EventBlockJobError',
       'BLOCK_JOB_READY'        : 'EventBlockJobReady',

       'NIC_RX_FILTER_CHANGED'  : 'EventNicRxFilterChanged',

       'VNC_CONNECTED'          : 'EventVncConnected',
       'VNC_INITIALIZED'        : 'EventVncInitialized',
       'VNC_DISCONNECTED'       : 'EventVncDisconnected',

       'SPICE_CONNECTED'        : 'EventSpiceConnected',
       'SPICE_INITIALIZED'      : 'EventSpiceInitialized',
       'SPICE_DISCONNECTED'     : 'EventSpiceDisconnected',
       'SPICE_MIGRATE_COMPLETED': 'EventSpiceMigrateCompleted',

       'BALLOON_CHANGE'         : 'EventBalloonChange',
       'GUEST_PANICKED'         : 'EventGuestPanicked'
   } }

Re: [Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 11925 bytes --]

On 10/22/2013 06:37 PM, Wenchao Xia wrote:
> Hi, here is my draft for qapi-schema.json, please have a look.
> Note:
> 1 it requires directly support of 'base', so I will sent additonal patch
> support it by key word '_base' in 'data' contents.
> 2 some define not labeled with "since 1.8', are code move.
> 3 reordered.

You may find it easier to break this into multiple patches (in
particular, code motion patches are almost always easier to review when
done separate from the patch that changes contents).

> ##
> { 'enum': 'QEvent',
>   'data': [
> # system related
>             'SHUTDOWN',
>             'POWERDOWN',
>             'RESET',
>             'STOP',
>             'RESUME',
>             'SUSPEND',
>             'SUSPEND_DISK',
>             'WAKEUP',
> 
> # system virtual device related
>             'RTC_CHANGE',

I like how you grouped events.

> 
> ##
> # @EventTimestamp
> #
> # Reflect the time when event happens
> #
> # @seconds: the seconds have passed on host system
> #
> # @microsecnds: the microseconds have passed on host system

s/microsecnds/microseconds/

> #
> # Since: 1.8
> ##
> { 'type': 'EventTimestamp',
>   'data': { 'seconds': 'int', 'microsecnds': 'int' } }

s/microsecnds/microseconds/

> 
> ##
> # @EventBase
> #
> # Base type containing properties that are available for all kind of events
> #
> # @event: type of the event
> #
> # @timestamp: the time stamp of the event, which is got from host system

Grammar is awkward, and you are missing a reference point; maybe:

@timestamp: time stamp when the event was issued by the host system, in
seconds since the Epoch

> 
> ##
> # @EventShutdown
> #
> # Emitted when the virtual machine shutdown, qemu terminates the
> emulation and

Line wrapping looks awkward here and elsewhere in your patch; be careful
that you fit in a reasonable column width.

s/terminates/terminated/

> # exit. If the command-line option "-no-shutdown" has been specified,

s/exit/is about to exit/

> qemu will
> # not exit, and a STOP event will eventually follow the SHUTDOWN event
> #
> # Since: 1.8
> ##
> { 'type': 'EventShutdown',
>   'data': { } }
> 
> ##
> # @EventPowerdown
> #
> # Emitted when the virtual machine is powered down, qemu tries to set the
> # system power control system, such as ACPI related virtual chips

Hmm, this one is undocumented in qmp-events.txt.  Maybe:

Emitted when the virtual machine is powered down through the system
power control system, such as via ACPI.

Do we have a better idea of how EventShutdown and EventPowerdown differ?

> #
> # Since: 1.8
> ##
> { 'type': 'EventPowerdown',
>   'data': { } }
> 
> ##
> # @EventReset
> #
> # Emitted when the virtual machine is reseted

s/reseted/reset/

> ##
> # @EventSuspend
> #
> # Emitted when guest enters S3 state

Is S3 an x86-specific term, or does it apply to other architectures?  I
know this is what qmp-events.txt says, but maybe it would be better as
"Emitted when guest enters a hardware suspension state (such as S3)".

> #
> # Since: 1.8
> ##
> { 'type': 'EventSuspend',
>   'data': { } }
> 
> ##
> # @EventSuspendDisk
> #
> # Emitted when the guest makes a request to enter S4 state. Note QEMU shuts
> # down (similar to @shutdown) when entering S4 state

Again, is S4 an x86-specific term?  The reference to '@shutdown' looks
awkward; should it just call out 'EventShutdown' instead?

> #
> # Since: 1.8
> ##
> { 'type': 'EventSuspendDisk',
>   'data': { } }
> 
> ##
> # @EventWakeup
> #
> # Emitted when the guest has woken up from S3 and is running

Again a question on S3 wording.

> 
> ##
> # @ActionTypeOnWatchdogExpired

That's a mouthful.  How about:

WatchdogExpirationAction

> #
> # An enumeration of the actions taken when the watchdog device's timer is
> # expired
> #
> # @reset: system resets
> #
> # @shutdown: system shutdown, note that it is similar to @powerdown, which
> #            tries to set to system status and notify guest
> #
> # @poweroff: system poweroff, the emulator program exits
> #
> # @pause: system pauses, similar to @stop
> #
> # @pause: system enters debug state

s/pause/debug/

> ##
> # @EventTrayMoved
> #
> # It's emitted whenever the tray of a removable device is moved by the

s/It's emitted/Emitted/

> 
> ##
> # @BlockdevOnError:

Again, separate code motion from new content.  qapi-schema.json does not
have to be in topological order (ie. we already allow for recursive
types, so there's no need to worry whether all intermediate types are
declared prior to the outer type that uses them).

> ##
> { 'type': 'BlockErrorInfo',
>   'data': { 'device': 'str', 'operation': 'IoOperationType',
>             'action': 'BlockdevOnError' } }
> 
> ##
> # @EventBlockIoError
> #
> # Emitted when a disk I/O error occurs
> #
> # Since: 1.8
> ##
> { 'type': 'EventBlockIoError',
>   'data': {
>       'data': {
>           '_base': 'BlockErrorInfo'

Huh?  I think you mean:

{ 'type': 'EventBlockIoError',
  'data': {
    'data': 'BlockErrorInfo'
  } }

> ##
> # @EventBlockImageCorrupted
> #
> # Emitted when a disk image is being marked corrupt
> #
> # @device: Device name
> #
> # @msg: Informative message, for example, reason for the corruption
> #
> # @offset: If the corruption resulted from an image access, this is the
> access
> #          offset into the image
> # @size: If the corruption resulted from an image access, this is the
> access
> #        size
> #
> # Since: 1.8
> ##
> { 'type': 'EventBlockImageCorrupted',
>   'data': {
>       'data': {
>           'device': 'str',
>           'msg'   : 'str',
>           'offset': 'int',
>           'size'  : 'int'

Double-check if 'offset' and 'size' are always present, or if they
should be written '*offset' and '*size'.

> ##
> # @BlockJobInfoBase
> #
> # Basic info of a block job
> #
> # @type: Job type
> #
> # @device: Device name
> #
> # @len: Maximum progress value
> #
> # @offset: Current progress value. On success this is equal to len.
> #          On failure this is less than len
> #
> # @speed: Rate limit, bytes per second
> #
> # Since: 1.8
> ##
> { 'type': 'BlockJobInfoBase',
>   'data': { 'type': 'BlockJobType', 'device': 'str', 'len': 'int',
>             'offset': 'int', 'speed': 'int' } }

I would fold '*error':'str' into this type, rename BlockJobInfoBase to
BlockJobEventInfo (so it is not confused with the existing BlockJobInfo,
and because it is not a base type), then...

> 
> ##
> # @EventBlockJobCompleted
> #
> # Emitted when a block job has completed
> #
> # @error: #optional, error message. Only present on failure. This field
> #         contains a human-readable error message. There are no semantics
> #         other than that streaming has failed and clients should not
> try to
> #         interpret the error string
> #
> # Since: 1.8
> ##
> { 'type': 'EventBlockJobCompleted',
>   'data': {
>       'data': {
>           '_base' : 'BlockJobInfoBase',
>           '*error': 'str'
>       } } }

...fix this to avoid a '_base' by just using:

{ 'type': 'EventBlockJobCompleted',
  'data': {
    'data': 'BlockJobEventInfo'
  } }
...

> 
> ##
> # @EventBlockJobCancelled
> #
> # Emitted when a block job has been cancelled
> #
> # @error: #optional, error message. Only present on failure. This field
> #         contains a human-readable error message. There are no semantics
> #         other than that streaming has failed and clients should not
> try to
> #         interpret the error string
> #
> # Since: 1.8
> ##
> { 'type': 'EventBlockJobCancelled',
>   'data': {
>       'data': {
>           '_base': 'BlockJobInfoBase'
>       } } }

...and do the same here (maybe with a doc that the optional '*error'
will never be populated):

{ 'type': 'EventBlockJobCancelled',
  'data': {
    'data': 'BlockJobEventInfo'
  } }

> 
> ##
> # @EventBlockJobError
> #
> # Emitted when a block job encounters an error
> #
> # Since: 1.8
> ##
> { 'type': 'EventBlockJobError',
>   'data': {
>       'data': {
>           '_base': 'BlockErrorInfo'
>       } } }

Again, don't use '_base', but just properly inline the
'data':'BlockErrorInfo'.

> 
> ##
> # @EventNicRxFilterChanged
> #
> # The event is emitted once until the query command is executed, the first

s/query/'query-rx-filter'/

> # event will always be emitted
> #

> ##
> # @NetworkConnectionInfo
> #
> # The information for network connection
> #
> # @host: IP address
> #
> # @service: port number
> #
> # @family: address family, "ipv4" or "ipv6"

Seems like a candidate for an enum rather than an open-coded string.

> ##
> { 'type': 'EventVncConnected',
>   'data': {
>       'data': {
>           'server': {
>               '_base': 'NetworkConnectionInfo',
>               '*auth': 'str' },

Doesn't work like that.  I think you want:

{ 'type': 'NetworkConnectionInfoServer',
  'base': 'NetworkConnectionInfo',
  'data': { '*auth': 'str'} }

followed by:

{ 'type': 'EventVncConnected',
  'data': {
    'data': {
      'server': 'NetworkConnectionInfoServer',
      'client': 'NetworkConnectionInfo'
    } } }

> ##
> # @EventVncInitialized
> #
> # Emitted after authentication takes place (if any) and the VNC session is
> # made active
> #
> # @server: Server information
> #
> #   @auth: #optional, authentication method
> #
> # @client: Client information
> #
> #   @x509_dname: #optional, TLS dname
> #
> #   @sasl_username: #optional, SASL username
> #
> # Since: 1.8
> ##
> { 'type': 'EventVncInitialized',
>   'data': {
>       'data': {
>           'server': {
>               '_base': 'NetworkConnectionInfo',
>               '*auth': 'str' },
>           'client': {
>               '_base'         : 'NetworkConnectionInfo',
>               '*x509_dname'   : 'str',
>               '*sasl_username': 'str' }

Again, _base doesn't work.  You'll need to create a struct containing
the optional members (but can be a child class of
NetworkConnectionInfo), then use direct reference to that struct, as in
'client':'NetworkConnectionInfoXYZ'.

> ##
> # @EventSpiceInitialized
> #
> # Emitted after initial handshake and authentication takes place (if any)
> # and the SPICE channel is up'n'running

s/up'n'running/up and running/

> ##
> { 'type': 'EventSpiceInitialized',
>   'data': {
>       'data': {
>           'server': {
>               '_base': 'NetworkConnectionInfo',
>               '*auth': 'str' },
>           'client': {
>               '_base'        : 'NetworkConnectionInfo',
>               'connection-id': 'int',
>               'channel-type' : 'int',
>               'channel-id'   : 'int',
>               'tls'          : 'bool' }

More uses of '_base' that instead need to be actual child structs
deriving from the common base.

> 
> ##
> # @EventGuestPanicked
> #
> # Emitted when guest OS panic is detected
> #
> # @action: Action that has been taken, currently always "pause"

Sounds like it should be an enum rather than a 'str'

> ##
> # @Event
> #
> # Emitted when an event happens
> #
> # Since: 1.8
> ##
> { 'Union': 'Event',

s/Union/union/

>   'base': 'EventBase',
>   'discriminator': 'event',
>   'data': {
>       'SHUTDOWN'               : 'EventShutdown',
>       'POWERDOWN'              : 'EventPowerdown',
...
>       'BALLOON_CHANGE'         : 'EventBalloonChange',
>       'GUEST_PANICKED'         : 'EventGuestPanicked'
>   } }

Looks like you're headed in the right direction.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Wenchao Xia]

于 2013/10/30 7:02, Eric Blake 写道:
> On 10/22/2013 06:37 PM, Wenchao Xia wrote:
>> Hi, here is my draft for qapi-schema.json, please have a look.
>> Note:
>> 1 it requires directly support of 'base', so I will sent additonal patch
>> support it by key word '_base' in 'data' contents.
>> 2 some define not labeled with "since 1.8', are code move.
>> 3 reordered.
> You may find it easier to break this into multiple patches (in
> particular, code motion patches are almost always easier to review when
> done separate from the patch that changes contents).
>
>> ##
>> { 'enum': 'QEvent',
>>    'data': [
>> # system related
>>              'SHUTDOWN',
>>              'POWERDOWN',
>>              'RESET',
>>              'STOP',
>>              'RESUME',
>>              'SUSPEND',
>>              'SUSPEND_DISK',
>>              'WAKEUP',
>>
>> # system virtual device related
>>              'RTC_CHANGE',
> I like how you grouped events.
>
>> ##
>> # @EventTimestamp
>> #
>> # Reflect the time when event happens
>> #
>> # @seconds: the seconds have passed on host system
>> #
>> # @microsecnds: the microseconds have passed on host system
> s/microsecnds/microseconds/
   Will fix.

>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventTimestamp',
>>    'data': { 'seconds': 'int', 'microsecnds': 'int' } }
> s/microsecnds/microseconds/
>
>> ##
>> # @EventBase
>> #
>> # Base type containing properties that are available for all kind of events
>> #
>> # @event: type of the event
>> #
>> # @timestamp: the time stamp of the event, which is got from host system
> Grammar is awkward, and you are missing a reference point; maybe:
>
> @timestamp: time stamp when the event was issued by the host system, in
> seconds since the Epoch
   Will doc as above.

>> ##
>> # @EventShutdown
>> #
>> # Emitted when the virtual machine shutdown, qemu terminates the
>> emulation and
> Line wrapping looks awkward here and elsewhere in your patch; be careful
> that you fit in a reasonable column width.
>
> s/terminates/terminated/
   OK.

>> # exit. If the command-line option "-no-shutdown" has been specified,
> s/exit/is about to exit/
   OK.

>> qemu will
>> # not exit, and a STOP event will eventually follow the SHUTDOWN event
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventShutdown',
>>    'data': { } }
>>
>> ##
>> # @EventPowerdown
>> #
>> # Emitted when the virtual machine is powered down, qemu tries to set the
>> # system power control system, such as ACPI related virtual chips
> Hmm, this one is undocumented in qmp-events.txt.  Maybe:
>
> Emitted when the virtual machine is powered down through the system
> power control system, such as via ACPI.
   Will use above.

> Do we have a better idea of how EventShutdown and EventPowerdown differ?
>
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventPowerdown',
>>    'data': { } }
>>
>> ##
>> # @EventReset
>> #
>> # Emitted when the virtual machine is reseted
> s/reseted/reset/
   OK.

>> ##
>> # @EventSuspend
>> #
>> # Emitted when guest enters S3 state
> Is S3 an x86-specific term, or does it apply to other architectures?  I
> know this is what qmp-events.txt says, but maybe it would be better as
> "Emitted when guest enters a hardware suspension state (such as S3)".
   I am not sure if other arch have S3 term, will use as:

"Emitted when guest enters a hardware suspension state (such as S3 on x86)"


>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventSuspend',
>>    'data': { } }
>>
>> ##
>> # @EventSuspendDisk
>> #
>> # Emitted when the guest makes a request to enter S4 state. Note QEMU shuts
>> # down (similar to @shutdown) when entering S4 state
> Again, is S4 an x86-specific term?  The reference to '@shutdown' looks
> awkward; should it just call out 'EventShutdown' instead?
   yes, it should be 'EventShutdown'

>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventSuspendDisk',
>>    'data': { } }
>>
>> ##
>> # @EventWakeup
>> #
>> # Emitted when the guest has woken up from S3 and is running
> Again a question on S3 wording.
   will reword:

'Emitted when the guest has woken up from suspension state'



>> ##
>> # @ActionTypeOnWatchdogExpired
> That's a mouthful.  How about:
>
> WatchdogExpirationAction
   OK.

>> #
>> # An enumeration of the actions taken when the watchdog device's timer is
>> # expired
>> #
>> # @reset: system resets
>> #
>> # @shutdown: system shutdown, note that it is similar to @powerdown, which
>> #            tries to set to system status and notify guest
>> #
>> # @poweroff: system poweroff, the emulator program exits
>> #
>> # @pause: system pauses, similar to @stop
>> #
>> # @pause: system enters debug state
> s/pause/debug/
>
>> ##
>> # @EventTrayMoved
>> #
>> # It's emitted whenever the tray of a removable device is moved by the
> s/It's emitted/Emitted/
   Will fix.

>> ##
>> # @BlockdevOnError:
> Again, separate code motion from new content.  qapi-schema.json does not
> have to be in topological order (ie. we already allow for recursive
> types, so there's no need to worry whether all intermediate types are
> declared prior to the outer type that uses them).
   Nice to hear no topological limit present, but put the define ahead
my make it easier to review? will use seprate patch to do it.

>> ##
>> { 'type': 'BlockErrorInfo',
>>    'data': { 'device': 'str', 'operation': 'IoOperationType',
>>              'action': 'BlockdevOnError' } }
>>
>> ##
>> # @EventBlockIoError
>> #
>> # Emitted when a disk I/O error occurs
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventBlockIoError',
>>    'data': {
>>        'data': {
>>            '_base': 'BlockErrorInfo'
> Huh?  I think you mean:
>
> { 'type': 'EventBlockIoError',
>    'data': {
>      'data': 'BlockErrorInfo'
>    } }
   Yes, I misse it.

>> ##
>> # @EventBlockImageCorrupted
>> #
>> # Emitted when a disk image is being marked corrupt
>> #
>> # @device: Device name
>> #
>> # @msg: Informative message, for example, reason for the corruption
>> #
>> # @offset: If the corruption resulted from an image access, this is the
>> access
>> #          offset into the image
>> # @size: If the corruption resulted from an image access, this is the
>> access
>> #        size
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventBlockImageCorrupted',
>>    'data': {
>>        'data': {
>>            'device': 'str',
>>            'msg'   : 'str',
>>            'offset': 'int',
>>            'size'  : 'int'
> Double-check if 'offset' and 'size' are always present, or if they
> should be written '*offset' and '*size'.
   The original doc says always present, But I think it is resonable to 
change
it as optional, do you agree?

>> ##
>> # @BlockJobInfoBase
>> #
>> # Basic info of a block job
>> #
>> # @type: Job type
>> #
>> # @device: Device name
>> #
>> # @len: Maximum progress value
>> #
>> # @offset: Current progress value. On success this is equal to len.
>> #          On failure this is less than len
>> #
>> # @speed: Rate limit, bytes per second
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'BlockJobInfoBase',
>>    'data': { 'type': 'BlockJobType', 'device': 'str', 'len': 'int',
>>              'offset': 'int', 'speed': 'int' } }
> I would fold '*error':'str' into this type, rename BlockJobInfoBase to
> BlockJobEventInfo (so it is not confused with the existing BlockJobInfo,
> and because it is not a base type), then...
>> ##
>> # @EventBlockJobCompleted
>> #
>> # Emitted when a block job has completed
>> #
>> # @error: #optional, error message. Only present on failure. This field
>> #         contains a human-readable error message. There are no semantics
>> #         other than that streaming has failed and clients should not
>> try to
>> #         interpret the error string
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventBlockJobCompleted',
>>    'data': {
>>        'data': {
>>            '_base' : 'BlockJobInfoBase',
>>            '*error': 'str'
>>        } } }
> ...fix this to avoid a '_base' by just using:
>
> { 'type': 'EventBlockJobCompleted',
>    'data': {
>      'data': 'BlockJobEventInfo'
>    } }
> ...
   OK.

>> ##
>> # @EventBlockJobCancelled
>> #
>> # Emitted when a block job has been cancelled
>> #
>> # @error: #optional, error message. Only present on failure. This field
>> #         contains a human-readable error message. There are no semantics
>> #         other than that streaming has failed and clients should not
>> try to
>> #         interpret the error string
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventBlockJobCancelled',
>>    'data': {
>>        'data': {
>>            '_base': 'BlockJobInfoBase'
>>        } } }
> ...and do the same here (maybe with a doc that the optional '*error'
> will never be populated):
>
> { 'type': 'EventBlockJobCancelled',
>    'data': {
>      'data': 'BlockJobEventInfo'
>    } }
   OK.

>> ##
>> # @EventBlockJobError
>> #
>> # Emitted when a block job encounters an error
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventBlockJobError',
>>    'data': {
>>        'data': {
>>            '_base': 'BlockErrorInfo'
>>        } } }
> Again, don't use '_base', but just properly inline the
> 'data':'BlockErrorInfo'.
   OK.

>> ##
>> # @EventNicRxFilterChanged
>> #
>> # The event is emitted once until the query command is executed, the first
> s/query/'query-rx-filter'/
   will fix.

>> # event will always be emitted
>> #
>> ##
>> # @NetworkConnectionInfo
>> #
>> # The information for network connection
>> #
>> # @host: IP address
>> #
>> # @service: port number
>> #
>> # @family: address family, "ipv4" or "ipv6"
> Seems like a candidate for an enum rather than an open-coded string.
   OK, will use enum.

>> ##
>> { 'type': 'EventVncConnected',
>>    'data': {
>>        'data': {
>>            'server': {
>>                '_base': 'NetworkConnectionInfo',
>>                '*auth': 'str' },
> Doesn't work like that.  I think you want:
>
> { 'type': 'NetworkConnectionInfoServer',
>    'base': 'NetworkConnectionInfo',
>    'data': { '*auth': 'str'} }
>
> followed by:
>
> { 'type': 'EventVncConnected',
>    'data': {
>      'data': {
>        'server': 'NetworkConnectionInfoServer',
>        'client': 'NetworkConnectionInfo'
>      } } }
>
>> ##
>> # @EventVncInitialized
>> #
>> # Emitted after authentication takes place (if any) and the VNC session is
>> # made active
>> #
>> # @server: Server information
>> #
>> #   @auth: #optional, authentication method
>> #
>> # @client: Client information
>> #
>> #   @x509_dname: #optional, TLS dname
>> #
>> #   @sasl_username: #optional, SASL username
>> #
>> # Since: 1.8
>> ##
>> { 'type': 'EventVncInitialized',
>>    'data': {
>>        'data': {
>>            'server': {
>>                '_base': 'NetworkConnectionInfo',
>>                '*auth': 'str' },
>>            'client': {
>>                '_base'         : 'NetworkConnectionInfo',
>>                '*x509_dname'   : 'str',
>>                '*sasl_username': 'str' }
> Again, _base doesn't work.  You'll need to create a struct containing
> the optional members (but can be a child class of
> NetworkConnectionInfo), then use direct reference to that struct, as in
> 'client':'NetworkConnectionInfoXYZ'.
   We will have a lot of struct defines, not sure if it is good:

NetworkConnectionInfoVNCConnectedServer
NetworkConnectionInfoVNCInitializedServer
NetworkConnectionInfoVNCInitializedClient
NetworkConnectionInfoSPICEConnectedServer
......

   Instead of above, I modified qapi script a bit to recognize keyword 
"_base",
which directly inherit data field, just as "base".

>> ##
>> # @EventSpiceInitialized
>> #
>> # Emitted after initial handshake and authentication takes place (if any)
>> # and the SPICE channel is up'n'running
> s/up'n'running/up and running/

   OK.

>> ##
>> { 'type': 'EventSpiceInitialized',
>>    'data': {
>>        'data': {
>>            'server': {
>>                '_base': 'NetworkConnectionInfo',
>>                '*auth': 'str' },
>>            'client': {
>>                '_base'        : 'NetworkConnectionInfo',
>>                'connection-id': 'int',
>>                'channel-type' : 'int',
>>                'channel-id'   : 'int',
>>                'tls'          : 'bool' }
> More uses of '_base' that instead need to be actual child structs
> deriving from the common base.
>
>> ##
>> # @EventGuestPanicked
>> #
>> # Emitted when guest OS panic is detected
>> #
>> # @action: Action that has been taken, currently always "pause"
> Sounds like it should be an enum rather than a 'str'
   OK.

>> ##
>> # @Event
>> #
>> # Emitted when an event happens
>> #
>> # Since: 1.8
>> ##
>> { 'Union': 'Event',
> s/Union/union/
   typo mistake.

>>    'base': 'EventBase',
>>    'discriminator': 'event',
>>    'data': {
>>        'SHUTDOWN'               : 'EventShutdown',
>>        'POWERDOWN'              : 'EventPowerdown',
> ...
>>        'BALLOON_CHANGE'         : 'EventBalloonChange',
>>        'GUEST_PANICKED'         : 'EventGuestPanicked'
>>    } }
> Looks like you're headed in the right direction.
>

Re: [Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Wenchao Xia]

于 2013/10/22 5:00, Eric Blake 写道:
> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   qapi-schema.json |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 56 insertions(+), 0 deletions(-)
> Incomplete.  Now that you are actually using the enum (see the spot I
> pointed out in 5/6), you ALSO need to change:
>
> -{ 'type': 'EventInfo', 'data': {'name': 'str'} }
> +{ 'type': 'EventInfo', 'data': {'name': 'QEvent'} }
>
> and make use of the enum in the QAPI documentation.
>
   I just found QEvent is a int(enum) so this change can't be done.
Is there a way to tell use QEvent's correspond string? Add a new
way to specify enum's correspond string automatically? for example:

{ 'type': 'EventInfo', 'data': {'name': '&QEvent'}

Re: [Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Wenchao Xia]

于 2013/10/22 14:55, Wenchao Xia 写道:
> 于 2013/10/22 5:00, Eric Blake 写道:
>> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   qapi-schema.json |   56 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 files changed, 56 insertions(+), 0 deletions(-)
>> Incomplete.  Now that you are actually using the enum (see the spot I
>> pointed out in 5/6), you ALSO need to change:
>>
>> -{ 'type': 'EventInfo', 'data': {'name': 'str'} }
>> +{ 'type': 'EventInfo', 'data': {'name': 'QEvent'} }
>>
>> and make use of the enum in the QAPI documentation.
>>
>   I just found QEvent is a int(enum) so this change can't be done.
> Is there a way to tell use QEvent's correspond string? Add a new
> way to specify enum's correspond string automatically? for example:
>
> { 'type': 'EventInfo', 'data': {'name': '&QEvent'}
>
   I just found visitor will do it for me automatically, so it is not
a problem, sorry for the disturbing messsage.

Re: [Qemu-devel] [PATCH 6/6] qapi: add doc for QEvent

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]

On 10/22/2013 07:55 AM, Wenchao Xia wrote:
> 于 2013/10/22 5:00, Eric Blake 写道:
>> On 10/21/2013 03:16 AM, Wenchao Xia wrote:
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   qapi-schema.json |   56
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 files changed, 56 insertions(+), 0 deletions(-)
>> Incomplete.  Now that you are actually using the enum (see the spot I
>> pointed out in 5/6), you ALSO need to change:
>>
>> -{ 'type': 'EventInfo', 'data': {'name': 'str'} }
>> +{ 'type': 'EventInfo', 'data': {'name': 'QEvent'} }
>>
>> and make use of the enum in the QAPI documentation.
>>
>   I just found QEvent is a int(enum) so this change can't be done.

Huh?  That's the whole point of qapi enums - they are stored as int
constants in C code, but sent as strings over QMP.

> Is there a way to tell use QEvent's correspond string? Add a new
> way to specify enum's correspond string automatically? for example:
> 
> { 'type': 'EventInfo', 'data': {'name': '&QEvent'}

No, QAPI does it for you already, no change in syntax needed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 0/6] qapi: generate event defines automatically

[Wenchao Xia]

Hi, Markus
I am coding V2 which support event in qapi-schema, and just remember
it is on your TODO list. Is it OK to let me implement it instead as V2?

Re: [Qemu-devel] [PATCH 0/6] qapi: generate event defines automatically

[Luiz Capitulino]

On Mon, 21 Oct 2013 10:15:59 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> This series move the event define to qapi code, so later other components
> could use it easily, it also make monitor code less and easier to decouple
> with other code.

Yes, this is an improvement over the current code. But it doesn't move
in the direction we (or better Anthony) originally had for events in
the QAPI.

Basically, *iirc*, the idea was to have an event type, so that we could
declare events as:

{ 'event': 'BLOCK_IO_ERROR', 'data': { 'device': 'str',
                                       'operation': 'str',
                                       'action': 'str' } }

 (Note that keys 'operation' and 'action' should be enums)

Then the QAPI could generate C functions to register and de-register from
an event. This way C code could benefit from events too, and we could
also allow QMP clients to register/de-register to/from events.

Maybe we could apply this series as a first step, but I can't tell if
later on we'll regret it due to compatibility issues or if we'll realize
it was unneeded churn.

Re: [Qemu-devel] [PATCH 0/6] qapi: generate event defines automatically

[Wenchao Xia]

于 2013/11/1 22:28, Luiz Capitulino 写道:
> On Mon, 21 Oct 2013 10:15:59 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> This series move the event define to qapi code, so later other components
>> could use it easily, it also make monitor code less and easier to decouple
>> with other code.
>
> Yes, this is an improvement over the current code. But it doesn't move
> in the direction we (or better Anthony) originally had for events in
> the QAPI.
>
> Basically, *iirc*, the idea was to have an event type, so that we could
> declare events as:
>
> { 'event': 'BLOCK_IO_ERROR', 'data': { 'device': 'str',
>                                         'operation': 'str',
>                                         'action': 'str' } }
>
>   (Note that keys 'operation' and 'action' should be enums)
>
> Then the QAPI could generate C functions to register and de-register from
> an event. This way C code could benefit from events too, and we could
> also allow QMP clients to register/de-register to/from events.
>
> Maybe we could apply this series as a first step, but I can't tell if
> later on we'll regret it due to compatibility issues or if we'll realize
> it was unneeded churn.
>

   I am coding a version to fullly support event in qmp schema, will send
it soon.