[libvirt] [PATCH v3 1/2] Add VIR_TYPED_PARAM_STRING
Eric Blake
eblake at redhat.com
Fri Sep 23 17:08:51 UTC 2011
On 09/23/2011 12:40 AM, 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.
These points should probably be documented in libvirt.h.in, as well.
I'm also thinking that libvirt.c should do some argument validation -
for every API that uses a virTypedParameter array, it should validate
that no VIR_TYPED_PARAM_STRING occurs if the flag is not present.
> +++ b/daemon/remote.c
> @@ -672,6 +672,15 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
> case VIR_TYPED_PARAM_BOOLEAN:
> val[i].value.remote_typed_param_value_u.b = params[i].value.b;
> break;
> + case VIR_TYPED_PARAM_STRING:
> + if (params[i].value.s) {
> + val[i].value.remote_typed_param_value_u.s = strdup(params[i].value.s);
> + if (val[i].value.remote_typed_param_value_u.s == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + }
> + break;
> default:
Memory leak. The cleanup: label must free any successfully allocated
remote_typed_param_value_u.s contents prior to the failure point.
> virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"),
> params[i].type);
> @@ -750,6 +759,14 @@ remoteDeserializeTypedParameters(remote_typed_param *args_params_val,
> params[i].value.b =
> args_params_val[i].value.remote_typed_param_value_u.b;
> break;
> + case VIR_TYPED_PARAM_STRING:
> + params[i].value.s =
> + strdup(args_params_val[i].value.remote_typed_param_value_u.s);
> + if (params[i].value.s == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + break;
> default:
> virNetError(VIR_ERR_INTERNAL_ERROR, _("unknown parameter type: %d"),
> params[i].type);
Memory leak. The cleanup: label must now iterate over params, and free
any successfully allocated params[i].value.s allocated prior to a failure.
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 39155a6..448a0e7 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -205,6 +205,7 @@ typedef enum {
> VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. */
> VIR_DOMAIN_AFFECT_LIVE = 1<< 0, /* Affect running domain state. */
> VIR_DOMAIN_AFFECT_CONFIG = 1<< 1, /* Affect persistent domain state. */
> + VIR_DOMAIN_TYPED_STRING_OKAY = 1<< 2,
> } virDomainModificationImpact;
I'm not sure if this is the best place to stick this enum value; it
might fit better if it is listed (and documented!) closer to the rest of
virTypedParameterType stuff. But I agree with setting it to the value
of 1<<2, since all current clients of virTypedParameterType seem to be
using VIR_DOMAIN_AFFECT_* and nothing further.
>
> /**
> @@ -489,7 +490,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 */
Put a trailing comma after 7, so that the next addition (if any) doesn't
have to touch two lines like this addition did.
> } virTypedParameterType;
>
> /**
> @@ -520,6 +522,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 */
Here, I'd use:
char *s; /* type is STRING, see also VIR_DOMAIN_TYPED_STRING_OKAY */
> } value; /* parameter value */
> };
>
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 1217d94..85eaeea 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -1276,6 +1276,15 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
> case VIR_TYPED_PARAM_BOOLEAN:
> val[i].value.remote_typed_param_value_u.b = params[i].value.b;
> break;
> + case VIR_TYPED_PARAM_STRING:
> + if (params[i].value.s) {
> + val[i].value.remote_typed_param_value_u.s = strdup(params[i].value.s);
> + if (val[i].value.remote_typed_param_value_u.s == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + }
> + break;
Counterpart memory leak.
> default:
> remoteError(VIR_ERR_RPC, _("unknown parameter type: %d"),
> params[i].type);
> @@ -1347,6 +1356,14 @@ remoteDeserializeTypedParameters(remote_typed_param *ret_params_val,
> params[i].value.b =
> ret_params_val[i].value.remote_typed_param_value_u.b;
> break;
> + case VIR_TYPED_PARAM_STRING:
> + params[i].value.s =
> + strdup(ret_params_val[i].value.remote_typed_param_value_u.s);
> + if (params[i].value.s == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + break;
And another memory leak.
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list