[libvirt] [PATCH v3 1/5] hyperv: Functions to work with invocation parameters.
Sri Ramanujam
sramanujam at datto.com
Thu May 4 22:58:34 UTC 2017
On Mon, May 1, 2017 at 6:25 PM, Matthias Bolte
<matthias.bolte at googlemail.com> wrote:
>
> 2017-04-24 20:19 GMT+02:00 Sri Ramanujam <sramanujam at datto.com>:
> > This commit introduces functionality for creating and working with
> > invoke parameters. This commit does not include any code for serializing
> > and actually performing the method invocations; it merely defines the
> > functions and API for using invocation parameters in driver code.
> >
> > HYPERV_DEFAULT_PARAM_COUNT was chosen because almost no method
> > invocations have more than 4 parameters.
>
> > diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
> > index a3c7dc0..1960c4c 100644
> > --- a/src/hyperv/hyperv_wmi.c
> > +++ b/src/hyperv/hyperv_wmi.c
>
> > @@ -142,6 +143,269 @@ hypervVerifyResponse(WsManClient *client, WsXmlDocH response,
> > }
> >
> >
> > +/*
> > + * Methods to work with method invocation parameters
> > + */
> > +
> > +/*
> > + * hypervInitInvokeParamsList:
> > + * @priv: hypervPrivate object associated with the connection.
> > + * @method: The name of the method you are calling
> > + * @selector: The selector for the object you are invoking the method on
> > + * @obj: The WmiInfo of the object class you are invoking the method on.
> > + *
> > + * Create a new InvokeParamsList object for the method call.
> > + *
> > + * Returns a pointer to the newly instantiated object on success, which should
> > + * be freed by hypervInvokeMethod. Otherwise returns NULL.
> > + */
> > +hypervInvokeParamsListPtr
> > +hypervInitInvokeParamsList(hypervPrivate *priv, const char *method,
> > + const char *selector, hypervWmiClassInfoListPtr obj)
>
> I'd rename this to hypervCreateInvokeParamsList to follow the common
> create/free naming pattern.
>
> > +{
> > + hypervInvokeParamsListPtr params = NULL;
> > + hypervWmiClassInfoPtr info = NULL;
> > +
> > + if (hypervGetWmiClassInfo(priv, obj, &info) < 0)
> > + goto cleanup;
> > +
> > + if (VIR_ALLOC(params) < 0)
> > + goto cleanup;
> > +
> > + if (VIR_ALLOC_N(params->params,
> > + HYPERV_DEFAULT_PARAM_COUNT) < 0) {
> > + VIR_FREE(params);
> > + goto cleanup;
> > + }
> > +
> > + params->method = method;
> > + params->ns = info->rootUri;
> > + params->resourceUri = info->resourceUri;
> > + params->selector = selector;
> > + params->nbParams = 0;
> > + params->nbAvailParams = HYPERV_DEFAULT_PARAM_COUNT;
> > +
> > + cleanup:
> > + return params;
> > +}
> > +
> > +/*
> > + * hypervFreeInvokeParams:
> > + * @params: Params object to be freed
> > + *
> > + */
> > +void
> > +hypervFreeInvokeParams(hypervInvokeParamsListPtr params)
> > +{
> > + hypervParamPtr p = NULL;
> > + int i = 0;
>
> Use size_t instead of int for i.
>
> > +
> > + if (params == NULL)
> > + return;
> > +
> > + for (i = 0; i < params->nbParams; i++) {
> > + p = &(params->params[i]);
> > +
> > + switch(p->type) {
> > + case HYPERV_SIMPLE_PARAM:
> > + VIR_FREE(p->simple.name);
> > + VIR_FREE(p->simple.value);
> > + break;
> > + case HYPERV_EPR_PARAM:
> > + virBufferFreeAndReset(p->epr.query);
> > + break;
> > + case HYPERV_EMBEDDED_PARAM:
> > + virHashFree(p->embedded.table);
> > + break;
> > + }
> > + }
> > +
> > + VIR_DISPOSE_N(params->params, params->nbAvailParams);
> > + VIR_FREE(params);
> > +}
>
> > +/*
> > + * hypervAddSimpleParam:
> > + * @params: Params object to add to
> > + * @name: Name of the parameter
> > + * @value: Value of the parameter
> > + *
> > + * Add a param of type HYPERV_SIMPLE_PARAM, which is essentially a serialized
> > + * key/value pair.
> > + *
> > + * Returns -1 on failure, 0 on success.
> > + */
> > +int
> > +hypervAddSimpleParam(hypervInvokeParamsListPtr params, const char *name,
> > + const char *value)
> > +{
> > + int result = -1;
> > + hypervParamPtr p = NULL;
> > +
> > + if (hypervCheckParams(params) < 0)
> > + goto cleanup;
> > +
> > + p = ¶ms->params[params->nbParams];
> > + p->type = HYPERV_SIMPLE_PARAM;
> > +
> > + if (VIR_STRDUP(p->simple.name, name) < 0)
> > + goto cleanup;
>
> > + if (VIR_STRDUP(p->simple.value, value) < 0) {
> > + VIR_FREE(p->simple.name);
> > + goto cleanup;
> > + }
>
> This is the only one of the three param types where you make a copy of
> the name and the value. This seems unnecessary to me. Why not just
> store them directly:
>
> p->simple.name = name;
> p->simple.value = value;
>
> > + params->nbParams++;
> > +
> > + result = 0;
> > +
> > + cleanup:
> > + return result;
> > +}
> > +
> > +/*
> > + * hypervAddEprParam:
> > + * @params: Params object to add to
> > + * @name: Parameter name
> > + * @priv: hypervPrivate object associated with the connection
> > + * @query: WQL filter
> > + * @eprInfo: WmiInfo of the object being filtered
> > + *
> > + * Adds an EPR param to the params list. Returns -1 on failure, 0 on success.
> > + */
> > +int
> > +hypervAddEprParam(hypervInvokeParamsListPtr params, const char *name,
> > + hypervPrivate *priv, virBufferPtr query,
> > + hypervWmiClassInfoListPtr eprInfo)
> > +{
> > + hypervParamPtr p = NULL;
> > + hypervWmiClassInfoPtr classInfo = NULL;
> > +
> > + if (hypervGetWmiClassInfo(priv, eprInfo, &classInfo) < 0 ||
> > + hypervCheckParams(params) < 0)
> > + return -1;
> > +
> > + p = ¶ms->params[params->nbParams];
> > + p->type = HYPERV_EPR_PARAM;
> > + p->epr.name = name;
> > + p->epr.query = query;
> > + p->epr.info = classInfo;
> > + params->nbParams++;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * hypervCreateEmbeddedParam:
> > + * @priv: hypervPrivate object associated with the connection
> > + * @info: WmiInfo of the object type to serialize
> > + *
> > + * Instantiates a virHashTable pre-filled with all the properties pre-added
> > + * a key/value pairs set to NULL. The user then sets only those properties that
> > + * they wish to serialize, and passes the table via hypervAddEmbeddedParam.
> > + *
> > + * Returns a pointer to the virHashTable on success, otherwise NULL.
> > + */
> > +virHashTablePtr
> > +hypervCreateEmbeddedParam(hypervPrivate *priv, hypervWmiClassInfoListPtr info)
> > +{
> > + size_t i;
> > + int count = 0;
> > + virHashTablePtr table = NULL;
> > + XmlSerializerInfo *typeinfo = NULL;
> > + XmlSerializerInfo *item = NULL;
> > + hypervWmiClassInfoPtr classInfo = NULL;
> > +
> > + /* Get the typeinfo out of the class info list */
> > + if (hypervGetWmiClassInfo(priv, info, &classInfo) < 0)
> > + goto error;
> > +
> > + typeinfo = classInfo->serializerInfo;
> > +
> > + /* loop through the items to find out how many fields there are */
> > + for (i = 0; typeinfo[i+1].name != NULL; i++) {}
> > +
> > + count = i + 1;
>
> This code assumes that typeinfo has at least one item. Is this
> assumption correct? Can there never be a case where typeinfo has no
> items?
I believe the assumption is correct for all practical purposes. The
typeinfo structs come from the generated class definition code, so
they will always be populated with information about the properties of
each object. While it is theoretically possible that a WMI class could
not have any properties associated with it (the class would exist
entirely to have methods invoked upon it), I have thus far not found
or used a class like that in the Hyper-V API.
>
>
> > + table = virHashCreate(count, virHashValueFree);
>
> I'd call virHashCreate(count, NULL) here, so that the hash table
> doesn't take ownership of the values. In almost all cases you don't
> make copies of the param names and values. I don't see why you'd need
> copies for embedded param values.
>
> > + if (table == NULL)
> > + goto error;
> > +
> > + for (i = 0; typeinfo[i+1].name != NULL; i++) {
> > + item = &typeinfo[i];
> > +
> > + if (virHashAddEntry(table, item->name, NULL) < 0)
> > + goto error;
> > + }
> > +
> > + return table;
> > +
> > + error:
> > + virHashFree(table);
> > + return table;
> > +}
> > +
> > +int
> > +hypervSetEmbeddedProperty(virHashTablePtr table, const char *name, char *value)
> > +{
> > + char *tmp = NULL;
> > +
> > + if (VIR_STRDUP(tmp, value) < 0)
> > + return -1;
>
> Instead of creating the hash table with virHashValueFree as the
> dataFree function and feeding copied values in here. just create the
> hash table with NULL as the dataFree function and use the value here
> directly, no copy.
>
> > + return virHashUpdateEntry(table, name, tmp);
> > +}
> > +
> > +/*
> > + * hypervAddEmbeddedParam:
> > + * @params: Params list to add to
> > + * @priv: hypervPrivate object associated with the connection
> > + * @name: Name of the parameter
> > + * @table: table of properties to add
> > + * @info: WmiInfo of the object to serialize
> > + *
> > + * Add a virHashTable containing object properties as an embedded param to
> > + * an invocation list. Returns -1 on failure, 0 on success.
> > + */
> > +int
> > +hypervAddEmbeddedParam(hypervInvokeParamsListPtr params, hypervPrivate *priv,
> > + const char *name, virHashTablePtr table, hypervWmiClassInfoListPtr info)
> > +{
> > + int result = -1;
> > + hypervParamPtr p = NULL;
> > + hypervWmiClassInfoPtr classInfo = NULL;
> > +
> > + if (hypervCheckParams(params) < 0)
> > + goto cleanup;
>
> There is no actual cleanup to be done in this function, just return -1
> directly here...
>
> > +
> > + /* Get the typeinfo out of the class info list */
> > + if (hypervGetWmiClassInfo(priv, info, &classInfo) < 0)
> > + goto cleanup;
>
> ... and here.
>
> > + p = ¶ms->params[params->nbParams];
> > + p->type = HYPERV_EMBEDDED_PARAM;
> > + p->embedded.name = name;
> > + p->embedded.table = table;
> > + p->embedded.info = classInfo;
> > + params->nbParams++;
> > +
> > + result = 0;
> > +
> > + cleanup:
> > + return result;
> > +}
> > +
> > +
> >
> > /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> > * Object
> > diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h
> > index edb8efa..0a9fac2 100644
> > --- a/src/hyperv/hyperv_wmi.h
> > +++ b/src/hyperv/hyperv_wmi.h
>
> > @@ -74,6 +76,80 @@ int hypervEnumAndPull(hypervPrivate *priv, hypervWqlQueryPtr wqlQuery,
> > void hypervFreeObject(hypervPrivate *priv, hypervObject *object);
> >
> >
> > +/*
> > + * Invoke
> > + */
> > +
> > +typedef enum {
> > + HYPERV_SIMPLE_PARAM,
> > + HYPERV_EPR_PARAM,
> > + HYPERV_EMBEDDED_PARAM
> > +} hypervStorageType;
> > +
> > +struct _hypervSimpleParam {
> > + char *name;
> > + char *value;
>
> I'd switch both to const char and don't store copies here.
>
> > +};
> > +typedef struct _hypervSimpleParam hypervSimpleParam;
>
>
> --
> Matthias Bolte
> http://photron.blogspot.com
More information about the libvir-list
mailing list