[PATCH 0/4] hyperv: Cleanup code create embedded properties
*** BLURB HERE *** Michal Prívozník (4): hyperv: Accept const @value in hypervSetEmbeddedProperty() hyperv: Use g_auto() for virHashTable in hypervCreateEmbeddedParam hyperv: Drop needles error label in hypervCreateEmbeddedParam() hyverv: hypervCreateEmbeddedParam: Rework items counting src/hyperv/hyperv_wmi.c | 43 ++++++++++++++++++++++++++--------------- src/hyperv/hyperv_wmi.h | 5 +++-- 2 files changed, 30 insertions(+), 18 deletions(-) -- 2.26.2
The hypervSetEmbeddedProperty() function is used to update a value for given property in a list of properties created by hypervCreateEmbeddedParam(). The list is nothing fancy - it's a virHashTable that has NULL as dataFree callback => the table does not own the value. This is not that obvious since hypervSetEmbeddedProperty() accepts a non-const pointer. This fact makes it unnecessary hard to consume, e.g. if we wanted to pass a stack allocated string. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hyperv/hyperv_wmi.c | 20 ++++++++++++++++++-- src/hyperv/hyperv_wmi.h | 5 +++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 41579858de..742a46bc28 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -351,10 +351,26 @@ hypervCreateEmbeddedParam(hypervPrivate *priv, hypervWmiClassInfoListPtr info) return NULL; } + +/** + * hypervSetEmbeddedProperty: + * @table: hash table allocated earlier by hypervCreateEmbeddedParam() + * @name: name of the property + * @value: value of the property + * + * For given table of properties, set property of @name to @value. + * Please note, that the hash table does NOT become owner of the @value and + * thus caller must ensure the pointer validity. + * + * Returns: 0 on success, + * -1 otherwise. + */ int -hypervSetEmbeddedProperty(virHashTablePtr table, const char *name, char *value) +hypervSetEmbeddedProperty(virHashTablePtr table, + const char *name, + const char *value) { - return virHashUpdateEntry(table, name, value); + return virHashUpdateEntry(table, name, (void*) value); } /* diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h index fb19a7f375..fa8e48a70e 100644 --- a/src/hyperv/hyperv_wmi.h +++ b/src/hyperv/hyperv_wmi.h @@ -146,8 +146,9 @@ int hypervAddEprParam(hypervInvokeParamsListPtr params, const char *name, virHashTablePtr hypervCreateEmbeddedParam(hypervPrivate *priv, hypervWmiClassInfoListPtr info); -int hypervSetEmbeddedProperty(virHashTablePtr table, const char *name, - char *value); +int hypervSetEmbeddedProperty(virHashTablePtr table, + const char *name, + const char *value); int hypervAddEmbeddedParam(hypervInvokeParamsListPtr params, hypervPrivate *priv, -- 2.26.2
This will allow us to drop 'error' label later. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hyperv/hyperv_wmi.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 742a46bc28..2faa2f396f 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -318,7 +318,7 @@ hypervCreateEmbeddedParam(hypervPrivate *priv, hypervWmiClassInfoListPtr info) { size_t i; int count = 0; - virHashTablePtr table = NULL; + g_autoptr(virHashTable) table = NULL; XmlSerializerInfo *typeinfo = NULL; XmlSerializerInfo *item = NULL; hypervWmiClassInfoPtr classInfo = NULL; @@ -344,10 +344,9 @@ hypervCreateEmbeddedParam(hypervPrivate *priv, hypervWmiClassInfoListPtr info) goto error; } - return table; + return g_steal_pointer(&table); error: - virHashFree(table); return NULL; } -- 2.26.2
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hyperv/hyperv_wmi.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 2faa2f396f..489bb03a9b 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -325,7 +325,7 @@ hypervCreateEmbeddedParam(hypervPrivate *priv, hypervWmiClassInfoListPtr info) /* Get the typeinfo out of the class info list */ if (hypervGetWmiClassInfo(priv, info, &classInfo) < 0) - goto error; + return NULL; typeinfo = classInfo->serializerInfo; @@ -335,19 +335,16 @@ hypervCreateEmbeddedParam(hypervPrivate *priv, hypervWmiClassInfoListPtr info) table = virHashCreate(count, NULL); if (table == NULL) - goto error; + return NULL; for (i = 0; typeinfo[i].name != NULL; i++) { item = &typeinfo[i]; if (virHashAddEntry(table, item->name, NULL) < 0) - goto error; + return NULL; } return g_steal_pointer(&table); - - error: - return NULL; } -- 2.26.2
It's not necessarily clear, why we need to create the hash table as big as number of fields we want to store, but nevertheless, the code can be written a bit better. The @count should be type of size_t and could be used directly in the loop that counts the fields. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hyperv/hyperv_wmi.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 489bb03a9b..8bebf02a50 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -317,10 +317,9 @@ virHashTablePtr hypervCreateEmbeddedParam(hypervPrivate *priv, hypervWmiClassInfoListPtr info) { size_t i; - int count = 0; + size_t count; g_autoptr(virHashTable) table = NULL; XmlSerializerInfo *typeinfo = NULL; - XmlSerializerInfo *item = NULL; hypervWmiClassInfoPtr classInfo = NULL; /* Get the typeinfo out of the class info list */ @@ -330,15 +329,15 @@ hypervCreateEmbeddedParam(hypervPrivate *priv, hypervWmiClassInfoListPtr info) typeinfo = classInfo->serializerInfo; /* loop through the items to find out how many fields there are */ - for (i = 0; typeinfo[i].name != NULL; i++) {} - count = i; + for (count = 0; typeinfo[count].name != NULL; count++) + ; table = virHashCreate(count, NULL); if (table == NULL) return NULL; for (i = 0; typeinfo[i].name != NULL; i++) { - item = &typeinfo[i]; + XmlSerializerInfo *item = &typeinfo[i]; if (virHashAddEntry(table, item->name, NULL) < 0) return NULL; -- 2.26.2
On Oct 19, 2020, at 7:35 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
*** BLURB HERE ***
Michal Prívozník (4): hyperv: Accept const @value in hypervSetEmbeddedProperty() hyperv: Use g_auto() for virHashTable in hypervCreateEmbeddedParam hyperv: Drop needles error label in hypervCreateEmbeddedParam() hyverv: hypervCreateEmbeddedParam: Rework items counting
src/hyperv/hyperv_wmi.c | 43 ++++++++++++++++++++++++++--------------- src/hyperv/hyperv_wmi.h | 5 +++-- 2 files changed, 30 insertions(+), 18 deletions(-)
Reviewed and tested locally. This will be much easier to work with. Thanks! Just one note: there’s a typo in patch 3’s description: "needles" should be "needless". -- Matt
participants (2)
-
Matt Coleman -
Michal Privoznik