[PATCH] opw: libxl: change remaining LIBXL_LOG to LOG in libxl_qmp.c

[PATCH] opw: libxl: change remaining LIBXL_LOG to LOG in libxl_qmp.c

[Kelley Nielsen]

Code cleanup -- no functional changes

Coding style has recently been changed for libxl. The convenience
macro LOG() has been introduced, and invocations of the old macro
LIBXL__LOG() are to be replaced with it. Change all remaining
occurences of the old macro to the new one. These are in functions
that do not have a local libxl__gc *gc, so create one by calling
GC_INIT() at the top of the function and destroy it by calling
GC_FREE at the end. In functions that have multiple exits, create
a labeled set of statements at the end with GC_FREE and the
return statement, and route all exits through it with gotos.

Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>
---
 tools/libxl/libxl_qmp.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 1f76d53..347ff9b 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -113,6 +113,7 @@ static int register_serials_chardev_callback(libxl__qmp_handler *qmp,
                                              const libxl__json_object *o,
                                              void *unused)
 {
+    GC_INIT(qmp->ctx);
     const libxl__json_object *obj = NULL;
     const libxl__json_object *label = NULL;
     const char *s = NULL;
@@ -137,20 +138,22 @@ static int register_serials_chardev_callback(libxl__qmp_handler *qmp,
             s += strlen("serial");
             port_number = strtol(s, &endptr, 10);
             if (*s == 0 || *endptr != 0) {
-                LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR,
-                           "Invalid serial port number: %s", s);
-                return -1;
+                LOG(ERROR, "Invalid serial port number: %s", s);
+                ret = -1;
+		goto out;
             }
             ret = store_serial_port_info(qmp, chardev, port_number);
             if (ret) {
                 LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR,
                                  "Failed to store serial port information"
                                  " in xenstore");
-                return ret;
+                goto out;
             }
         }
     };
 
+out:
+    GC_FREE;
     return ret;
 }
 
@@ -263,6 +266,7 @@ static callback_id_pair *qmp_get_callback_from_id(libxl__qmp_handler *qmp,
 static void qmp_handle_error_response(libxl__qmp_handler *qmp,
                                       const libxl__json_object *resp)
 {
+    GC_INIT(qmp->ctx);
     callback_id_pair *pp = qmp_get_callback_from_id(qmp, resp);
 
     resp = libxl__json_map_get("error", resp, JSON_MAP);
@@ -283,19 +287,20 @@ static void qmp_handle_error_response(libxl__qmp_handler *qmp,
         free(pp);
     }
 
-    LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR,
-               "received an error message from QMP server: %s",
+    LOG(ERROR, "received an error message from QMP server: %s",
                libxl__json_object_get_string(resp));
+    GC_FREE;
 }
 
 static int qmp_handle_response(libxl__qmp_handler *qmp,
                                const libxl__json_object *resp)
 {
+    GC_INIT(qmp->ctx);
     libxl__qmp_message_type type = LIBXL__QMP_MESSAGE_TYPE_INVALID;
+    int ret = 0;
 
     type = qmp_response_type(qmp, resp);
-    LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG,
-               "message type: %s", libxl__qmp_message_type_to_string(type));
+    LOG(DEBUG, "message type: %s", libxl__qmp_message_type_to_string(type));
 
     switch (type) {
     case LIBXL__QMP_MESSAGE_TYPE_QMP:
@@ -321,17 +326,22 @@ static int qmp_handle_response(libxl__qmp_handler *qmp,
                 &qmp->callback_list, pp, callback_id_pair, next);
             free(pp);
         }
-        return 0;
+        goto out;
     }
     case LIBXL__QMP_MESSAGE_TYPE_ERROR:
         qmp_handle_error_response(qmp, resp);
-        return -1;
+        ret = -1;
+	goto out;
     case LIBXL__QMP_MESSAGE_TYPE_EVENT:
-        return 0;
+        goto out;
     case LIBXL__QMP_MESSAGE_TYPE_INVALID:
-        return -1;
+        ret = -1;
+	goto out;
     }
-    return 0;
+
+out:
+    GC_FREE;
+    return ret;
 }
 
 /*
-- 
1.8.1.2

Re: [PATCH] opw: libxl: change remaining LIBXL_LOG to LOG in libxl_qmp.c

[Ian Campbell]

On Sat, 2013-11-09 at 20:05 -0800, Kelley Nielsen wrote:
> Code cleanup -- no functional changes
> 
> Coding style has recently been changed for libxl. The convenience
> macro LOG() has been introduced, and invocations of the old macro
> LIBXL__LOG() are to be replaced with it. Change all remaining
> occurences of the old macro to the new one. These are in functions
> that do not have a local libxl__gc *gc, so create one by calling
> GC_INIT() at the top of the function and destroy it by calling
> GC_FREE at the end. In functions that have multiple exits, create
> a labeled set of statements at the end with GC_FREE and the
> return statement, and route all exits through it with gotos.
> 
> Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>

Thanks but I'm afraid I don't think this is the correct approach.

The GC should normally be created upon entry into the library and
cleaned up on return to the application. So I think the correct approach
here would be to plumb through the gc from the callers. e.g. in the
qmp_synchronous_send case that would mean adding a gc parameter to
libxl__qmp_query_serial and _vnc and passing the gc from
libxl__qmp_initializations, as well as passing in the existing gc from
qmp_run_command, libxl__qmp_pci_add and qmp_change (plus any I have
missed)

Ian.