[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 1/2] Add VIR_TYPED_PARAM_STRING



On 09/23/2011 12:40 AM, Hu Tao wrote:
This makes string can be transported between client and server.
For compatibility,

     o new server should not send strings to old client if it
       doesn't see the flag VIR_DOMAIN_TYPED_STRING_OKAY.
     o new client that wants to be able to send/receive strings
       should always set the flag VIR_DOMAIN_TYPED_STRING_OKAY;
       if it is rejected by a old server that doesn't understand
       VIR_DOMAIN_TYPED_STRING_OKAY, then the client should have
       a second try with out flag VIR_DOMAIN_TYPED_STRING_OKAY
       to cope with an old server.

These points should probably be documented in libvirt.h.in, as well.

I'm also thinking that libvirt.c should do some argument validation - for every API that uses a virTypedParameter array, it should validate that no VIR_TYPED_PARAM_STRING occurs if the flag is not present.

+++ b/daemon/remote.c
@@ -672,6 +672,15 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
          case VIR_TYPED_PARAM_BOOLEAN:
              val[i].value.remote_typed_param_value_u.b = params[i].value.b;
              break;
+        case VIR_TYPED_PARAM_STRING:
+            if (params[i].value.s) {
+                val[i].value.remote_typed_param_value_u.s = strdup(params[i].value.s);
+                if (val[i].value.remote_typed_param_value_u.s == NULL) {
+                    virReportOOMError();
+                    goto cleanup;
+                }
+            }
+            break;
          default:

Memory leak. The cleanup: label must free any successfully allocated remote_typed_param_value_u.s contents prior to the failure point.

              virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"),
                          params[i].type);
@@ -750,6 +759,14 @@ remoteDeserializeTypedParameters(remote_typed_param *args_params_val,
              params[i].value.b =
                  args_params_val[i].value.remote_typed_param_value_u.b;
              break;
+        case VIR_TYPED_PARAM_STRING:
+            params[i].value.s =
+                strdup(args_params_val[i].value.remote_typed_param_value_u.s);
+            if (params[i].value.s == NULL) {
+                virReportOOMError();
+                goto cleanup;
+            }
+            break;
          default:
              virNetError(VIR_ERR_INTERNAL_ERROR, _("unknown parameter type: %d"),
                          params[i].type);

Memory leak. The cleanup: label must now iterate over params, and free any successfully allocated params[i].value.s allocated prior to a failure.

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 39155a6..448a0e7 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -205,6 +205,7 @@ typedef enum {
      VIR_DOMAIN_AFFECT_CURRENT = 0,      /* Affect current domain state.  */
      VIR_DOMAIN_AFFECT_LIVE    = 1<<  0, /* Affect running domain state.  */
      VIR_DOMAIN_AFFECT_CONFIG  = 1<<  1, /* Affect persistent domain state.  */
+    VIR_DOMAIN_TYPED_STRING_OKAY = 1<<  2,
  } virDomainModificationImpact;

I'm not sure if this is the best place to stick this enum value; it might fit better if it is listed (and documented!) closer to the rest of virTypedParameterType stuff. But I agree with setting it to the value of 1<<2, since all current clients of virTypedParameterType seem to be using VIR_DOMAIN_AFFECT_* and nothing further.


  /**
@@ -489,7 +490,8 @@ typedef enum {
      VIR_TYPED_PARAM_LLONG   = 3, /* long long case */
      VIR_TYPED_PARAM_ULLONG  = 4, /* unsigned long long case */
      VIR_TYPED_PARAM_DOUBLE  = 5, /* double case */
-    VIR_TYPED_PARAM_BOOLEAN = 6  /* boolean(character) case */
+    VIR_TYPED_PARAM_BOOLEAN = 6, /* boolean(character) case */
+    VIR_TYPED_PARAM_STRING  = 7  /* string case */

Put a trailing comma after 7, so that the next addition (if any) doesn't have to touch two lines like this addition did.

  } virTypedParameterType;

  /**
@@ -520,6 +522,7 @@ struct _virTypedParameter {
          unsigned long long int ul;  /* type is ULLONG */
          double d;                   /* type is DOUBLE */
          char b;                     /* type is BOOLEAN */
+        char *s;                    /* type is STRING */

Here, I'd use:

char *s; /* type is STRING, see also VIR_DOMAIN_TYPED_STRING_OKAY */

      } value; /* parameter value */
  };

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 1217d94..85eaeea 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1276,6 +1276,15 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
          case VIR_TYPED_PARAM_BOOLEAN:
              val[i].value.remote_typed_param_value_u.b = params[i].value.b;
              break;
+        case VIR_TYPED_PARAM_STRING:
+            if (params[i].value.s) {
+                val[i].value.remote_typed_param_value_u.s = strdup(params[i].value.s);
+                if (val[i].value.remote_typed_param_value_u.s == NULL) {
+                    virReportOOMError();
+                    goto cleanup;
+                }
+            }
+            break;

Counterpart memory leak.

          default:
              remoteError(VIR_ERR_RPC, _("unknown parameter type: %d"),
                  params[i].type);
@@ -1347,6 +1356,14 @@ remoteDeserializeTypedParameters(remote_typed_param *ret_params_val,
              params[i].value.b =
                  ret_params_val[i].value.remote_typed_param_value_u.b;
              break;
+        case VIR_TYPED_PARAM_STRING:
+            params[i].value.s =
+                strdup(ret_params_val[i].value.remote_typed_param_value_u.s);
+            if (params[i].value.s == NULL) {
+                virReportOOMError();
+                goto cleanup;
+            }
+            break;

And another memory leak.

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]