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