[libvirt] [PATCH 1/2] Add VIR_TYPED_PARAM_STRING

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org