[Qemu-devel] [PATCH 00/13] trivial patches for event, error and monitor

[Qemu-devel] [PATCH 00/13] trivial patches for event, error and monitor

[Wenchao Xia]

I was trying to decouple components between block and qemu code, and reduce
stub files. I found fixing some small code problem found could help, so pick up
those which can benifit upstream code either and send them as separate series.

patch 1 to 4 comes from my RFC series earlier with title:
Remove stub mon-protocol-event for block

Wenchao Xia (13):
  1 block: use type MonitorEvent directly
  2 block: do not include monitor.h in block.c
  3 qapi: move MonitorEvent define
  4 qapi: rename MonitorEvent to QEvent
  5 error: define struct Error in only one place
  6 error: remove error_printf_unless_qmp()
  7 error: make error_print_loc() static
  8 error: don't set sep when print progname
  9 error: print progname with error_vprintf()
  10 qerror: deref once in qerror_report()
  11 qerror: folder qerror emit logic
  12 monitor: hide *cur_mon in monitor_get_fd()
  13 stubs: do not call monitor_printf()

 block.c                     |    3 +-
 dump.c                      |    2 +-
 hw/usb/bus.c                |    2 +-
 hw/usb/hcd-ehci.c           |    4 +-
 include/block/block_int.h   |    2 +-
 include/monitor/monitor.h   |   42 ++-------------------------
 include/qapi/error.h        |    5 ++-
 include/qapi/qmp/qevent.h   |   66 +++++++++++++++++++++++++++++++++++++++++++
 include/qapi/qmp/types.h    |    1 +
 include/qemu/error-report.h |    2 -
 migration-fd.c              |    2 +-
 monitor.c                   |   19 ++++++++----
 qmp.c                       |    2 +-
 qobject/qerror.c            |   36 ++++++++++-------------
 stubs/get-fd.c              |    2 +-
 stubs/mon-protocol-event.c  |    2 +-
 stubs/pci-drive-hot-add.c   |    1 -
 ui/vnc.c                    |    2 +-
 util/error.c                |    6 ----
 util/qemu-error.c           |   27 ++++++++----------
 util/qemu-sockets.c         |    4 +-
 21 files changed, 127 insertions(+), 105 deletions(-)
 create mode 100644 include/qapi/qmp/qevent.h

[Qemu-devel] [PATCH 01/13] 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 02/13] block: do not include monitor.h in block.c

[Wenchao Xia]

block_int.h already included it.

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

diff --git a/block.c b/block.c
index 2c15e5d..e92a556 100644
--- a/block.c
+++ b/block.c
@@ -24,7 +24,6 @@
 #include "config-host.h"
 #include "qemu-common.h"
 #include "trace.h"
-#include "monitor/monitor.h"
 #include "block/block_int.h"
 #include "block/blockjob.h"
 #include "qemu/module.h"
-- 
1.7.1

[Qemu-devel] [PATCH 03/13] qapi: move MonitorEvent define

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 include/monitor/monitor.h |   38 +-------------------------
 include/qapi/qmp/qevent.h |   66 +++++++++++++++++++++++++++++++++++++++++++++
 include/qapi/qmp/types.h  |    1 +
 3 files changed, 68 insertions(+), 37 deletions(-)
 create mode 100644 include/qapi/qmp/qevent.h

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 10fa0e3..686c0eb 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -4,6 +4,7 @@
 #include "qemu-common.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qevent.h"
 #include "block/block.h"
 #include "monitor/readline.h"
 
@@ -19,43 +20,6 @@ extern Monitor *default_mon;
 /* flags for monitor commands */
 #define MONITOR_CMD_ASYNC       0x0001
 
-/* QMP events */
-typedef enum MonitorEvent {
-    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,
-
-    /* Add to 'monitor_event_names' array in monitor.c when
-     * defining new events here */
-
-    QEVENT_MAX,
-} MonitorEvent;
-
 int monitor_cur_is_qmp(void);
 
 void monitor_protocol_event(MonitorEvent event, QObject *data);
diff --git a/include/qapi/qmp/qevent.h b/include/qapi/qmp/qevent.h
new file mode 100644
index 0000000..e6d09fc
--- /dev/null
+++ b/include/qapi/qmp/qevent.h
@@ -0,0 +1,66 @@
+/*
+ * QEvent related defines and functions
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+
+#ifndef QEVENT_H
+#define QEVENT_H
+
+/* QMP events */
+typedef enum MonitorEvent {
+    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,
+
+    /* Add to 'monitor_event_names' array in monitor.c when
+     * defining new events here */
+
+    QEVENT_MAX,
+} MonitorEvent;
+
+#endif
diff --git a/include/qapi/qmp/types.h b/include/qapi/qmp/types.h
index 7782ec5..ba317bf 100644
--- a/include/qapi/qmp/types.h
+++ b/include/qapi/qmp/types.h
@@ -21,5 +21,6 @@
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qevent.h"
 
 #endif /* QEMU_OBJECTS_H */
-- 
1.7.1

[Qemu-devel] [PATCH 04/13] qapi: rename MonitorEvent to QEvent

[Wenchao Xia]

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 +-
 include/monitor/monitor.h  |    2 +-
 include/qapi/qmp/qevent.h  |    4 ++--
 monitor.c                  |   12 ++++++------
 stubs/mon-protocol-event.c |    2 +-
 ui/vnc.c                   |    2 +-
 7 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index e92a556..642369f 100644
--- a/block.c
+++ b/block.c
@@ -1759,7 +1759,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 686c0eb..97fcee3 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -22,7 +22,7 @@ extern Monitor *default_mon;
 
 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/include/qapi/qmp/qevent.h b/include/qapi/qmp/qevent.h
index e6d09fc..c4f43e5 100644
--- a/include/qapi/qmp/qevent.h
+++ b/include/qapi/qmp/qevent.h
@@ -27,7 +27,7 @@
 #define QEVENT_H
 
 /* QMP events */
