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

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



On Wed, Oct 12, 2011 at 05:26:34PM +0800, Hu Tao wrote:
> This makes string can be transported between client and server.
> For compatibility,
> 
>     o new server should not send strings to old client if it
>       doesn't see the flag VIR_DOMAIN_TYPED_STRING_OKAY.
>     o new client that wants to be able to send/receive strings
>       should always set the flag VIR_DOMAIN_TYPED_STRING_OKAY;
>       if it is rejected by a old server that doesn't understand
>       VIR_DOMAIN_TYPED_STRING_OKAY, then the client should have
>       a second try with out flag VIR_DOMAIN_TYPED_STRING_OKAY
>       to cope with an old server.
> 
> Ideas for compatibility are coming from Eric, thanks.
> ---
>  daemon/remote.c              |   32 +++++++++++++++++++++++++++++---
>  include/libvirt/libvirt.h.in |   25 ++++++++++++++++++++++++-
>  src/libvirt.c                |   38 ++++++++++++++++++++++++++++++++++++++
>  src/remote/remote_driver.c   |   30 ++++++++++++++++++++++++++++--
>  src/remote/remote_protocol.x |    2 ++
>  src/remote_protocol-structs  |    2 ++
>  src/rpc/gendispatch.pl       |    2 +-
>  src/util/util.c              |   15 +++++++++++++++
>  src/util/util.h              |    2 ++
>  9 files changed, 141 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index c991dfc..fa53147 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -208,6 +208,27 @@ typedef enum {
>  } virDomainModificationImpact;
>  
>  /**
> + * virDomainFlags:
> + *
> + * Flags that can be used with some libvirt APIs.
> + *
> + * These enums should not confilict with those of virDomainModificationImpact.
> + */
> +typedef enum {
> +    VIR_DOMAIN_TYPED_STRING_OKAY = 1 << 2, /*  Usage of this flag:
> +                                            *    o new server should not send strings to old client if it
> +                                            *      doesn't see the flag VIR_DOMAIN_TYPED_STRING_OKAY.
> +                                            *    o new client that wants to be able to send/receive strings
> +                                            *      should always set the flag VIR_DOMAIN_TYPED_STRING_OKAY;
> +                                            *      if it is rejected by a old server that doesn't understand
> +                                            *      VIR_DOMAIN_TYPED_STRING_OKAY, then the client should have
> +                                            *      a second try without flag VIR_DOMAIN_TYPED_STRING_OKAY to
> +                                            *      cope with an old server.
> +                                            */
> +
> +} virDomainFlags;

I'm not really a fan of this - it is exposing details of
the RPC implementation to the applications. IMHO we should
be using the internal RPC features capability, and the
VIR_DRV_SUPPORTS_FEATURE to detect support internally,
and then return 'VIR_ERR_NO_SUPPORT' if string parameters
are not supported.

> diff --git a/src/libvirt.c b/src/libvirt.c
> index f07c720..59d6d26 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -3472,6 +3489,9 @@ virDomainSetMemoryParameters(virDomainPtr domain,
>  
>      virResetLastError();
>  
> +    if (virDomainCheckTypedStringFlag(params, nparams, flags) < 0)
> +        return -1;
> +
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
>          virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          virDispatchError(NULL);
> @@ -3547,6 +3567,9 @@ virDomainGetMemoryParameters(virDomainPtr domain,
>  
>      virResetLastError();
>  
> +    if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0)
> +        return -1;
> +
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
>          virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          virDispatchError(NULL);
> @@ -3598,6 +3621,9 @@ virDomainSetBlkioParameters(virDomainPtr domain,
>  
>      virResetLastError();
>  
> +    if (virDomainCheckTypedStringFlag(params, nparams, flags) < 0)
> +        return -1;
> +
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
>          virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          virDispatchError(NULL);
> @@ -3657,6 +3683,9 @@ virDomainGetBlkioParameters(virDomainPtr domain,
>  
>      virResetLastError();
>  
> +    if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0)
> +        return -1;
> +
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
>          virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          virDispatchError(NULL);
> @@ -6279,6 +6308,9 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain,
>  
>      virResetLastError();
>  
> +    if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0)
> +        return -1;
> +
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
>          virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          virDispatchError(NULL);
> @@ -6396,6 +6428,9 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain,
>  
>      virResetLastError();
>  
> +    if (virDomainCheckTypedStringFlag(params, nparams, flags) < 0)
> +        return -1;
> +
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
>          virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          virDispatchError(NULL);
> @@ -6537,6 +6572,9 @@ int virDomainBlockStatsFlags (virDomainPtr dom,
>  
>      virResetLastError();
>  
> +    if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0)
> +        return -1;
> +
>      if (!VIR_IS_CONNECTED_DOMAIN (dom)) {
>          virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          virDispatchError(NULL);

So IMHO all these should be VIR_DRV_SUPPORTS_FEATURE(VIR_DRV_FEATURE_TYPED_STRING)
and if the app gets back an error, it can re-try without the string parameter.


> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index f95253e..994c339 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -317,6 +317,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;
>  };
>  
>  struct remote_typed_param {

I'm kind of suprised this actually works, but I presume
you've tested old client with new server, and vica-verca
so I guess its OK.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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