[libvirt] [PATCH 1/2] Add VIR_TYPED_PARAM_STRING

[Daniel Veillard]

On Tue, Sep 06, 2011 at 07:09:53AM -0600, Eric Blake wrote:
> 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,

  Well on an hypothetic arch with 128 bits pointers, that would have
broken the ABI since none of the existing type in the union are more
than 64 bits, but right in this case I think we are safe but let's check
this.

> >>+++ 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?

  yes as I said we need some testing, but we have no API yet making use
of strings.

> >>  };
> >>
> >>  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

  Yeah I got confused, I had checked on what I though was a 0.9.4
  checkout but I made a mistake...

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

  Agreed,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/