Re: [libvirt] [PATCHv3] qemu: monitor: Refactor and fix monitor checking

2015-04-16 Thread Peter Krempa
On Thu, Apr 16, 2015 at 14:38:25 +0200, Jiri Denemark wrote:
 On Wed, Apr 15, 2015 at 16:27:35 +0200, Peter Krempa wrote:
  Among all the monitor APIs some where checking if mon is NULL and some
  were not. Since it's possible to have mon equal to NULL in case a second
  call is attempted once entered the monitor. This requires that every
  single API checks for the monitor.
  
  This patch adds a macro that helps checking the state of the monitor and
  either refactors existing checking code to use the macro or adds it in
  case it was missing.
  ---
  
  Notes:
  Version 3:
  - fixed the check in qemuMonitorGetAllBlockStatsInfo by moving it 
  before the allocation
  - added macros for all the return value combinations
  - removed 'mon' from existing VIR_DEBUG macros
  
   src/qemu/qemu_monitor.c | 1151 
  ---
   src/qemu/qemu_monitor.h |   18 +-
   2 files changed, 292 insertions(+), 877 deletions(-)
  
  diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
  index 1f95547..6d7562d 100644
  --- a/src/qemu/qemu_monitor.c
  +++ b/src/qemu/qemu_monitor.c
 ...
  @@ -4418,16 +3839,12 @@ int
   qemuMonitorGetMemoryDeviceInfo(qemuMonitorPtr mon,
  virHashTablePtr *info)
   {
  -VIR_DEBUG(mon=%p info=%p, mon, info);
  +VIR_DEBUG(info=%p, info);
   int ret;
  
   *info = NULL;
  
  -if (!mon) {
  -virReportError(VIR_ERR_INVALID_ARG, %s,
  -   _(monitor must not be NULL));
  -return -1;
  -}
  +QEMU_CHECK_MONITOR_JSON(mon);
 
 s/QEMU_CHECK_MONITOR_JSON/QEMU_CHECK_MONITOR/ because of:

oh, right. I screwed up the last one :)

 
   if (!mon-json)
   return -2;
 
 The rest looks correct.
 
 ACK once you fix this last issue.

Pushed; Thanks.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv3] qemu: monitor: Refactor and fix monitor checking

2015-04-16 Thread Jiri Denemark
On Wed, Apr 15, 2015 at 16:27:35 +0200, Peter Krempa wrote:
 Among all the monitor APIs some where checking if mon is NULL and some
 were not. Since it's possible to have mon equal to NULL in case a second
 call is attempted once entered the monitor. This requires that every
 single API checks for the monitor.
 
 This patch adds a macro that helps checking the state of the monitor and
 either refactors existing checking code to use the macro or adds it in
 case it was missing.
 ---
 
 Notes:
 Version 3:
 - fixed the check in qemuMonitorGetAllBlockStatsInfo by moving it before 
 the allocation
 - added macros for all the return value combinations
 - removed 'mon' from existing VIR_DEBUG macros
 
  src/qemu/qemu_monitor.c | 1151 
 ---
  src/qemu/qemu_monitor.h |   18 +-
  2 files changed, 292 insertions(+), 877 deletions(-)
 
 diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
 index 1f95547..6d7562d 100644
 --- a/src/qemu/qemu_monitor.c
 +++ b/src/qemu/qemu_monitor.c
