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.

Functions added:
* hypervInitInvokeParamsList
* hypervFreeInvokeParams
* hypervAddSimpleParam
* hypervAddEprParam
* hypervCreateEmbeddedParam
* hypervSetEmbeddedProperty
* hypervAddEmbeddedParam
---
 src/hyperv/hyperv_wmi.c | 264 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/hyperv/hyperv_wmi.h |  78 +++++++++++++-
 2 files changed, 341 insertions(+), 1 deletion(-)

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
@@ -2,6 +2,7 @@
  * hyperv_wmi.c: general WMI over WSMAN related functions and structures for
  *               managing Microsoft Hyper-V hosts
  *
+ * Copyright (C) 2017 Datto Inc
  * Copyright (C) 2014 Red Hat, Inc.
  * Copyright (C) 2011 Matthias Bolte <matthias.bolte@googlemail.com>
  * Copyright (C) 2009 Michael Sievers <msievers83@googlemail.com>
@@ -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)
+{
+    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;
+
+    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);
+}
+
+static inline int
+hypervCheckParams(hypervInvokeParamsListPtr params)
+{
+    if (params->nbParams + 1 > params->nbAvailParams) {
+        if (VIR_EXPAND_N(params->params, params->nbAvailParams, 5) < 0)
+            return -1;
+    }
+
+    return 0;
+}
+
+/*
+ * 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 = &params->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;
+    }
+    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 = &params->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;
+    table = virHashCreate(count, virHashValueFree);
+    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;
+
+    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;
+
+    /* Get the typeinfo out of the class info list */
+    if (hypervGetWmiClassInfo(priv, info, &classInfo) < 0)
+        goto cleanup;
+
+    p = &params->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
@@ -28,11 +28,13 @@
 # include "hyperv_private.h"
 # include "hyperv_wmi_classes.h"
 # include "openwsman.h"
-
+# include "virhash.h"
 
 
 # define HYPERV_WQL_QUERY_INITIALIZER { NULL, NULL }
 
+# define HYPERV_DEFAULT_PARAM_COUNT 5
+
 int hypervVerifyResponse(WsManClient *client, WsXmlDocH response,
                          const char *detail);
 
@@ -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;
+};
+typedef struct _hypervSimpleParam hypervSimpleParam;
+
+struct _hypervEprParam {
+    const char *name;
+    virBufferPtr query;
+    hypervWmiClassInfoPtr info; // info of the object this param represents
+};
+typedef struct _hypervEprParam hypervEprParam;
+
+struct _hypervEmbeddedParam {
+    const char *name;
+    virHashTablePtr table;
+    hypervWmiClassInfoPtr info; // info of the object this param represents
+};
+typedef struct _hypervEmbeddedParam hypervEmbeddedParam;
+
+struct _hypervParam {
+    hypervStorageType type;
+    union {
+        hypervSimpleParam simple;
+        hypervEprParam epr;
+        hypervEmbeddedParam embedded;
+    };
+};
+typedef struct _hypervParam hypervParam;
+typedef hypervParam *hypervParamPtr;
+
+struct _hypervInvokeParamsList {
+    const char *method;
+    const char *ns;
+    const char *resourceUri;
+    const char *selector;
+    hypervParamPtr params;
+    size_t nbParams;
+    size_t nbAvailParams;
+};
+typedef struct _hypervInvokeParamsList hypervInvokeParamsList;
+typedef hypervInvokeParamsList *hypervInvokeParamsListPtr;
+
+
+hypervInvokeParamsListPtr hypervInitInvokeParamsList(hypervPrivate *priv,
+        const char *method, const char *selector, hypervWmiClassInfoListPtr obj);
+
+void hypervFreeInvokeParams(hypervInvokeParamsListPtr params);
+
+int hypervAddSimpleParam(hypervInvokeParamsListPtr params, const char *name,
+        const char *value);
+
+int hypervAddEprParam(hypervInvokeParamsListPtr params, const char *name,
+        hypervPrivate *priv, virBufferPtr query,
+        hypervWmiClassInfoListPtr eprInfo);
+
+virHashTablePtr hypervCreateEmbeddedParam(hypervPrivate *priv,
+        hypervWmiClassInfoListPtr info);
+
+int hypervSetEmbeddedProperty(virHashTablePtr table, const char *name,
+        char *value);
+
+int hypervAddEmbeddedParam(hypervInvokeParamsListPtr params, hypervPrivate *priv,
+        const char *name, virHashTablePtr table, hypervWmiClassInfoListPtr info);
 
 /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
  * CIM/Msvm_ReturnCode
-- 
2.9.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

2017-04-24 20:19 GMT+02:00 Sri Ramanujam <sramanujam@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 = &params->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 = &params->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?

> +    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 = &params->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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, May 1, 2017 at 6:25 PM, Matthias Bolte
<matthias.bolte@googlemail.com> wrote:
>
> 2017-04-24 20:19 GMT+02:00 Sri Ramanujam <sramanujam@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 = &params->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 = &params->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 = &params->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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list