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

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



On 09/06/2011 04:44 AM, Daniel Veillard wrote:
On Tue, Sep 06, 2011 at 06:18:40PM +0800, Hu Tao wrote:
---
  daemon/remote.c              |   15 +++++++++++++++
  include/libvirt/libvirt.h.in |    4 +++-
  src/remote/remote_driver.c   |   15 +++++++++++++++
  src/remote/remote_protocol.x |    2 ++
  4 files changed, 35 insertions(+), 1 deletions(-)

+++ b/include/libvirt/libvirt.h.in
@@ -481,7 +481,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 */

New enum value is probably okay,

  } virTypedParameterType;

  /**
@@ -512,6 +513,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 */

and new char * field to a union doesn't change the struct size,

+++ b/src/remote/remote_protocol.x
@@ -314,6 +314,8 @@ union remote_typed_param_value switch (int type) {
       double d;
   case VIR_TYPED_PARAM_BOOLEAN:
       int b;
+ case VIR_TYPED_PARAM_STRING:
+     remote_nonnull_string s;

but I'm quite worried about the on-the-wire compatibility aspect of this change. If a new server sends the new enum value and a string, will the old server that does not know that enum value properly reject the rpc call, or will it cause a crash or other bad things to happen?

  };

  struct remote_typed_param {

   Ohhh, good point and better to add this before the first official
release to avoid possible ABI breakages in the future if we need it !

Huh? remote_typed_param has already had an official release. The addition is to a union, but this really is a case of modifying an existing on-the-wire layout, so I hope that the fact that it was an enumerated union with a new enumeration value is what is making this safe.

Also, before you push this, please install pdwtags (from the dwarves package if you use Fedora), and fix src/remote_protocol-structs so that 'make check' passes. Actually, please post that diff before pushing the patch - I want to verify that the size of remote_typed_param doesn't change; if it does change, then we have an incompatible change, and need to think of a different approach.

--
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]