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.