-typedef enum MonitorEvent {
+typedef enum QEvent {
     QEVENT_SHUTDOWN,
     QEVENT_RESET,
     QEVENT_POWERDOWN,
@@ -61,6 +61,6 @@ typedef enum MonitorEvent {
      * defining new events here */
 
     QEVENT_MAX,
-} MonitorEvent;
+} QEvent;
 
 #endif
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

[Qemu-devel] [PATCH 05/13] error: define struct Error in only one place

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 include/qapi/error.h |    5 ++++-
 qobject/qerror.c     |    7 -------
 util/error.c         |    6 ------
 3 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 7d4c696..8688aaf 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -20,7 +20,10 @@
  * A class representing internal errors within QEMU.  An error has a ErrorClass
  * code and a human message.
  */
-typedef struct Error Error;
+typedef struct Error {
+    char *msg;
+    ErrorClass err_class;
+} Error;
 
 /**
  * Set an indirect pointer to an error given a ErrorClass value and a
diff --git a/qobject/qerror.c b/qobject/qerror.c
index 3aee1cf..5b487f3 100644
--- a/qobject/qerror.c
+++ b/qobject/qerror.c
@@ -97,13 +97,6 @@ void qerror_report(ErrorClass eclass, const char *fmt, ...)
     }
 }
 
-/* Evil... */
-struct Error
-{
-    char *msg;
-    ErrorClass err_class;
-};
-
 void qerror_report_err(Error *err)
 {
     QError *qerr;
diff --git a/util/error.c b/util/error.c
index ec0faa6..da0d221 100644
--- a/util/error.c
+++ b/util/error.c
@@ -17,12 +17,6 @@
 #include "qapi-types.h"
 #include "qapi/qmp/qerror.h"
 
-struct Error
-{
-    char *msg;
-    ErrorClass err_class;
-};
-
 void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
 {
     Error *err;
-- 
1.7.1

[Qemu-devel] [PATCH 06/13] error: remove error_printf_unless_qmp()

[Wenchao Xia]

monitor_vprintf() is called in the code path, and it will not print
when monitor is in qmp mode, so checking monitor mode in
error_printf_unless_qmp() is useless, remove it to simplify the code.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 hw/usb/bus.c                |    2 +-
 hw/usb/hcd-ehci.c           |    4 ++--
 include/qemu/error-report.h |    1 -
 util/qemu-error.c           |   11 -----------
 4 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 72d5b92..817a69a 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -328,7 +328,7 @@ int usb_register_companion(const char *masterbus, USBPort *ports[],
         qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
                       "an USB masterbus");
         if (bus) {
-            error_printf_unless_qmp(
+            error_report(
                 "USB bus '%s' does not allow companion controllers\n",
                 masterbus);
         }
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 22bdbf4..bb7e003 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -848,7 +848,7 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
     if (firstport + portcount > NB_PORTS) {
         qerror_report(QERR_INVALID_PARAMETER_VALUE, "firstport",
                       "firstport on masterbus");
-        error_printf_unless_qmp(
+        error_report(
             "firstport value of %u makes companion take ports %u - %u, which "
             "is outside of the valid range of 0 - %u\n", firstport, firstport,
             firstport + portcount - 1, NB_PORTS - 1);
@@ -859,7 +859,7 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
         if (s->companion_ports[firstport + i]) {
             qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
                           "an USB masterbus");
-            error_printf_unless_qmp(
+            error_report(
                 "port %u on masterbus %s already has a companion assigned\n",
                 firstport + i, bus->qbus.name);
             return -1;
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 3b098a9..5297e65 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -36,7 +36,6 @@ void loc_set_file(const char *fname, int lno);
 
 void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
 void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
-void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void error_print_loc(void);
 void error_set_progname(const char *argv0);
 void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
diff --git a/util/qemu-error.c b/util/qemu-error.c
index fec02c6..85e8a79 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -40,17 +40,6 @@ void error_printf(const char *fmt, ...)
     va_end(ap);
 }
 
-void error_printf_unless_qmp(const char *fmt, ...)
-{
-    va_list ap;
-
-    if (!monitor_cur_is_qmp()) {
-        va_start(ap, fmt);
-        error_vprintf(fmt, ap);
-        va_end(ap);
-    }
-}
-
 static Location std_loc = {
     .kind = LOC_NONE
 };
-- 
1.7.1

[Qemu-devel] [PATCH 07/13] error: make error_print_loc() static

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 include/qemu/error-report.h |    1 -
 util/qemu-error.c           |    2 +-
 2 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 5297e65..a08ee95 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -36,7 +36,6 @@ void loc_set_file(const char *fname, int lno);
 
 void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
 void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
-void error_print_loc(void);
 void error_set_progname(const char *argv0);
 void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 const char *error_get_progname(void);
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 85e8a79..0ccd3e9 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -154,7 +154,7 @@ const char *error_get_progname(void)
 /*
  * Print current location to current monitor if we have one, else to stderr.
  */
-void error_print_loc(void)
+static void error_print_loc(void)
 {
     const char *sep = "";
     int i;
-- 
1.7.1

[Qemu-devel] [PATCH 08/13] error: don't set sep when print progname

[Wenchao Xia]

The behavior to set sep brings trouble to modification later,
the logic is not changed by add tailing space in fprintf().

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 util/qemu-error.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/util/qemu-error.c b/util/qemu-error.c
index 0ccd3e9..d1e858a 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -161,8 +161,7 @@ static void error_print_loc(void)
     const char *const *argp;
 
     if (!cur_mon && progname) {
-        fprintf(stderr, "%s:", progname);
-        sep = " ";
+        fprintf(stderr, "%s: ", progname);
     }
     switch (cur_loc->kind) {
     case LOC_CMDLINE:
@@ -181,7 +180,7 @@ static void error_print_loc(void)
         error_printf(" ");
         break;
     default:
-        error_printf("%s", sep);
+        break;
     }
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 09/13] error: print progname with error_vprintf()

[Wenchao Xia]

This remove additional code path about where to print the error,
error_vprintf() is only the controller now, making future change
easier.

The logic is not changed since when cur_mon = NULL, error_vprintf()
will still print to stderr.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 util/qemu-error.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/util/qemu-error.c b/util/qemu-error.c
index d1e858a..c29fcbd 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -151,6 +151,15 @@ const char *error_get_progname(void)
     return progname;
 }
 
+static void error_print_progname(const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_vprintf(fmt, ap);
+    va_end(ap);
+}
+
 /*
  * Print current location to current monitor if we have one, else to stderr.
  */
@@ -161,7 +170,7 @@ static void error_print_loc(void)
     const char *const *argp;
 
     if (!cur_mon && progname) {
-        fprintf(stderr, "%s: ", progname);
+        error_print_progname("%s: ", progname);
     }
     switch (cur_loc->kind) {
     case LOC_CMDLINE:
-- 
1.7.1

[Qemu-devel] [PATCH 10/13] qerror: deref once in qerror_report()

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qobject/qerror.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/qobject/qerror.c b/qobject/qerror.c
index 5b487f3..685167a 100644
--- a/qobject/qerror.c
+++ b/qobject/qerror.c
@@ -77,7 +77,6 @@ static void qerror_print(QError *qerror)
     loc_push_restore(&qerror->loc);
     error_report("%s", qstring_get_str(qstring));
     loc_pop(&qerror->loc);
-    QDECREF(qstring);
 }
 
 void qerror_report(ErrorClass eclass, const char *fmt, ...)
-- 
1.7.1

[Qemu-devel] [PATCH 11/13] qerror: folder qerror emit logic

[Wenchao Xia]

The code about how to print the message, is foldered into a function,
so if we want to change the print behavior in the future, just modify
that function only.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qobject/qerror.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/qobject/qerror.c b/qobject/qerror.c
index 685167a..86d1b07 100644
--- a/qobject/qerror.c
+++ b/qobject/qerror.c
@@ -79,6 +79,16 @@ static void qerror_print(QError *qerror)
     loc_pop(&qerror->loc);
 }
 
+static void monitor_print_qerror(QError *qerror)
+{
+    if (monitor_cur_is_qmp()) {
+        QINCREF(qerror);
+        monitor_set_error(cur_mon, qerror);
+    } else {
+        qerror_print(qerror);
+    }
+}
+
 void qerror_report(ErrorClass eclass, const char *fmt, ...)
 {
     va_list va;
@@ -88,12 +98,9 @@ void qerror_report(ErrorClass eclass, const char *fmt, ...)
     qerror = qerror_from_info(eclass, fmt, &va);
     va_end(va);
 
-    if (monitor_cur_is_qmp()) {
-        monitor_set_error(cur_mon, qerror);
-    } else {
-        qerror_print(qerror);
-        QDECREF(qerror);
-    }
+    monitor_print_qerror(qerror);
+
+    QDECREF(qerror);
 }
 
 void qerror_report_err(Error *err)
@@ -105,12 +112,9 @@ void qerror_report_err(Error *err)
     qerr->err_msg = g_strdup(err->msg);
     qerr->err_class = err->err_class;
 
-    if (monitor_cur_is_qmp()) {
-        monitor_set_error(cur_mon, qerr);
-    } else {
-        qerror_print(qerr);
-        QDECREF(qerr);
-    }
+    monitor_print_qerror(qerr);
+
+    QDECREF(qerr);
 }
 
 void assert_no_error(Error *err)
-- 
1.7.1

[Qemu-devel] [PATCH 12/13] monitor: hide *cur_mon in monitor_get_fd()

[Wenchao Xia]

All existing caller are using *cur_mon as its parameter, and *cur_mon
is an internal variable which used inside monitor.c. This patch reduce
the exposing of details in monitor.c, by introduce a new function
monitor_get_fd_cur() and make old one static.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 dump.c                    |    2 +-
 include/monitor/monitor.h |    2 +-
 migration-fd.c            |    2 +-
 monitor.c                 |    7 ++++++-
 qmp.c                     |    2 +-
 stubs/get-fd.c            |    2 +-
 util/qemu-sockets.c       |    4 ++--
 7 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/dump.c b/dump.c
index 846155c..8f5b6b0 100644
--- a/dump.c
+++ b/dump.c
@@ -860,7 +860,7 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
 
 #if !defined(WIN32)
     if (strstart(file, "fd:", &p)) {
-        fd = monitor_get_fd(cur_mon, p, errp);
+        fd = monitor_get_fd_cur(p, errp);
         if (fd == -1) {
             return;
         }
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 97fcee3..637f7f3 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -35,7 +35,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device,
                                   BlockDriverCompletionFunc *completion_cb,
                                   void *opaque);
 
-int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
+int monitor_get_fd_cur(const char *fdname, Error **errp);
 int monitor_handle_fd_param(Monitor *mon, const char *fdname);
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
diff --git a/migration-fd.c b/migration-fd.c
index d2e523a..022bc50 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -33,7 +33,7 @@
 
 void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
 {
-    int fd = monitor_get_fd(cur_mon, fdname, errp);
+    int fd = monitor_get_fd_cur(fdname, errp);
     if (fd == -1) {
         return;
     }
diff --git a/monitor.c b/monitor.c
index 9377834..80a9dfd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2290,7 +2290,7 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
     }
 }
 
-int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
+static int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
 {
     mon_fd_t *monfd;
 
@@ -2315,6 +2315,11 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
     return -1;
 }
 
+int monitor_get_fd_cur(const char *fdname, Error **errp)
+{
+    return monitor_get_fd(cur_mon, fdname, errp);
+}
+
 static void monitor_fdset_cleanup(MonFdset *mon_fdset)
 {
     MonFdsetFd *mon_fdset_fd;
diff --git a/qmp.c b/qmp.c
index 4c149b3..a02804b 100644
--- a/qmp.c
+++ b/qmp.c
@@ -493,7 +493,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
     CharDriverState *s;
     int fd;
 
-    fd = monitor_get_fd(cur_mon, fdname, errp);
+    fd = monitor_get_fd_cur(fdname, errp);
     if (fd < 0) {
         return;
     }
diff --git a/stubs/get-fd.c b/stubs/get-fd.c
index 9f2c65c..7d9ec3b 100644
--- a/stubs/get-fd.c
+++ b/stubs/get-fd.c
@@ -1,7 +1,7 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 
-int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
+int monitor_get_fd_cur(const char *name, Error **errp)
 {
     error_setg(errp, "only QEMU supports file descriptor passing");
     return -1;
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 6b97dc1..9cd85dd 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -902,7 +902,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
         break;
 
     case SOCKET_ADDRESS_KIND_FD:
-        fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
+        fd = monitor_get_fd_cur(addr->fd->str, errp);
         if (fd >= 0 && callback) {
             qemu_set_nonblock(fd);
             callback(fd, opaque);
@@ -934,7 +934,7 @@ int socket_listen(SocketAddress *addr, Error **errp)
         break;
 
     case SOCKET_ADDRESS_KIND_FD:
-        fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
+        fd = monitor_get_fd_cur(addr->fd->str, errp);
         break;
 
     default:
-- 
1.7.1

[Qemu-devel] [PATCH 13/13] stubs: do not call monitor_printf()

[Wenchao Xia]

stubs/pci-drive-host-add.o is packaged together with /stubs/mon-printf.o
so it would not work normal, remove it.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 stubs/pci-drive-hot-add.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/stubs/pci-drive-hot-add.c b/stubs/pci-drive-hot-add.c
index 1d98145..af4612f 100644
--- a/stubs/pci-drive-hot-add.c
+++ b/stubs/pci-drive-hot-add.c
@@ -5,6 +5,5 @@
 int pci_drive_hot_add(Monitor *mon, const QDict *qdict, DriveInfo *dinfo)
 {
     /* On non-x86 we don't do PCI hotplug */
-    monitor_printf(mon, "Can't hot-add drive to type %d\n", dinfo->type);
     return -1;
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 02/13] block: do not include monitor.h in block.c

[Paolo Bonzini]

Il 18/10/2013 03:11, Wenchao Xia ha scritto:
> block_int.h already included it.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 2c15e5d..e92a556 100644
> --- a/block.c
> +++ b/block.c
> @@ -24,7 +24,6 @@
>  #include "config-host.h"
>  #include "qemu-common.h"
>  #include "trace.h"
> -#include "monitor/monitor.h"
>  #include "block/block_int.h"
>  #include "block/blockjob.h"
>  #include "qemu/module.h"
> 

Does this cause problems?  block.c uses monitor_protocol_event, so it's
good to include the file directly instead of relying on other header files.

Paolo

Re: [Qemu-devel] [PATCH 03/13] qapi: move MonitorEvent define

[Paolo Bonzini]

Il 18/10/2013 03:11, Wenchao Xia ha scritto:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  include/monitor/monitor.h |   38 +-------------------------
>  include/qapi/qmp/qevent.h |   66 +++++++++++++++++++++++++++++++++++++++++++++
>  include/qapi/qmp/types.h  |    1 +
>  3 files changed, 68 insertions(+), 37 deletions(-)
>  create mode 100644 include/qapi/qmp/qevent.h
> 
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 10fa0e3..686c0eb 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -4,6 +4,7 @@
>  #include "qemu-common.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qevent.h"
>  #include "block/block.h"
>  #include "monitor/readline.h"
>  
> @@ -19,43 +20,6 @@ extern Monitor *default_mon;
>  /* flags for monitor commands */
>  #define MONITOR_CMD_ASYNC       0x0001
>  
> -/* QMP events */
> -typedef enum MonitorEvent {
> -    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,
> -
> -    /* Add to 'monitor_event_names' array in monitor.c when
> -     * defining new events here */
> -
> -    QEVENT_MAX,
> -} MonitorEvent;
> -
>  int monitor_cur_is_qmp(void);
>  
>  void monitor_protocol_event(MonitorEvent event, QObject *data);
> diff --git a/include/qapi/qmp/qevent.h b/include/qapi/qmp/qevent.h
> new file mode 100644
> index 0000000..e6d09fc
> --- /dev/null
> +++ b/include/qapi/qmp/qevent.h
> @@ -0,0 +1,66 @@
> +/*
> + * QEvent related defines and functions
> + *
> + * Copyright (c) 2003-2004 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +
> +#ifndef QEVENT_H
> +#define QEVENT_H
> +
> +/* QMP events */
> +typedef enum MonitorEvent {
> +    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,
> +
> +    /* Add to 'monitor_event_names' array in monitor.c when
> +     * defining new events here */
> +
> +    QEVENT_MAX,
> +} MonitorEvent;
> +
> +#endif
> diff --git a/include/qapi/qmp/types.h b/include/qapi/qmp/types.h
> index 7782ec5..ba317bf 100644
> --- a/include/qapi/qmp/types.h
> +++ b/include/qapi/qmp/types.h
> @@ -21,5 +21,6 @@
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qlist.h"
>  #include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qevent.h"
>  
>  #endif /* QEMU_OBJECTS_H */
> 

Please move it qemu-schema.json instead.

Paolo

Re: [Qemu-devel] [PATCH 05/13] error: define struct Error in only one place

[Paolo Bonzini]

Il 18/10/2013 03:11, Wenchao Xia ha scritto:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  include/qapi/error.h |    5 ++++-
>  qobject/qerror.c     |    7 -------
>  util/error.c         |    6 ------
>  3 files changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 7d4c696..8688aaf 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -20,7 +20,10 @@
>   * A class representing internal errors within QEMU.  An error has a ErrorClass
>   * code and a human message.
>   */
> -typedef struct Error Error;
> +typedef struct Error {
> +    char *msg;
> +    ErrorClass err_class;
> +} Error;

Please add a comment that it should be treated as an opaque type.

Paolo

>  
>  /**
>   * Set an indirect pointer to an error given a ErrorClass value and a
> diff --git a/qobject/qerror.c b/qobject/qerror.c
> index 3aee1cf..5b487f3 100644
> --- a/qobject/qerror.c
> +++ b/qobject/qerror.c
> @@ -97,13 +97,6 @@ void qerror_report(ErrorClass eclass, const char *fmt, ...)
>      }
>  }
>  
> -/* Evil... */
> -struct Error
> -{
> -    char *msg;
> -    ErrorClass err_class;
> -};
> -
>  void qerror_report_err(Error *err)
>  {
>      QError *qerr;
> diff --git a/util/error.c b/util/error.c
> index ec0faa6..da0d221 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -17,12 +17,6 @@
>  #include "qapi-types.h"
>  #include "qapi/qmp/qerror.h"
>  
> -struct Error
> -{
> -    char *msg;
> -    ErrorClass err_class;
> -};
> -
>  void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>  {
>      Error *err;
> 

Re: [Qemu-devel] [PATCH 06/13] error: remove error_printf_unless_qmp()

[Paolo Bonzini]

Il 18/10/2013 03:11, Wenchao Xia ha scritto:
> monitor_vprintf() is called in the code path, and it will not print
> when monitor is in qmp mode, so checking monitor mode in
> error_printf_unless_qmp() is useless, remove it to simplify the code.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> ---
>  hw/usb/bus.c                |    2 +-
>  hw/usb/hcd-ehci.c           |    4 ++--
>  include/qemu/error-report.h |    1 -
>  util/qemu-error.c           |   11 -----------
>  4 files changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 72d5b92..817a69a 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -328,7 +328,7 @@ int usb_register_companion(const char *masterbus, USBPort *ports[],
>          qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
>                        "an USB masterbus");
>          if (bus) {
> -            error_printf_unless_qmp(
> +            error_report(
>                  "USB bus '%s' does not allow companion controllers\n",
>                  masterbus);
>          }
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 22bdbf4..bb7e003 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -848,7 +848,7 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
>      if (firstport + portcount > NB_PORTS) {
>          qerror_report(QERR_INVALID_PARAMETER_VALUE, "firstport",
>                        "firstport on masterbus");
> -        error_printf_unless_qmp(
> +        error_report(
>              "firstport value of %u makes companion take ports %u - %u, which "
>              "is outside of the valid range of 0 - %u\n", firstport, firstport,
>              firstport + portcount - 1, NB_PORTS - 1);
> @@ -859,7 +859,7 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
>          if (s->companion_ports[firstport + i]) {
>              qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
>                            "an USB masterbus");
> -            error_printf_unless_qmp(
> +            error_report(
>                  "port %u on masterbus %s already has a companion assigned\n",
>                  firstport + i, bus->qbus.name);
>              return -1;
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 3b098a9..5297e65 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -36,7 +36,6 @@ void loc_set_file(const char *fname, int lno);
>  
>  void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
>  void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> -void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  void error_print_loc(void);
>  void error_set_progname(const char *argv0);
>  void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index fec02c6..85e8a79 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -40,17 +40,6 @@ void error_printf(const char *fmt, ...)
>      va_end(ap);
>  }
>  
> -void error_printf_unless_qmp(const char *fmt, ...)
> -{
> -    va_list ap;
> -
> -    if (!monitor_cur_is_qmp()) {
> -        va_start(ap, fmt);
> -        error_vprintf(fmt, ap);
> -        va_end(ap);
> -    }
> -}
> -
>  static Location std_loc = {
>      .kind = LOC_NONE
>  };
> 

Re: [Qemu-devel] [PATCH 08/13] error: don't set sep when print progname

[Paolo Bonzini]

Il 18/10/2013 03:11, Wenchao Xia ha scritto:
> The behavior to set sep brings trouble to modification later,
> the logic is not changed by add tailing space in fprintf().
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  util/qemu-error.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index 0ccd3e9..d1e858a 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -161,8 +161,7 @@ static void error_print_loc(void)
>      const char *const *argp;
>  
>      if (!cur_mon && progname) {
> -        fprintf(stderr, "%s:", progname);
> -        sep = " ";
> +        fprintf(stderr, "%s: ", progname);
>      }
>      switch (cur_loc->kind) {
>      case LOC_CMDLINE:
> @@ -181,7 +180,7 @@ static void error_print_loc(void)
>          error_printf(" ");
>          break;
>      default:
> -        error_printf("%s", sep);
> +        break;
>      }
>  }
>  
> 

This changes behavior for LOC_FILE.

Before:

$ cat xyz.cfg
[device "abc"]
        driver = def
$ qemu-system-x86_64 -readconfig xyz.cfg
qemu-system-x86_64:xyz.cfg:2: parse error

After:

$ qemu-system-x86_64 -readconfig xyz.cfg
qemu-system-x86_64: xyz.cfg:2: parse error

Could even be an improvement, but you need to note it in the commit message.

Paolo

Re: [Qemu-devel] [PATCH 09/13] error: print progname with error_vprintf()

[Paolo Bonzini]

Il 18/10/2013 03:11, Wenchao Xia ha scritto:
> This remove additional code path about where to print the error,
> error_vprintf() is only the controller now, making future change
> easier.
> 
> The logic is not changed since when cur_mon = NULL, error_vprintf()
> will still print to stderr.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  util/qemu-error.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index d1e858a..c29fcbd 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -151,6 +151,15 @@ const char *error_get_progname(void)
>      return progname;
>  }
>  
> +static void error_print_progname(const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    error_vprintf(fmt, ap);
> +    va_end(ap);
> +}
> +
>  /*
>   * Print current location to current monitor if we have one, else to stderr.
>   */
> @@ -161,7 +170,7 @@ static void error_print_loc(void)
>      const char *const *argp;
>  
>      if (!cur_mon && progname) {
> -        fprintf(stderr, "%s: ", progname);
> +        error_print_progname("%s: ", progname);
>      }
>      switch (cur_loc->kind) {
>      case LOC_CMDLINE:
> 

I agree that using fprintf looks odd, but why not use error_printf directly?

Paolo

Re: [Qemu-devel] [PATCH 10/13] qerror: deref once in qerror_report()

[Paolo Bonzini]

Il 18/10/2013 03:11, Wenchao Xia ha scritto:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  qobject/qerror.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/qobject/qerror.c b/qobject/qerror.c
> index 5b487f3..685167a 100644
> --- a/qobject/qerror.c
> +++ b/qobject/qerror.c
> @@ -77,7 +77,6 @@ static void qerror_print(QError *qerror)
>      loc_push_restore(&qerror->loc);
>      error_report("%s", qstring_get_str(qstring));
>      loc_pop(&qerror->loc);
> -    QDECREF(qstring);
>  }
>  
>  void qerror_report(ErrorClass eclass, const char *fmt, ...)
> 

Why isn't this a memory leak?

Paolo

Re: [Qemu-devel] [PATCH 11/13] qerror: folder qerror emit logic

[Paolo Bonzini]

Il 18/10/2013 03:11, Wenchao Xia ha scritto:
> The code about how to print the message, is foldered into a function,
> so if we want to change the print behavior in the future, just modify
> that function only.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  qobject/qerror.c |   28 ++++++++++++++++------------
>  1 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/qobject/qerror.c b/qobject/qerror.c
> index 685167a..86d1b07 100644
> --- a/qobject/qerror.c
> +++ b/qobject/qerror.c
> @@ -79,6 +79,16 @@ static void qerror_print(QError *qerror)
>      loc_pop(&qerror->loc);
>  }
>  
> +static void monitor_print_qerror(QError *qerror)
> +{
> +    if (monitor_cur_is_qmp()) {
> +        QINCREF(qerror);
> +        monitor_set_error(cur_mon, qerror);
> +    } else {
> +        qerror_print(qerror);
> +    }
> +}
> +
>  void qerror_report(ErrorClass eclass, const char *fmt, ...)
>  {
>      va_list va;
> @@ -88,12 +98,9 @@ void qerror_report(ErrorClass eclass, const char *fmt, ...)
>      qerror = qerror_from_info(eclass, fmt, &va);
>      va_end(va);
>  
> -    if (monitor_cur_is_qmp()) {
> -        monitor_set_error(cur_mon, qerror);
> -    } else {
> -        qerror_print(qerror);
> -        QDECREF(qerror);
> -    }
> +    monitor_print_qerror(qerror);
> +
> +    QDECREF(qerror);
>  }
>  
>  void qerror_report_err(Error *err)
> @@ -105,12 +112,9 @@ void qerror_report_err(Error *err)
>      qerr->err_msg = g_strdup(err->msg);
>      qerr->err_class = err->err_class;
>  
> -    if (monitor_cur_is_qmp()) {
> -        monitor_set_error(cur_mon, qerr);
> -    } else {
> -        qerror_print(qerr);
> -        QDECREF(qerr);
> -    }
> +    monitor_print_qerror(qerr);
> +
> +    QDECREF(qerr);
>  }
>  
>  void assert_no_error(Error *err)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 12/13] monitor: hide *cur_mon in monitor_get_fd()

[Paolo Bonzini]

Il 18/10/2013 03:11, Wenchao Xia ha scritto:
> All existing caller are using *cur_mon as its parameter, and *cur_mon
> is an internal variable which used inside monitor.c. This patch reduce
> the exposing of details in monitor.c, by introduce a new function
> monitor_get_fd_cur() and make old one static.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  dump.c                    |    2 +-
>  include/monitor/monitor.h |    2 +-
>  migration-fd.c            |    2 +-
>  monitor.c                 |    7 ++++++-
>  qmp.c                     |    2 +-
>  stubs/get-fd.c            |    2 +-
>  util/qemu-sockets.c       |    4 ++--
>  7 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 846155c..8f5b6b0 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -860,7 +860,7 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
>  
>  #if !defined(WIN32)
>      if (strstart(file, "fd:", &p)) {
> -        fd = monitor_get_fd(cur_mon, p, errp);
> +        fd = monitor_get_fd_cur(p, errp);
>          if (fd == -1) {
>              return;
>          }
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 97fcee3..637f7f3 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -35,7 +35,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device,
>                                    BlockDriverCompletionFunc *completion_cb,
>                                    void *opaque);
>  
> -int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
> +int monitor_get_fd_cur(const char *fdname, Error **errp);
>  int monitor_handle_fd_param(Monitor *mon, const char *fdname);
>  
>  void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
> diff --git a/migration-fd.c b/migration-fd.c
> index d2e523a..022bc50 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -33,7 +33,7 @@
>  
>  void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
>  {
> -    int fd = monitor_get_fd(cur_mon, fdname, errp);
> +    int fd = monitor_get_fd_cur(fdname, errp);
>      if (fd == -1) {
>          return;
>      }
> diff --git a/monitor.c b/monitor.c
> index 9377834..80a9dfd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2290,7 +2290,7 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> -int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> +static int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>  {
>      mon_fd_t *monfd;
>  
> @@ -2315,6 +2315,11 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>      return -1;
>  }
>  
> +int monitor_get_fd_cur(const char *fdname, Error **errp)
> +{
> +    return monitor_get_fd(cur_mon, fdname, errp);
> +}
> +
>  static void monitor_fdset_cleanup(MonFdset *mon_fdset)
>  {
>      MonFdsetFd *mon_fdset_fd;
> diff --git a/qmp.c b/qmp.c
> index 4c149b3..a02804b 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -493,7 +493,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
>      CharDriverState *s;
>      int fd;
>  
> -    fd = monitor_get_fd(cur_mon, fdname, errp);
> +    fd = monitor_get_fd_cur(fdname, errp);
>      if (fd < 0) {
>          return;
>      }
> diff --git a/stubs/get-fd.c b/stubs/get-fd.c
> index 9f2c65c..7d9ec3b 100644
> --- a/stubs/get-fd.c
> +++ b/stubs/get-fd.c
> @@ -1,7 +1,7 @@
>  #include "qemu-common.h"
>  #include "monitor/monitor.h"
>  
> -int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
> +int monitor_get_fd_cur(const char *name, Error **errp)
>  {
>      error_setg(errp, "only QEMU supports file descriptor passing");
>      return -1;
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 6b97dc1..9cd85dd 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -902,7 +902,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
>          break;
>  
>      case SOCKET_ADDRESS_KIND_FD:
> -        fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
> +        fd = monitor_get_fd_cur(addr->fd->str, errp);
>          if (fd >= 0 && callback) {
>              qemu_set_nonblock(fd);
>              callback(fd, opaque);
> @@ -934,7 +934,7 @@ int socket_listen(SocketAddress *addr, Error **errp)
>          break;
>  
>      case SOCKET_ADDRESS_KIND_FD:
> -        fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
> +        fd = monitor_get_fd_cur(addr->fd->str, errp);
>          break;
>  
>      default:
> 

Doesn't seem like an improvement.  It would be if you could then make
cur_mon static.

Paolo

Re: [Qemu-devel] [PATCH 13/13] stubs: do not call monitor_printf()

[Paolo Bonzini]

Il 18/10/2013 03:11, Wenchao Xia ha scritto:
> stubs/pci-drive-host-add.o is packaged together with /stubs/mon-printf.o
> so it would not work normal, remove it.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  stubs/pci-drive-hot-add.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/stubs/pci-drive-hot-add.c b/stubs/pci-drive-hot-add.c
> index 1d98145..af4612f 100644
> --- a/stubs/pci-drive-hot-add.c
> +++ b/stubs/pci-drive-hot-add.c
> @@ -5,6 +5,5 @@
>  int pci_drive_hot_add(Monitor *mon, const QDict *qdict, DriveInfo *dinfo)
>  {
>      /* On non-x86 we don't do PCI hotplug */
> -    monitor_printf(mon, "Can't hot-add drive to type %d\n", dinfo->type);
>      return -1;
>  }
> 

This is wrong; stubs are independent, that's why each of them is in its
own C file.

When linking a version of QEMU that does not have pci_drive_hot_add,
this function will call the monitor_printf from QEMU.

Paolo

Re: [Qemu-devel] [PATCH 05/13] error: define struct Error in only one place

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>  include/qapi/error.h |    5 ++++-
>>  qobject/qerror.c     |    7 -------
>>  util/error.c         |    6 ------
>>  3 files changed, 4 insertions(+), 14 deletions(-)
>> 
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 7d4c696..8688aaf 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -20,7 +20,10 @@
>>   * A class representing internal errors within QEMU.  An error has a ErrorClass
>>   * code and a human message.
>>   */
>> -typedef struct Error Error;
>> +typedef struct Error {
>> +    char *msg;
>> +    ErrorClass err_class;
>> +} Error;
>
> Please add a comment that it should be treated as an opaque type.

Or keep it opaque here, and complete the type in an internal header.
But see below.

>
> Paolo
>
>>  
>>  /**
>>   * Set an indirect pointer to an error given a ErrorClass value and a
>> diff --git a/qobject/qerror.c b/qobject/qerror.c
>> index 3aee1cf..5b487f3 100644
>> --- a/qobject/qerror.c
>> +++ b/qobject/qerror.c
>> @@ -97,13 +97,6 @@ void qerror_report(ErrorClass eclass, const char *fmt, ...)
>>      }
>>  }
>>  
>> -/* Evil... */
>> -struct Error
>> -{
>> -    char *msg;
>> -    ErrorClass err_class;
>> -};
>> -
>>  void qerror_report_err(Error *err)
>>  {
>>      QError *qerr;

        qerr = qerror_new();
        loc_save(&qerr->loc);
        qerr->err_msg = g_strdup(err->msg);
        qerr->err_class = err->err_class;

        if (monitor_cur_is_qmp()) {
            monitor_set_error(cur_mon, qerr);
        } else {
            qerror_print(qerr);
            QDECREF(qerr);
        }
    }

This is the only use of the "evil" duplicate.  I suspect it could be
cleaned up like this:

        qerr->err_msg = g_strdup(error_get_pretty(err));
        qerr->err_class = error_get_class(err);

If that's true, the duplicate goes away, and we can keep the type
opaque.

>> diff --git a/util/error.c b/util/error.c
>> index ec0faa6..da0d221 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -17,12 +17,6 @@
>>  #include "qapi-types.h"
>>  #include "qapi/qmp/qerror.h"
>>  
>> -struct Error
>> -{
>> -    char *msg;
>> -    ErrorClass err_class;
>> -};
>> -
>>  void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>>  {
>>      Error *err;
>> 

Re: [Qemu-devel] [PATCH 06/13] error: remove error_printf_unless_qmp()

[Markus Armbruster]

Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:

> monitor_vprintf() is called in the code path, and it will not print
> when monitor is in qmp mode, so checking monitor mode in
> error_printf_unless_qmp() is useless, remove it to simplify the code.

Suggest to reword:

error_printf_unless_qmp() is no longer useful, because plain
error_printf() doesn't print anything in a QMP monitor since commit
b8b0826.

> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  hw/usb/bus.c                |    2 +-
>  hw/usb/hcd-ehci.c           |    4 ++--
>  include/qemu/error-report.h |    1 -
>  util/qemu-error.c           |   11 -----------
>  4 files changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 72d5b92..817a69a 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -328,7 +328,7 @@ int usb_register_companion(const char *masterbus, USBPort *ports[],
>          qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
>                        "an USB masterbus");
>          if (bus) {
> -            error_printf_unless_qmp(
> +            error_report(
>                  "USB bus '%s' does not allow companion controllers\n",
>                  masterbus);
>          }

Doesn't this change add location information?  I think you want
error_printf(), not error_report().  Same for the other uses.

[...]

Re: [Qemu-devel] [PATCH 08/13] error: don't set sep when print progname

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>> The behavior to set sep brings trouble to modification later,
>> the logic is not changed by add tailing space in fprintf().
>> 
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>  util/qemu-error.c |    5 ++---
>>  1 files changed, 2 insertions(+), 3 deletions(-)
>> 
>> diff --git a/util/qemu-error.c b/util/qemu-error.c
>> index 0ccd3e9..d1e858a 100644
>> --- a/util/qemu-error.c
>> +++ b/util/qemu-error.c
>> @@ -161,8 +161,7 @@ static void error_print_loc(void)
>>      const char *const *argp;
>>  
>>      if (!cur_mon && progname) {
>> -        fprintf(stderr, "%s:", progname);
>> -        sep = " ";
>> +        fprintf(stderr, "%s: ", progname);
>>      }
>>      switch (cur_loc->kind) {
>>      case LOC_CMDLINE:
>> @@ -181,7 +180,7 @@ static void error_print_loc(void)
>>          error_printf(" ");
>>          break;
>>      default:
>> -        error_printf("%s", sep);
>> +        break;
>>      }
>>  }
>>  
>> 
>
> This changes behavior for LOC_FILE.
>
> Before:
>
> $ cat xyz.cfg
> [device "abc"]
>         driver = def
> $ qemu-system-x86_64 -readconfig xyz.cfg
> qemu-system-x86_64:xyz.cfg:2: parse error
>
> After:
>
> $ qemu-system-x86_64 -readconfig xyz.cfg
> qemu-system-x86_64: xyz.cfg:2: parse error
>
> Could even be an improvement, but you need to note it in the commit message.

No, it is not an improvement.  The old format matches exactly how other
report errors with location, e.g. jade.  Please leave it that way,

Re: [Qemu-devel] [PATCH 03/13] qapi: move MonitorEvent define

[Eric Blake]

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

On 10/18/2013 03:36 AM, Paolo Bonzini wrote:
> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>  include/monitor/monitor.h |   38 +-------------------------
>>  include/qapi/qmp/qevent.h |   66 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/qapi/qmp/types.h  |    1 +
>>  3 files changed, 68 insertions(+), 37 deletions(-)
>>  create mode 100644 include/qapi/qmp/qevent.h

>>
> 
> Please move it qemu-schema.json instead.

qapi-schema.json, but yes, I agree that declaring it as a qapi enum and
letting the code generator create the constants, rather than moving the
hand-maintained constants into a new header, is smarter.

-- 
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 07/13] error: make error_print_loc() static

[Eric Blake]

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

On 10/17/2013 07:11 PM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  include/qemu/error-report.h |    1 -
>  util/qemu-error.c           |    2 +-
>  2 files changed, 1 insertions(+), 2 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 02/13] block: do not include monitor.h in block.c

[Wenchao Xia]

于 2013/10/18 17:36, Paolo Bonzini 写道:
> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>> block_int.h already included it.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block.c |    1 -
>>   1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 2c15e5d..e92a556 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -24,7 +24,6 @@
>>   #include "config-host.h"
>>   #include "qemu-common.h"
>>   #include "trace.h"
>> -#include "monitor/monitor.h"
>>   #include "block/block_int.h"
>>   #include "block/blockjob.h"
>>   #include "qemu/module.h"
>>
> Does this cause problems?  block.c uses monitor_protocol_event, so it's
> good to include the file directly instead of relying on other header files.
>
> Paolo
>
OK, will drop this patch.

Re: [Qemu-devel] [PATCH 03/13] qapi: move MonitorEvent define

[Wenchao Xia]

于 2013/10/18 20:38, Eric Blake 写道:
> On 10/18/2013 03:36 AM, Paolo Bonzini wrote:
>> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   include/monitor/monitor.h |   38 +-------------------------
>>>   include/qapi/qmp/qevent.h |   66 +++++++++++++++++++++++++++++++++++++++++++++
>>>   include/qapi/qmp/types.h  |    1 +
>>>   3 files changed, 68 insertions(+), 37 deletions(-)
>>>   create mode 100644 include/qapi/qmp/qevent.h
>> Please move it qemu-schema.json instead.
> qapi-schema.json, but yes, I agree that declaring it as a qapi enum and
> letting the code generator create the constants, rather than moving the
> hand-maintained constants into a new header, is smarter.
>
    I will try use qapi-schema.json in next version.

Re: [Qemu-devel] [PATCH 05/13] error: define struct Error in only one place

[Wenchao Xia]

于 2013/10/18 19:22, Markus Armbruster 写道:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>  include/qapi/error.h |    5 ++++-
>>>  qobject/qerror.c     |    7 -------
>>>  util/error.c         |    6 ------
>>>  3 files changed, 4 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index 7d4c696..8688aaf 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -20,7 +20,10 @@
>>>   * A class representing internal errors within QEMU.  An error has a ErrorClass
>>>   * code and a human message.
>>>   */
>>> -typedef struct Error Error;
>>> +typedef struct Error {
>>> +    char *msg;
>>> +    ErrorClass err_class;
>>> +} Error;
>> Please add a comment that it should be treated as an opaque type.
> Or keep it opaque here, and complete the type in an internal header.
> But see below.
>
>> Paolo
>>
>>>  
>>>  /**
>>>   * Set an indirect pointer to an error given a ErrorClass value and a
>>> diff --git a/qobject/qerror.c b/qobject/qerror.c
>>> index 3aee1cf..5b487f3 100644
>>> --- a/qobject/qerror.c
>>> +++ b/qobject/qerror.c
>>> @@ -97,13 +97,6 @@ void qerror_report(ErrorClass eclass, const char *fmt, ...)
>>>      }
>>>  }
>>>  
>>> -/* Evil... */
>>> -struct Error
>>> -{
>>> -    char *msg;
>>> -    ErrorClass err_class;
>>> -};
>>> -
>>>  void qerror_report_err(Error *err)
>>>  {
>>>      QError *qerr;
>         qerr = qerror_new();
>         loc_save(&qerr->loc);
>         qerr->err_msg = g_strdup(err->msg);
>         qerr->err_class = err->err_class;
>
>         if (monitor_cur_is_qmp()) {
>             monitor_set_error(cur_mon, qerr);
>         } else {
>             qerror_print(qerr);
>             QDECREF(qerr);
>         }
>     }
>
> This is the only use of the "evil" duplicate.  I suspect it could be
> cleaned up like this:
>
>         qerr->err_msg = g_strdup(error_get_pretty(err));
>         qerr->err_class = error_get_class(err);
>
> If that's true, the duplicate goes away, and we can keep the type
> opaque.
seems a smart idea, will use it.


>>> diff --git a/util/error.c b/util/error.c
>>> index ec0faa6..da0d221 100644
>>> --- a/util/error.c
>>> +++ b/util/error.c
>>> @@ -17,12 +17,6 @@
>>>  #include "qapi-types.h"
>>>  #include "qapi/qmp/qerror.h"
>>>  
>>> -struct Error
>>> -{
>>> -    char *msg;
>>> -    ErrorClass err_class;
>>> -};
>>> -
>>>  void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>>>  {
>>>      Error *err;
>>>

Re: [Qemu-devel] [PATCH 08/13] error: don't set sep when print progname

[Wenchao Xia]

于 2013/10/18 19:40, Markus Armbruster 写道:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>>> The behavior to set sep brings trouble to modification later,
>>> the logic is not changed by add tailing space in fprintf().
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>  util/qemu-error.c |    5 ++---
>>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/util/qemu-error.c b/util/qemu-error.c
>>> index 0ccd3e9..d1e858a 100644
>>> --- a/util/qemu-error.c
>>> +++ b/util/qemu-error.c
>>> @@ -161,8 +161,7 @@ static void error_print_loc(void)
>>>      const char *const *argp;
>>>  
>>>      if (!cur_mon && progname) {
>>> -        fprintf(stderr, "%s:", progname);
>>> -        sep = " ";
>>> +        fprintf(stderr, "%s: ", progname);
>>>      }
>>>      switch (cur_loc->kind) {
>>>      case LOC_CMDLINE:
>>> @@ -181,7 +180,7 @@ static void error_print_loc(void)
>>>          error_printf(" ");
>>>          break;
>>>      default:
>>> -        error_printf("%s", sep);
>>> +        break;
>>>      }
>>>  }
>>>  
>>>
>> This changes behavior for LOC_FILE.
>>
>> Before:
>>
>> $ cat xyz.cfg
>> [device "abc"]
>>         driver = def
>> $ qemu-system-x86_64 -readconfig xyz.cfg
>> qemu-system-x86_64:xyz.cfg:2: parse error
>>
>> After:
>>
>> $ qemu-system-x86_64 -readconfig xyz.cfg
>> qemu-system-x86_64: xyz.cfg:2: parse error
>>
>> Could even be an improvement, but you need to note it in the commit message.
> No, it is not an improvement.  The old format matches exactly how other
> report errors with location, e.g. jade.  Please leave it that way,
>
I'll check whether there is way to leave the logic as it was.

Re: [Qemu-devel] [PATCH 09/13] error: print progname with error_vprintf()

[Wenchao Xia]

于 2013/10/18 17:44, Paolo Bonzini 写道:
> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>> This remove additional code path about where to print the error,
>> error_vprintf() is only the controller now, making future change
>> easier.
>>
>> The logic is not changed since when cur_mon = NULL, error_vprintf()
>> will still print to stderr.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   util/qemu-error.c |   11 ++++++++++-
>>   1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/util/qemu-error.c b/util/qemu-error.c
>> index d1e858a..c29fcbd 100644
>> --- a/util/qemu-error.c
>> +++ b/util/qemu-error.c
>> @@ -151,6 +151,15 @@ const char *error_get_progname(void)
>>       return progname;
>>   }
>>   
>> +static void error_print_progname(const char *fmt, ...)
>> +{
>> +    va_list ap;
>> +
>> +    va_start(ap, fmt);
>> +    error_vprintf(fmt, ap);
>> +    va_end(ap);
>> +}
>> +
>>   /*
>>    * Print current location to current monitor if we have one, else to stderr.
>>    */
>> @@ -161,7 +170,7 @@ static void error_print_loc(void)
>>       const char *const *argp;
>>   
>>       if (!cur_mon && progname) {
>> -        fprintf(stderr, "%s: ", progname);
>> +        error_print_progname("%s: ", progname);
>>       }
>>       switch (cur_loc->kind) {
>>       case LOC_CMDLINE:
>>
> I agree that using fprintf looks odd, but why not use error_printf directly?
>
> Paolo
>
I used custom function since I have a following modification in my 
private branch. Since
it is not send, I will use error_printf(), which is more straight.

Re: [Qemu-devel] [PATCH 10/13] qerror: deref once in qerror_report()

[Wenchao Xia]

于 2013/10/18 17:46, Paolo Bonzini 写道:
> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   qobject/qerror.c |    1 -
>>   1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/qobject/qerror.c b/qobject/qerror.c
>> index 5b487f3..685167a 100644
>> --- a/qobject/qerror.c
>> +++ b/qobject/qerror.c
>> @@ -77,7 +77,6 @@ static void qerror_print(QError *qerror)
>>       loc_push_restore(&qerror->loc);
>>       error_report("%s", qstring_get_str(qstring));
>>       loc_pop(&qerror->loc);
>> -    QDECREF(qstring);
>>   }
>>   
>>   void qerror_report(ErrorClass eclass, const char *fmt, ...)
>>
> Why isn't this a memory leak?
>
> Paolo
>
My bad, I mistake QDECREF(qstring) as QDECREF(qerror), will drop this 
path, sorry to disturb.

Re: [Qemu-devel] [PATCH 12/13] monitor: hide *cur_mon in monitor_get_fd()

[Wenchao Xia]

于 2013/10/18 17:51, Paolo Bonzini 写道:
> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>> All existing caller are using *cur_mon as its parameter, and *cur_mon
>> is an internal variable which used inside monitor.c. This patch reduce
>> the exposing of details in monitor.c, by introduce a new function
>> monitor_get_fd_cur() and make old one static.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   dump.c                    |    2 +-
>>   include/monitor/monitor.h |    2 +-
>>   migration-fd.c            |    2 +-
>>   monitor.c                 |    7 ++++++-
>>   qmp.c                     |    2 +-
>>   stubs/get-fd.c            |    2 +-
>>   util/qemu-sockets.c       |    4 ++--
>>   7 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/dump.c b/dump.c
>> index 846155c..8f5b6b0 100644
>> --- a/dump.c
>> +++ b/dump.c
>> @@ -860,7 +860,7 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
>>   
>>   #if !defined(WIN32)
>>       if (strstart(file, "fd:", &p)) {
>> -        fd = monitor_get_fd(cur_mon, p, errp);
>> +        fd = monitor_get_fd_cur(p, errp);
>>           if (fd == -1) {
>>               return;
>>           }
>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> index 97fcee3..637f7f3 100644
>> --- a/include/monitor/monitor.h
>> +++ b/include/monitor/monitor.h
>> @@ -35,7 +35,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device,
>>                                     BlockDriverCompletionFunc *completion_cb,
>>                                     void *opaque);
>>   
>> -int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
>> +int monitor_get_fd_cur(const char *fdname, Error **errp);
>>   int monitor_handle_fd_param(Monitor *mon, const char *fdname);
>>   
>>   void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>> diff --git a/migration-fd.c b/migration-fd.c
>> index d2e523a..022bc50 100644
>> --- a/migration-fd.c
>> +++ b/migration-fd.c
>> @@ -33,7 +33,7 @@
>>   
>>   void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
>>   {
>> -    int fd = monitor_get_fd(cur_mon, fdname, errp);
>> +    int fd = monitor_get_fd_cur(fdname, errp);
>>       if (fd == -1) {
>>           return;
>>       }
>> diff --git a/monitor.c b/monitor.c
>> index 9377834..80a9dfd 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2290,7 +2290,7 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
>>       }
>>   }
>>   
>> -int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>> +static int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>>   {
>>       mon_fd_t *monfd;
>>   
>> @@ -2315,6 +2315,11 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>>       return -1;
>>   }
>>   
>> +int monitor_get_fd_cur(const char *fdname, Error **errp)
>> +{
>> +    return monitor_get_fd(cur_mon, fdname, errp);
>> +}
>> +
>>   static void monitor_fdset_cleanup(MonFdset *mon_fdset)
>>   {
>>       MonFdsetFd *mon_fdset_fd;
>> diff --git a/qmp.c b/qmp.c
>> index 4c149b3..a02804b 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -493,7 +493,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
>>       CharDriverState *s;
>>       int fd;
>>   
>> -    fd = monitor_get_fd(cur_mon, fdname, errp);
>> +    fd = monitor_get_fd_cur(fdname, errp);
>>       if (fd < 0) {
>>           return;
>>       }
>> diff --git a/stubs/get-fd.c b/stubs/get-fd.c
>> index 9f2c65c..7d9ec3b 100644
>> --- a/stubs/get-fd.c
>> +++ b/stubs/get-fd.c
>> @@ -1,7 +1,7 @@
>>   #include "qemu-common.h"
>>   #include "monitor/monitor.h"
>>   
>> -int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
>> +int monitor_get_fd_cur(const char *name, Error **errp)
>>   {
>>       error_setg(errp, "only QEMU supports file descriptor passing");
>>       return -1;
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index 6b97dc1..9cd85dd 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -902,7 +902,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
>>           break;
>>   
>>       case SOCKET_ADDRESS_KIND_FD:
>> -        fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
>> +        fd = monitor_get_fd_cur(addr->fd->str, errp);
>>           if (fd >= 0 && callback) {
>>               qemu_set_nonblock(fd);
>>               callback(fd, opaque);
>> @@ -934,7 +934,7 @@ int socket_listen(SocketAddress *addr, Error **errp)
>>           break;
>>   
>>       case SOCKET_ADDRESS_KIND_FD:
>> -        fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
>> +        fd = monitor_get_fd_cur(addr->fd->str, errp);
>>           break;
>>   
>>       default:
>>
> Doesn't seem like an improvement.  It would be if you could then make
> cur_mon static.
>
> Paolo
>
OK, I will check all code using cur_mon and make it static.

Re: [Qemu-devel] [PATCH 13/13] stubs: do not call monitor_printf()

[Wenchao Xia]

于 2013/10/18 17:52, Paolo Bonzini 写道:
> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>> stubs/pci-drive-host-add.o is packaged together with /stubs/mon-printf.o
>> so it would not work normal, remove it.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   stubs/pci-drive-hot-add.c |    1 -
>>   1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/stubs/pci-drive-hot-add.c b/stubs/pci-drive-hot-add.c
>> index 1d98145..af4612f 100644
>> --- a/stubs/pci-drive-hot-add.c
>> +++ b/stubs/pci-drive-hot-add.c
>> @@ -5,6 +5,5 @@
>>   int pci_drive_hot_add(Monitor *mon, const QDict *qdict, DriveInfo *dinfo)
>>   {
>>       /* On non-x86 we don't do PCI hotplug */
>> -    monitor_printf(mon, "Can't hot-add drive to type %d\n", dinfo->type);
>>       return -1;
>>   }
>>
> This is wrong; stubs are independent, that's why each of them is in its
> own C file.
>
> When linking a version of QEMU that does not have pci_drive_hot_add,
> this function will call the monitor_printf from QEMU.
>
> Paolo
>
I see, will drop it.