[libvirt] [PATCH 0/5] Export remote typed params methods internally via util
Erik Skultety (5): cfg.mk: Adjust sc_prohibit_int_ijk to support 'exempt from syntax-check' util: Introduce virTypedParameterRemote datatype util: Export remoteDeserializeTypedParameters internally via util util: Export remoteFreeTypedParameters internally via util util: Export remoteSerializeTypedParameters internally via util cfg.mk | 3 +- daemon/remote.c | 218 ++++++--------------------------------- src/libvirt_private.syms | 3 + src/remote/remote_driver.c | 196 +++--------------------------------- src/rpc/gendispatch.pl | 9 +- src/util/virtypedparam.c | 246 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virtypedparam.h | 40 ++++++++ 7 files changed, 341 insertions(+), 374 deletions(-) -- 2.4.3
There might be cases, like with typed params, where triggering this check isn't desirable. But including the whole module in the exception regex is not always to right way of doing things. By adding an option to manually disable this check on a specific occurrence, the module itself will still be checked against the rule. --- cfg.mk | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index b009b28..9bcbd5d 100644 --- a/cfg.mk +++ b/cfg.mk @@ -571,7 +571,8 @@ sc_prohibit_int_index: $(_sc_search_regexp) sc_prohibit_int_ijk: - @prohibit='\<(int|unsigned) ([^(=]* )*(i|j|k)\>(\s|,|;)' \ + @prohibit='\<(int|unsigned) ([^(=]* )*(i|j|k)\>(\s|,|;) *\(' \ + exclude='exempt from syntax-check' \ halt='use size_t, not int/unsigned int for loop vars i, j, k' \ $(_sc_search_regexp) -- 2.4.3
On Tue, Feb 02, 2016 at 04:46:17PM +0100, Erik Skultety wrote:
There might be cases, like with typed params, where triggering this check isn't desirable. But including the whole module in the exception regex is not always to right way of doing things. By adding an option to manually disable this check on a specific occurrence, the module itself will still be checked against the rule. --- cfg.mk | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk index b009b28..9bcbd5d 100644 --- a/cfg.mk +++ b/cfg.mk @@ -571,7 +571,8 @@ sc_prohibit_int_index: $(_sc_search_regexp)
sc_prohibit_int_ijk: - @prohibit='\<(int|unsigned) ([^(=]* )*(i|j|k)\>(\s|,|;)' \ + @prohibit='\<(int|unsigned) ([^(=]* )*(i|j|k)\>(\s|,|;) *\(' \
The prohibit line should be left unchanged.
+ exclude='exempt from syntax-check' \
ACK to this change. Jan
Both admin and remote protocols define their own types (remote_typed_param vs admin_typed_param). Because of the naming convention, admin typed params wouldn't be able to reuse the serialization/deserialization methods, which are tailored for use by remote protocol, even if those method were exported properly. In that case, introduce a new internal data type structurally copying both admin and remote protocols which, eventually, would allow serializer and deserializer to be used in a more generic way. --- src/util/virtypedparam.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 9bef204..7dd3a78 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -36,6 +36,30 @@ verify(!(VIR_TYPED_PARAM_LAST & VIR_TYPED_PARAM_MULTIPLE)); +typedef struct _virTypedParameterRemoteValue virTypedParameterRemoteValue; +typedef struct virTypedParameterRemoteValue *virTypedParameterRemoteValuePtr; + +struct _virTypedParameterRemoteValue { + int type; + union { + int i; /* exempt from syntax-check */ + unsigned int ui; + long long int l; + unsigned long long int ul; + double d; + char b; + char *s; + } remote_typed_param_value; +}; + +typedef struct _virTypedParameterRemote *virTypedParameterRemotePtr; + +struct _virTypedParameterRemote { + char *field; + virTypedParameterRemoteValue value; +}; + + int virTypedParamsValidate(virTypedParameterPtr params, int nparams, /* const char *name, int type ... */ ...) ATTRIBUTE_SENTINEL ATTRIBUTE_RETURN_CHECK; -- 2.4.3
Currently, the deserializer is hardcoded into remote_driver which makes it impossible for admin to use it. One way to achieve a shared implementation (besides moving the code to another module) would be pass @ret_params_val as a void pointer as opposed to the remote_typed_param pointer and add a new extra argument specifying which of those two protocols is being used and typecast the pointer at the function entry. An example from remote_protocol: struct remote_typed_param_value { int type; union { int i; u_int ui; int64_t l; uint64_t ul; double d; int b; remote_nonnull_string s; } remote_typed_param_value_u; }; typedef struct remote_typed_param_value remote_typed_param_value; struct remote_typed_param { remote_nonnull_string field; remote_typed_param_value value; }; That would leave us with a bunch of if-then-elses that needed to be used across the method. This patch takes the other approach using the new datatype introduced in one of earlier commits. --- daemon/remote.c | 125 +++++++++----------------------------------- src/libvirt_private.syms | 1 + src/remote/remote_driver.c | 107 ++------------------------------------ src/rpc/gendispatch.pl | 9 ++-- src/util/virtypedparam.c | 127 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virtypedparam.h | 7 +++ 6 files changed, 169 insertions(+), 207 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 370f442..f76a176 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -101,11 +101,12 @@ static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr static void make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst, virNWFilterPtr nwfilter_src); static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src); -static virTypedParameterPtr -remoteDeserializeTypedParameters(remote_typed_param *args_params_val, - u_int args_params_len, - int limit, - int *nparams); +#define remoteDeserializeTypedParameters(remote_params_val, \ + remote_params_len, \ + limit, params, nparams) \ + virTypedParamsDeserialize(__FUNCTION__, \ + (virTypedParameterRemotePtr) remote_params_val, \ + remote_params_len, limit, params, nparams) static int remoteSerializeTypedParameters(virTypedParameterPtr params, @@ -1495,84 +1496,6 @@ remoteSerializeTypedParameters(virTypedParameterPtr params, return rv; } -/* Helper to deserialize typed parameters. */ -static virTypedParameterPtr -remoteDeserializeTypedParameters(remote_typed_param *args_params_val, - u_int args_params_len, - int limit, - int *nparams) -{ - size_t i = 0; - int rv = -1; - virTypedParameterPtr params = NULL; - - /* Check the length of the returned list carefully. */ - if (limit && args_params_len > limit) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - if (VIR_ALLOC_N(params, args_params_len) < 0) - goto cleanup; - - *nparams = args_params_len; - - /* Deserialise the result. */ - for (i = 0; i < args_params_len; ++i) { - if (virStrcpyStatic(params[i].field, - args_params_val[i].field) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Parameter %s too big for destination"), - args_params_val[i].field); - goto cleanup; - } - params[i].type = args_params_val[i].value.type; - switch (params[i].type) { - case VIR_TYPED_PARAM_INT: - params[i].value.i = - args_params_val[i].value.remote_typed_param_value_u.i; - break; - case VIR_TYPED_PARAM_UINT: - params[i].value.ui = - args_params_val[i].value.remote_typed_param_value_u.ui; - break; - case VIR_TYPED_PARAM_LLONG: - params[i].value.l = - args_params_val[i].value.remote_typed_param_value_u.l; - break; - case VIR_TYPED_PARAM_ULLONG: - params[i].value.ul = - args_params_val[i].value.remote_typed_param_value_u.ul; - break; - case VIR_TYPED_PARAM_DOUBLE: - params[i].value.d = - args_params_val[i].value.remote_typed_param_value_u.d; - break; - case VIR_TYPED_PARAM_BOOLEAN: - params[i].value.b = - args_params_val[i].value.remote_typed_param_value_u.b; - break; - case VIR_TYPED_PARAM_STRING: - if (VIR_STRDUP(params[i].value.s, - args_params_val[i].value.remote_typed_param_value_u.s) < 0) - goto cleanup; - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown parameter type: %d"), - params[i].type); - goto cleanup; - } - } - - rv = 0; - - cleanup: - if (rv < 0) { - virTypedParamsFree(params, i); - params = NULL; - } - return params; -} - static int remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, @@ -5392,9 +5315,9 @@ remoteDispatchDomainMigrateBegin3Params(virNetServerPtr server ATTRIBUTE_UNUSED, if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; - if (!(params = remoteDeserializeTypedParameters(args->params.params_val, - args->params.params_len, - 0, &nparams))) + if (remoteDeserializeTypedParameters(args->params.params_val, + args->params.params_len, + 0, ¶ms, &nparams) < 0) goto cleanup; if (!(xml = virDomainMigrateBegin3Params(dom, params, nparams, @@ -5445,9 +5368,9 @@ remoteDispatchDomainMigratePrepare3Params(virNetServerPtr server ATTRIBUTE_UNUSE goto cleanup; } - if (!(params = remoteDeserializeTypedParameters(args->params.params_val, - args->params.params_len, - 0, &nparams))) + if (remoteDeserializeTypedParameters(args->params.params_val, + args->params.params_len, + 0, ¶ms, &nparams) < 0) goto cleanup; /* Wacky world of XDR ... */ @@ -5506,9 +5429,9 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server ATTRIBUTE goto cleanup; } - if (!(params = remoteDeserializeTypedParameters(args->params.params_val, - args->params.params_len, - 0, &nparams))) + if (remoteDeserializeTypedParameters(args->params.params_val, + args->params.params_len, + 0, ¶ms, &nparams) < 0) goto cleanup; if (!(st = virStreamNew(priv->conn, VIR_STREAM_NONBLOCK)) || @@ -5579,9 +5502,9 @@ remoteDispatchDomainMigratePerform3Params(virNetServerPtr server ATTRIBUTE_UNUSE if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; - if (!(params = remoteDeserializeTypedParameters(args->params.params_val, - args->params.params_len, - 0, &nparams))) + if (remoteDeserializeTypedParameters(args->params.params_val, + args->params.params_len, + 0, ¶ms, &nparams) < 0) goto cleanup; dconnuri = args->dconnuri == NULL ? NULL : *args->dconnuri; @@ -5636,9 +5559,9 @@ remoteDispatchDomainMigrateFinish3Params(virNetServerPtr server ATTRIBUTE_UNUSED goto cleanup; } - if (!(params = remoteDeserializeTypedParameters(args->params.params_val, - args->params.params_len, - 0, &nparams))) + if (remoteDeserializeTypedParameters(args->params.params_val, + args->params.params_len, + 0, ¶ms, &nparams) < 0) goto cleanup; dom = virDomainMigrateFinish3Params(priv->conn, params, nparams, @@ -5696,9 +5619,9 @@ remoteDispatchDomainMigrateConfirm3Params(virNetServerPtr server ATTRIBUTE_UNUSE if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; - if (!(params = remoteDeserializeTypedParameters(args->params.params_val, - args->params.params_len, - 0, &nparams))) + if (remoteDeserializeTypedParameters(args->params.params_val, + args->params.params_len, + 0, ¶ms, &nparams) < 0) goto cleanup; if (virDomainMigrateConfirm3Params(dom, params, nparams, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3d0ec9d..cbd71fa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2349,6 +2349,7 @@ virTypedParameterTypeFromString; virTypedParameterTypeToString; virTypedParamsCheck; virTypedParamsCopy; +virTypedParamsDeserialize; virTypedParamsFilter; virTypedParamsGetStringList; virTypedParamsReplaceString; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d9d7ec8..d712005 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -72,10 +72,12 @@ VIR_LOG_INIT("remote.remote_driver"); # define HYPER_TO_ULONG(_to, _from) (_to) = (_from) #endif -#define remoteDeserializeTypedParameters(ret_params_val, ret_params_len, \ +#define remoteDeserializeTypedParameters(remote_params_val, \ + remote_params_len, \ limit, params, nparams) \ - deserializeTypedParameters(__FUNCTION__, ret_params_val, ret_params_len, \ - limit, params, nparams) + virTypedParamsDeserialize(__FUNCTION__, \ + (virTypedParameterRemotePtr) remote_params_val, \ + remote_params_len, limit, params, nparams) static bool inside_daemon; @@ -1747,105 +1749,6 @@ remoteSerializeTypedParameters(virTypedParameterPtr params, return rv; } -/* Helper to deserialize typed parameters. */ -static int -deserializeTypedParameters(const char *funcname, - remote_typed_param *ret_params_val, - u_int ret_params_len, - int limit, - virTypedParameterPtr *params, - int *nparams) -{ - size_t i = 0; - int rv = -1; - bool userAllocated = *params != NULL; - - if (ret_params_len > limit) { - virReportError(VIR_ERR_RPC, - _("%s: too many parameters '%u' for limit '%d'"), - funcname, ret_params_len, limit); - goto cleanup; - } - - if (userAllocated) { - /* Check the length of the returned list carefully. */ - if (ret_params_len > *nparams) { - virReportError(VIR_ERR_RPC, - _("%s: too many parameters '%u' for nparams '%d'"), - funcname, ret_params_len, *nparams); - goto cleanup; - } - } else { - if (VIR_ALLOC_N(*params, ret_params_len) < 0) - goto cleanup; - } - *nparams = ret_params_len; - - /* Deserialise the result. */ - for (i = 0; i < ret_params_len; ++i) { - virTypedParameterPtr param = *params + i; - remote_typed_param *ret_param = ret_params_val + i; - - if (virStrcpyStatic(param->field, - ret_param->field) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: parameter %s too big for destination"), - funcname, ret_param->field); - goto cleanup; - } - - param->type = ret_param->value.type; - switch (param->type) { - case VIR_TYPED_PARAM_INT: - param->value.i = - ret_param->value.remote_typed_param_value_u.i; - break; - case VIR_TYPED_PARAM_UINT: - param->value.ui = - ret_param->value.remote_typed_param_value_u.ui; - break; - case VIR_TYPED_PARAM_LLONG: - param->value.l = - ret_param->value.remote_typed_param_value_u.l; - break; - case VIR_TYPED_PARAM_ULLONG: - param->value.ul = - ret_param->value.remote_typed_param_value_u.ul; - break; - case VIR_TYPED_PARAM_DOUBLE: - param->value.d = - ret_param->value.remote_typed_param_value_u.d; - break; - case VIR_TYPED_PARAM_BOOLEAN: - param->value.b = - ret_param->value.remote_typed_param_value_u.b; - break; - case VIR_TYPED_PARAM_STRING: - if (VIR_STRDUP(param->value.s, - ret_param->value.remote_typed_param_value_u.s) < 0) - goto cleanup; - break; - default: - virReportError(VIR_ERR_RPC, _("%s: unknown parameter type: %d"), - funcname, param->type); - goto cleanup; - } - } - - rv = 0; - - cleanup: - if (rv < 0) { - if (userAllocated) { - virTypedParamsClear(*params, i); - } else { - virTypedParamsFree(*params, i); - *params = NULL; - } - } - return rv; -} - static int remoteDeserializeDomainDiskErrors(remote_domain_disk_error *ret_errors_val, u_int ret_errors_len, diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 3740130..3ee4267 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -557,10 +557,11 @@ elsif ($mode eq "server") { } push(@args_list, "$1"); push(@args_list, "n$1"); - push(@getters_list, " if (($1 = remoteDeserializeTypedParameters(args->$1.$1_val,\n" . - " args->$1.$1_len,\n" . - " $2,\n" . - " &n$1)) == NULL)\n" . + push(@getters_list, " if (remoteDeserializeTypedParameters(args->$1.$1_val,\n" . + " args->$1.$1_len,\n" . + " $2,\n" . + " &$1,\n" . + " &n$1) < 0)\n" . " goto cleanup;\n"); push(@free_list, " virTypedParamsFree($1, n$1);"); } elsif ($args_member =~ m/<\S+>;/ or $args_member =~ m/\[\S+\];/) { diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index f3ce157..649d032 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1315,3 +1315,130 @@ virTypedParamsFree(virTypedParameterPtr params, virTypedParamsClear(params, nparams); VIR_FREE(params); } + + +/** + * virTypedParamsDeserialize: + * @funcname: caller's funcname + * @remote_params: protocol data to be deserialized (obtained from remote side) + * @remote_params_len: number of parameters returned in @remote_params + * @limit: user specified maximum limit to @remote_params_len + * @params: pointer which will hold the deserialized @remote_params data + * @nparams: number of entries in @params + * + * This function will attempt to deserialize protocol-encoded data obtained + * from remote side. Two modes of operation are supported, depending on the + * caller's design: + * 1) Older APIs do not rely on deserializer allocating memory for @params, + * thus calling the deserializer twice, once to find out the actual number of + * parameters for @params to hold, followed by an allocation of @params and + * a second call to the deserializer to actually retrieve the data. + * 2) Newer APIs rely completely on the deserializer to allocate the right + * ammount of memory for @params to hold all the data obtained in + * @remote_params. + * + * If used with model 1, two checks are performed, first one comparing the user + * specified limit to the actual size of remote data and the second one + * verifying the user allocated amount of memory is indeed capable of holding + * remote data @remote_params. + * With model 2, only the first check against @limit is performed. + * + * Returns 0 on success or -1 in case of an error. + */ +int +virTypedParamsDeserialize(const char *funcname, + virTypedParameterRemotePtr remote_params, + unsigned int remote_params_len, + int limit, + virTypedParameterPtr *params, + int *nparams) +{ + size_t i = 0; + int rv = -1; + bool userAllocated = *params != NULL; + + if (limit && remote_params_len > limit) { + virReportError(VIR_ERR_RPC, + _("%s: too many parameters '%u' for limit '%d'"), + funcname, remote_params_len, limit); + goto cleanup; + } + + if (userAllocated) { + /* Check the length of the returned list carefully. */ + if (remote_params_len > *nparams) { + virReportError(VIR_ERR_RPC, + _("%s: too many parameters '%u' for nparams '%d'"), + funcname, remote_params_len, *nparams); + goto cleanup; + } + } else { + if (VIR_ALLOC_N(*params, remote_params_len) < 0) + goto cleanup; + } + *nparams = remote_params_len; + + /* Deserialise the result. */ + for (i = 0; i < remote_params_len; ++i) { + virTypedParameterPtr param = *params + i; + virTypedParameterRemotePtr remote_param = remote_params + i; + + if (virStrcpyStatic(param->field, + remote_param->field) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: parameter %s too big for destination"), + funcname, remote_param->field); + goto cleanup; + } + + param->type = remote_param->value.type; + switch (param->type) { + case VIR_TYPED_PARAM_INT: + param->value.i = + remote_param->value.remote_typed_param_value.i; + break; + case VIR_TYPED_PARAM_UINT: + param->value.ui = + remote_param->value.remote_typed_param_value.ui; + break; + case VIR_TYPED_PARAM_LLONG: + param->value.l = + remote_param->value.remote_typed_param_value.l; + break; + case VIR_TYPED_PARAM_ULLONG: + param->value.ul = + remote_param->value.remote_typed_param_value.ul; + break; + case VIR_TYPED_PARAM_DOUBLE: + param->value.d = + remote_param->value.remote_typed_param_value.d; + break; + case VIR_TYPED_PARAM_BOOLEAN: + param->value.b = + remote_param->value.remote_typed_param_value.b; + break; + case VIR_TYPED_PARAM_STRING: + if (VIR_STRDUP(param->value.s, + remote_param->value.remote_typed_param_value.s) < 0) + goto cleanup; + break; + default: + virReportError(VIR_ERR_RPC, _("%s: unknown parameter type: %d"), + funcname, param->type); + goto cleanup; + } + } + + rv = 0; + + cleanup: + if (rv < 0) { + if (userAllocated) { + virTypedParamsClear(*params, i); + } else { + virTypedParamsFree(*params, i); + *params = NULL; + } + } + return rv; +} diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 7dd3a78..88b6241 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -104,6 +104,13 @@ int virTypedParamsCopy(virTypedParameterPtr *dst, char *virTypedParameterToString(virTypedParameterPtr param); +int virTypedParamsDeserialize(const char *funcname, + virTypedParameterRemotePtr remote_params, + unsigned int remote_params_len, + int limit, + virTypedParameterPtr *params, + int *nparams); + VIR_ENUM_DECL(virTypedParameter) # define VIR_TYPED_PARAMS_DEBUG(params, nparams) \ -- 2.4.3
On Tue, Feb 02, 2016 at 04:46:19PM +0100, Erik Skultety wrote:
Currently, the deserializer is hardcoded into remote_driver which makes it impossible for admin to use it. One way to achieve a shared implementation (besides moving the code to another module) would be pass @ret_params_val as a void pointer as opposed to the remote_typed_param pointer and add a new extra argument specifying which of those two protocols is being used and typecast the pointer at the function entry. An example from remote_protocol:
struct remote_typed_param_value { int type; union { int i; u_int ui; int64_t l; uint64_t ul; double d; int b; remote_nonnull_string s; } remote_typed_param_value_u; }; typedef struct remote_typed_param_value remote_typed_param_value;
struct remote_typed_param { remote_nonnull_string field; remote_typed_param_value value; };
That would leave us with a bunch of if-then-elses that needed to be used across the method. This patch takes the other approach using the new datatype introduced in one of earlier commits. --- daemon/remote.c | 125 +++++++++----------------------------------- src/libvirt_private.syms | 1 + src/remote/remote_driver.c | 107 ++------------------------------------ src/rpc/gendispatch.pl | 9 ++-- src/util/virtypedparam.c | 127 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virtypedparam.h | 7 +++ 6 files changed, 169 insertions(+), 207 deletions(-)
@@ -1747,105 +1749,6 @@ remoteSerializeTypedParameters(virTypedParameterPtr params, return rv; }
-/* Helper to deserialize typed parameters. */ -static int -deserializeTypedParameters(const char *funcname,
What is the point of the funcname parameter? Dropping it first would make it easier to unify the code paths.
+ /* Deserialise the result. */ + for (i = 0; i < remote_params_len; ++i) { + virTypedParameterPtr param = *params + i; + virTypedParameterRemotePtr remote_param = remote_params + i;
Please use (*params)[i] and remote_params[i]. Jan
-/* Helper to deserialize typed parameters. */ -static int -deserializeTypedParameters(const char *funcname,
What is the point of the funcname parameter? Dropping it first would make it easier to unify the code paths.
Alright, no problem with this one.
+ /* Deserialise the result. */ + for (i = 0; i < remote_params_len; ++i) { + virTypedParameterPtr param = *params + i; + virTypedParameterRemotePtr remote_param = remote_params + i;
Please use (*params)[i] and remote_params[i].
About this one, if you meant replacing them everywhere and also removing those 2 lines ^^above, that would definitely make it less readable. However, if you meant a replacement within those 2 lines above, that would leave you with incompatible expressions. Erik
Jan
On Wed, Feb 03, 2016 at 01:21:53PM +0100, Erik Skultety wrote:
-/* Helper to deserialize typed parameters. */ -static int -deserializeTypedParameters(const char *funcname,
What is the point of the funcname parameter? Dropping it first would make it easier to unify the code paths.
Alright, no problem with this one.
+ /* Deserialise the result. */ + for (i = 0; i < remote_params_len; ++i) { + virTypedParameterPtr param = *params + i; + virTypedParameterRemotePtr remote_param = remote_params + i;
Please use (*params)[i] and remote_params[i].
About this one, if you meant replacing them everywhere and also removing those 2 lines ^^above, that would definitely make it less readable. However, if you meant a replacement within those 2 lines above, that would leave you with incompatible expressions.
I meant the latter, I just pointed the pointers the wrong way. Jan
Since the method is static to remote_driver, it can't even be used by our daemon. Other than that, it would be useful to be able to use it with admin as well. This patch uses the new virTypedParameterRemote datatype introduced in one of previous patches. --- src/libvirt_private.syms | 1 + src/remote/remote_driver.c | 23 ++++------------------- src/util/virtypedparam.c | 29 +++++++++++++++++++++++++++++ src/util/virtypedparam.h | 3 +++ 4 files changed, 37 insertions(+), 19 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cbd71fa..5f34875 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2352,6 +2352,7 @@ virTypedParamsCopy; virTypedParamsDeserialize; virTypedParamsFilter; virTypedParamsGetStringList; +virTypedParamsRemoteFree; virTypedParamsReplaceString; virTypedParamsValidate; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d712005..2702fcb 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -72,6 +72,10 @@ VIR_LOG_INIT("remote.remote_driver"); # define HYPER_TO_ULONG(_to, _from) (_to) = (_from) #endif +#define remoteFreeTypedParameters(remote_params_val, remote_params_len) \ + virTypedParamsRemoteFree((virTypedParameterRemotePtr) remote_params_val, \ + remote_params_len) + #define remoteDeserializeTypedParameters(remote_params_val, \ remote_params_len, \ limit, params, nparams) \ @@ -1670,25 +1674,6 @@ remoteConnectListAllDomains(virConnectPtr conn, return rv; } -/* Helper to free typed parameters. */ -static void -remoteFreeTypedParameters(remote_typed_param *args_params_val, - u_int args_params_len) -{ - size_t i; - - if (args_params_val == NULL) - return; - - for (i = 0; i < args_params_len; i++) { - VIR_FREE(args_params_val[i].field); - if (args_params_val[i].value.type == VIR_TYPED_PARAM_STRING) - VIR_FREE(args_params_val[i].value.remote_typed_param_value_u.s); - } - - VIR_FREE(args_params_val); -} - /* Helper to serialize typed parameters. */ static int remoteSerializeTypedParameters(virTypedParameterPtr params, diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 649d032..3363147 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1316,6 +1316,35 @@ virTypedParamsFree(virTypedParameterPtr params, VIR_FREE(params); } +/** + * virTypedParamsRemoteFree: + * @remote_params_val: array of typed parameters as specified by + * (remote|admin)_protocol.h + * @remote_params_len: number of parameters in @remote_params_val + * + * Frees memory used by string representations of parameter identificators, + * memory used by string values of parameters and the memory occupied by + * @remote_params_val itself. + * + * Returns nothing. + */ +void +virTypedParamsRemoteFree(virTypedParameterRemotePtr remote_params_val, + unsigned int remote_params_len) +{ + size_t i; + + if (!remote_params_val) + return; + + for (i = 0; i < remote_params_len; i++) { + VIR_FREE(remote_params_val[i].field); + if (remote_params_val[i].value.type == VIR_TYPED_PARAM_STRING) + VIR_FREE(remote_params_val[i].value.remote_typed_param_value.s); + } + VIR_FREE(remote_params_val); +} + /** * virTypedParamsDeserialize: diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 88b6241..72d10c2 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -104,6 +104,9 @@ int virTypedParamsCopy(virTypedParameterPtr *dst, char *virTypedParameterToString(virTypedParameterPtr param); +void virTypedParamsRemoteFree(virTypedParameterRemotePtr remote_params_val, + unsigned int remote_params_len); + int virTypedParamsDeserialize(const char *funcname, virTypedParameterRemotePtr remote_params, unsigned int remote_params_len, -- 2.4.3
Same as for deserializer, this method might get handy for admin one day. The major reason for this patch is to stay consistent with idea, i.e. when deserializer can be shared, why not serializer as well. The only problem to be solved was that the daemon side serializer uses a code snippet which handles sparse arrays returned by some APIs as well as removes any string parameters that can't be returned to older clients. This patch makes of the new virTypedParameterRemote datatype introduced by one of the pvious patches. --- daemon/remote.c | 91 +++------------------------------------------- src/libvirt_private.syms | 1 + src/remote/remote_driver.c | 66 +++------------------------------ src/util/virtypedparam.c | 90 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virtypedparam.h | 6 +++ 5 files changed, 108 insertions(+), 146 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index f76a176..cc6474a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -108,13 +108,11 @@ static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapsho (virTypedParameterRemotePtr) remote_params_val, \ remote_params_len, limit, params, nparams) -static int -remoteSerializeTypedParameters(virTypedParameterPtr params, - int nparams, - remote_typed_param **ret_params_val, - u_int *ret_params_len, - unsigned int flags); - +#define remoteSerializeTypedParameters(params, nparams, remote_params_val, \ + remote_params_len, flags) \ + virTypedParamsSerialize(params, nparams, \ + (virTypedParameterRemotePtr *) remote_params_val, \ + remote_params_len, flags) static int remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors, int nerrors, @@ -1417,85 +1415,6 @@ remoteDispatchDomainGetSchedulerType(virNetServerPtr server ATTRIBUTE_UNUSED, return rv; } -/* Helper to serialize typed parameters. This also filters out any string - * parameters that must not be returned to older clients. */ -static int -remoteSerializeTypedParameters(virTypedParameterPtr params, - int nparams, - remote_typed_param **ret_params_val, - u_int *ret_params_len, - unsigned int flags) -{ - size_t i; - size_t j; - int rv = -1; - remote_typed_param *val; - - *ret_params_len = nparams; - if (VIR_ALLOC_N(val, nparams) < 0) - goto cleanup; - - for (i = 0, j = 0; i < nparams; ++i) { - /* virDomainGetCPUStats can return a sparse array; also, we - * can't pass back strings to older clients. */ - if (!params[i].type || - (!(flags & VIR_TYPED_PARAM_STRING_OKAY) && - params[i].type == VIR_TYPED_PARAM_STRING)) { - --*ret_params_len; - continue; - } - - /* remoteDispatchClientRequest will free this: */ - if (VIR_STRDUP(val[j].field, params[i].field) < 0) - goto cleanup; - val[j].value.type = params[i].type; - switch (params[i].type) { - case VIR_TYPED_PARAM_INT: - val[j].value.remote_typed_param_value_u.i = params[i].value.i; - break; - case VIR_TYPED_PARAM_UINT: - val[j].value.remote_typed_param_value_u.ui = params[i].value.ui; - break; - case VIR_TYPED_PARAM_LLONG: - val[j].value.remote_typed_param_value_u.l = params[i].value.l; - break; - case VIR_TYPED_PARAM_ULLONG: - val[j].value.remote_typed_param_value_u.ul = params[i].value.ul; - break; - case VIR_TYPED_PARAM_DOUBLE: - val[j].value.remote_typed_param_value_u.d = params[i].value.d; - break; - case VIR_TYPED_PARAM_BOOLEAN: - val[j].value.remote_typed_param_value_u.b = params[i].value.b; - break; - case VIR_TYPED_PARAM_STRING: - if (VIR_STRDUP(val[j].value.remote_typed_param_value_u.s, params[i].value.s) < 0) - goto cleanup; - break; - default: - virReportError(VIR_ERR_RPC, _("unknown parameter type: %d"), - params[i].type); - goto cleanup; - } - j++; - } - - *ret_params_val = val; - val = NULL; - rv = 0; - - cleanup: - if (val) { - for (i = 0; i < nparams; i++) { - VIR_FREE(val[i].field); - if (val[i].value.type == VIR_TYPED_PARAM_STRING) - VIR_FREE(val[i].value.remote_typed_param_value_u.s); - } - VIR_FREE(val); - } - return rv; -} - static int remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5f34875..a4cc14e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2354,6 +2354,7 @@ virTypedParamsFilter; virTypedParamsGetStringList; virTypedParamsRemoteFree; virTypedParamsReplaceString; +virTypedParamsSerialize; virTypedParamsValidate; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 2702fcb..bf32802 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -76,6 +76,12 @@ VIR_LOG_INIT("remote.remote_driver"); virTypedParamsRemoteFree((virTypedParameterRemotePtr) remote_params_val, \ remote_params_len) +#define remoteSerializeTypedParameters(params, nparams, remote_params_val, \ + remote_params_len) \ + virTypedParamsSerialize(params, nparams, \ + (virTypedParameterRemotePtr *) remote_params_val, \ + remote_params_len, 0) + #define remoteDeserializeTypedParameters(remote_params_val, \ remote_params_len, \ limit, params, nparams) \ @@ -1674,66 +1680,6 @@ remoteConnectListAllDomains(virConnectPtr conn, return rv; } -/* Helper to serialize typed parameters. */ -static int -remoteSerializeTypedParameters(virTypedParameterPtr params, - int nparams, - remote_typed_param **args_params_val, - u_int *args_params_len) -{ - size_t i; - int rv = -1; - remote_typed_param *val; - - *args_params_len = nparams; - if (VIR_ALLOC_N(val, nparams) < 0) - goto cleanup; - - for (i = 0; i < nparams; ++i) { - /* call() will free this: */ - if (VIR_STRDUP(val[i].field, params[i].field) < 0) - goto cleanup; - val[i].value.type = params[i].type; - switch (params[i].type) { - case VIR_TYPED_PARAM_INT: - val[i].value.remote_typed_param_value_u.i = params[i].value.i; - break; - case VIR_TYPED_PARAM_UINT: - val[i].value.remote_typed_param_value_u.ui = params[i].value.ui; - break; - case VIR_TYPED_PARAM_LLONG: - val[i].value.remote_typed_param_value_u.l = params[i].value.l; - break; - case VIR_TYPED_PARAM_ULLONG: - val[i].value.remote_typed_param_value_u.ul = params[i].value.ul; - break; - case VIR_TYPED_PARAM_DOUBLE: - val[i].value.remote_typed_param_value_u.d = params[i].value.d; - break; - 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 (VIR_STRDUP(val[i].value.remote_typed_param_value_u.s, - params[i].value.s) < 0) - goto cleanup; - break; - default: - virReportError(VIR_ERR_RPC, _("unknown parameter type: %d"), - params[i].type); - goto cleanup; - } - } - - *args_params_val = val; - val = NULL; - rv = 0; - - cleanup: - remoteFreeTypedParameters(val, nparams); - return rv; -} - static int remoteDeserializeDomainDiskErrors(remote_domain_disk_error *ret_errors_val, u_int ret_errors_len, diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 3363147..f6e4f1e 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1471,3 +1471,93 @@ virTypedParamsDeserialize(const char *funcname, } return rv; } + + +/** + * virTypedParamsSerialize: + * @params: array of parameters to be serialized and later sent to remote side + * @nparams: number of elements in @params + * @remote_params_val: protocol independent remote representation of @params + * @remote_params_len: the final number of elements in @remote_params_val + * @flags: bitwise-OR of virTypedParameterFlags + * + * This method serializes typed parameters provided by @params into + * @remote_params_val which is the representation actually being sent. + * + * Server side using this method also filters out any string parameters that + * must not be returned to older clients and handles possibly sparse arrays + * returned by some APIs. + * + * Returns 0 on success, -1 on error. + */ +int +virTypedParamsSerialize(virTypedParameterPtr params, + int nparams, + virTypedParameterRemotePtr *remote_params_val, + unsigned int *remote_params_len, + unsigned int flags) +{ + size_t i; + size_t j; + int rv = -1; + virTypedParameterRemotePtr val; + + *remote_params_len = nparams; + if (VIR_ALLOC_N(val, nparams) < 0) + goto cleanup; + + for (i = 0, j = 0; i < nparams; ++i) { + /* NOTE: Following snippet is relevant to server only, because + * virDomainGetCPUStats can return a sparse array; also, we can't pass + * back strings to older clients. */ + if (!params[i].type || + (!(flags & VIR_TYPED_PARAM_STRING_OKAY) && + params[i].type == VIR_TYPED_PARAM_STRING)) { + --*remote_params_len; + continue; + } + + /* This will be either freed by virNetServerDispatchCall or call(), + * depending on the calling side, i.e. server or client */ + if (VIR_STRDUP(val[j].field, params[i].field) < 0) + goto cleanup; + val[j].value.type = params[i].type; + switch (params[i].type) { + case VIR_TYPED_PARAM_INT: + val[j].value.remote_typed_param_value.i = params[i].value.i; + break; + case VIR_TYPED_PARAM_UINT: + val[j].value.remote_typed_param_value.ui = params[i].value.ui; + break; + case VIR_TYPED_PARAM_LLONG: + val[j].value.remote_typed_param_value.l = params[i].value.l; + break; + case VIR_TYPED_PARAM_ULLONG: + val[j].value.remote_typed_param_value.ul = params[i].value.ul; + break; + case VIR_TYPED_PARAM_DOUBLE: + val[j].value.remote_typed_param_value.d = params[i].value.d; + break; + case VIR_TYPED_PARAM_BOOLEAN: + val[j].value.remote_typed_param_value.b = params[i].value.b; + break; + case VIR_TYPED_PARAM_STRING: + if (VIR_STRDUP(val[j].value.remote_typed_param_value.s, params[i].value.s) < 0) + goto cleanup; + break; + default: + virReportError(VIR_ERR_RPC, _("unknown parameter type: %d"), + params[i].type); + goto cleanup; + } + j++; + } + + *remote_params_val = val; + val = NULL; + rv = 0; + + cleanup: + virTypedParamsRemoteFree(val, nparams); + return rv; +} diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 72d10c2..c303b50 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -114,6 +114,12 @@ int virTypedParamsDeserialize(const char *funcname, virTypedParameterPtr *params, int *nparams); +int virTypedParamsSerialize(virTypedParameterPtr params, + int nparams, + virTypedParameterRemotePtr *remote_params_val, + unsigned int *remote_params_len, + unsigned int flags); + VIR_ENUM_DECL(virTypedParameter) # define VIR_TYPED_PARAMS_DEBUG(params, nparams) \ -- 2.4.3
participants (2)
-
Erik Skultety -
Ján Tomko