...
 @@ -4418,16 +3839,12 @@ int
  qemuMonitorGetMemoryDeviceInfo(qemuMonitorPtr mon,
 virHashTablePtr *info)
  {
 -VIR_DEBUG(mon=%p info=%p, mon, info);
 +VIR_DEBUG(info=%p, info);
  int ret;
 
  *info = NULL;
 
 -if (!mon) {
 -virReportError(VIR_ERR_INVALID_ARG, %s,
 -   _(monitor must not be NULL));
 -return -1;
 -}
 +QEMU_CHECK_MONITOR_JSON(mon);

s/QEMU_CHECK_MONITOR_JSON/QEMU_CHECK_MONITOR/ because of:

  if (!mon-json)
  return -2;

The rest looks correct.

ACK once you fix this last issue.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


[libvirt] [PATCHv3] qemu: monitor: Refactor and fix monitor checking

2015-04-15 Thread Peter Krempa
Among all the monitor APIs some where checking if mon is NULL and some
were not. Since it's possible to have mon equal to NULL in case a second
call is attempted once entered the monitor. This requires that every
single API checks for the monitor.

This patch adds a macro that helps checking the state of the monitor and
either refactors existing checking code to use the macro or adds it in
case it was missing.
---

Notes:
Version 3:
- fixed the check in qemuMonitorGetAllBlockStatsInfo by moving it before 
the allocation
- added macros for all the return value combinations
- removed 'mon' from existing VIR_DEBUG macros

 src/qemu/qemu_monitor.c | 1151 ---
 src/qemu/qemu_monitor.h |   18 +-
 2 files changed, 292 insertions(+), 877 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 1f95547..6d7562d 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -98,6 +98,47 @@ struct _qemuMonitor {
 int logfd;
 };

+/**
+ * QEMU_CHECK_MONITOR_FULL:
+ * @mon: monitor pointer variable to check, evaluated multiple times, no 
parentheses
+ * @force_json: force JSON monitor, true or false
+ * @exit: statement that is used to exit the function
+ *
+ * This macro checks that the monitor is valid for given operation and exits
+ * the function if not. The macro also adds a debug statement regarding the
+ * monitor.
+ */
+#define QEMU_CHECK_MONITOR_FULL(mon, force_json, exit) 
\
+if (!mon) {
\
+virReportError(VIR_ERR_INVALID_ARG, %s,  
\
+   _(monitor must not be NULL)); 
\
+exit;  
\
+}  
\
+VIR_DEBUG(mon:%p vm:%p json:%d fd:%d, mon, mon-vm, mon-json, mon-fd); 
\
+if (force_json  !mon-json) {
\
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, %s,
\
+   _(JSON monitor is required)); 
\
+exit;  
\
+}
+
+/* Check monitor and return NULL on error */
+#define QEMU_CHECK_MONITOR_NULL(mon) \
+QEMU_CHECK_MONITOR_FULL(mon, false, return NULL)
+#define QEMU_CHECK_MONITOR_JSON_NULL(mon) \
+QEMU_CHECK_MONITOR_FULL(mon, true, return NULL)
+
+/* Check monitor and return -1 on error */
+#define QEMU_CHECK_MONITOR(mon) \
+QEMU_CHECK_MONITOR_FULL(mon, false, return -1)
+#define QEMU_CHECK_MONITOR_JSON(mon) \
+QEMU_CHECK_MONITOR_FULL(mon, true, return -1)
+
+/* Check monitor and jump to the provided label */
+#define QEMU_CHECK_MONITOR_GOTO(mon, label) \
+QEMU_CHECK_MONITOR_FULL(mon, false, goto label)
+#define QEMU_CHECK_MONITOR_JSON_GOTO(mon, label) \
+QEMU_CHECK_MONITOR_FULL(mon, true, goto label)
+
 static virClassPtr qemuMonitorClass;
 static void qemuMonitorDispose(void *obj);

@@ -1188,6 +1229,8 @@ qemuMonitorUpdateVideoMemorySize(qemuMonitorPtr mon,
 int ret = -1;
 char *path = NULL;

+QEMU_CHECK_MONITOR(mon);
+
 if (mon-json) {
 ret = qemuMonitorFindObjectPath(mon, /, videoName, path);
 if (ret  0) {
@@ -1216,6 +1259,8 @@ qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon,
 char *json_cmd = NULL;
 int ret = -1;

+QEMU_CHECK_MONITOR(mon);
+
 if (mon-json) {
 /* hack to avoid complicating each call to text monitor functions */
 json_cmd = qemuMonitorUnescapeArg(cmd);
@@ -1527,13 +1572,7 @@ qemuMonitorEmitSerialChange(qemuMonitorPtr mon,
 int
 qemuMonitorSetCapabilities(qemuMonitorPtr mon)
 {
-VIR_DEBUG(mon=%p, mon);
-
-if (!mon) {
-virReportError(VIR_ERR_INVALID_ARG, %s,
-   _(monitor must not be NULL));
-return -1;
-}
+QEMU_CHECK_MONITOR(mon);

 if (!mon-json)
 return 0;
@@ -1546,13 +1585,7 @@ int
 qemuMonitorStartCPUs(qemuMonitorPtr mon,
  virConnectPtr conn)
 {
-VIR_DEBUG(mon=%p, mon);
-
-if (!mon) {
-virReportError(VIR_ERR_INVALID_ARG, %s,
-   _(monitor must not be NULL));
-return -1;
-}
+QEMU_CHECK_MONITOR(mon);

 if (mon-json)
 return qemuMonitorJSONStartCPUs(mon, conn);
@@ -1564,13 +1597,7 @@ qemuMonitorStartCPUs(qemuMonitorPtr mon,
 int
 qemuMonitorStopCPUs(qemuMonitorPtr mon)
 {
-VIR_DEBUG(mon=%p, mon);
-
-if (!mon) {
-virReportError(VIR_ERR_INVALID_ARG, %s,
-   _(monitor must not be NULL));
-return -1;
-}
+QEMU_CHECK_MONITOR(mon);

 if (mon-json)
 return qemuMonitorJSONStopCPUs(mon);
@@ -1584,13 +1611,9 @@ qemuMonitorGetStatus(qemuMonitorPtr mon,
  bool *running,