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

2013-11-05 Thread Luiz Capitulino
On Tue, 05 Nov 2013 13:31:09 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:

 于 2013/11/5 10:51, Luiz Capitulino 写道:
  On Tue, 05 Nov 2013 10:17:28 +0800
  Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
 
  于 2013/11/4 21:33, Luiz Capitulino 写道:
  On Mon, 04 Nov 2013 09:59:50 +0800
  Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
 
  于 2013/11/1 22:02, Luiz Capitulino 写道:
  On Mon, 21 Oct 2013 10:16:01 +0800
  Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
 
  Signed-off-by: Wenchao Xia xiaw...@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()
 

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

2013-11-05 Thread Wenchao Xia

于 2013/11/5 22:06, Luiz Capitulino 写道:

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


于 2013/11/5 10:51, Luiz Capitulino 写道:

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


于 2013/11/4 21:33, Luiz Capitulino 写道:

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


于 2013/11/1 22:02, Luiz Capitulino 写道:

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


Signed-off-by: Wenchao Xia xiaw...@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, 

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

2013-11-04 Thread Luiz Capitulino
On Mon, 04 Nov 2013 09:59:50 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:

 于 2013/11/1 22:02, Luiz Capitulino 写道:
  On Mon, 21 Oct 2013 10:16:01 +0800
  Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
 
  Signed-off-by: Wenchao Xia xiaw...@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

2013-11-04 Thread Wenchao Xia

于 2013/11/4 21:33, Luiz Capitulino 写道:

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


于 2013/11/1 22:02, Luiz Capitulino 写道:

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


Signed-off-by: Wenchao Xia xiaw...@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

2013-11-04 Thread Luiz Capitulino
On Tue, 05 Nov 2013 10:17:28 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:

 于 2013/11/4 21:33, Luiz Capitulino 写道:
  On Mon, 04 Nov 2013 09:59:50 +0800
  Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
 
  于 2013/11/1 22:02, Luiz Capitulino 写道:
  On Mon, 21 Oct 2013 10:16:01 +0800
  Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
 
  Signed-off-by: Wenchao Xia xiaw...@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

2013-11-04 Thread Wenchao Xia

于 2013/11/5 10:51, Luiz Capitulino 写道:

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


于 2013/11/4 21:33, Luiz Capitulino 写道:

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


于 2013/11/1 22:02, Luiz Capitulino 写道:

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


Signed-off-by: Wenchao Xia xiaw...@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

2013-11-03 Thread Wenchao Xia

于 2013/11/1 22:02, Luiz Capitulino 写道:

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


Signed-off-by: Wenchao Xia xiaw...@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
--- 

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

2013-11-01 Thread Luiz Capitulino
On Mon, 21 Oct 2013 10:16:01 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:

 Signed-off-by: Wenchao Xia xiaw...@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 

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

2013-10-21 Thread Eric Blake
On 10/21/2013 03:16 AM, Wenchao Xia wrote:
 Signed-off-by: Wenchao Xia xiaw...@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 ebl...@redhat.com

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



signature.asc
Description: OpenPGP digital signature