[PATCH 0/5] hyperv: Make ownershipt transfer obvious
*** BLURB HERE *** Michal Prívozník (5): hyperv: Make it obvious that hypervInvokeMethod() consumes an argument hyperv: Reindent hypervInvokeMethod() body hyperv: Drop needless label in hypervDomainSetMemoryFlags() hyperv: Make it obvious that hypervAddEmbeddedParam() consumes an argument hyperv: Simplify @memResource freeing in hypervDomainSetMemoryFlags() src/hyperv/hyperv_driver.c | 34 ++++------ src/hyperv/hyperv_wmi.c | 129 +++++++++++++++++++++---------------- src/hyperv/hyperv_wmi.h | 12 ++-- 3 files changed, 94 insertions(+), 81 deletions(-) -- 2.26.2
Upon invocation, hypervInvokeMethod() consumes passed @params (the second argument) regardless whether success or failure is released. However, it takes only simple pointer (which hides this ownership transfer) and because of that it doesn't clear it. Switch to double pointer and tweak the documentation. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hyperv/hyperv_driver.c | 6 +++--- src/hyperv/hyperv_wmi.c | 20 ++++++++++++++------ src/hyperv/hyperv_wmi.h | 5 +++-- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 2ac30fa4c6..eeb65bd4d7 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1677,7 +1677,7 @@ hypervDomainSendKey(virDomainPtr domain, unsigned int codeset, goto cleanup; } - if (hypervInvokeMethod(priv, params, NULL) < 0) { + if (hypervInvokeMethod(priv, ¶ms, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not press key %d"), translatedKeycodes[i]); goto cleanup; @@ -1704,7 +1704,7 @@ hypervDomainSendKey(virDomainPtr domain, unsigned int codeset, goto cleanup; } - if (hypervInvokeMethod(priv, params, NULL) < 0) { + if (hypervInvokeMethod(priv, ¶ms, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not release key %s"), keycodeStr); goto cleanup; @@ -1807,7 +1807,7 @@ hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, } } - if (hypervInvokeMethod(priv, params, NULL) < 0) { + if (hypervInvokeMethod(priv, ¶ms, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not set memory")); goto cleanup; } diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 2b40e72053..78d15e2a43 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -753,17 +753,24 @@ hypervSerializeEmbeddedParam(hypervParamPtr p, const char *resourceUri, /* * hypervInvokeMethod: * @priv: hypervPrivate object associated with the connection - * @params: object containing the all necessary information for method - * invocation + * @paramsPtr: pointer to object containing the all necessary information for + * method invocation (consumed on invocation) * @res: Optional out parameter to contain the response XML. * - * Performs an invocation described by @params, and optionally returns the - * XML containing the result. Returns -1 on failure, 0 on success. + * Performs an invocation described by object at @paramsPtr, and optionally + * returns the XML containing the result. + * + * Please note that, object at @paramsPtr is consumed by this function and the + * pointer is cleared out, regardless of returning success or failure. + * + * Returns -1 on failure, 0 on success. */ int -hypervInvokeMethod(hypervPrivate *priv, hypervInvokeParamsListPtr params, - WsXmlDocH *res) +hypervInvokeMethod(hypervPrivate *priv, + hypervInvokeParamsListPtr *paramsPtr, + WsXmlDocH *res) { + hypervInvokeParamsListPtr params = *paramsPtr; int result = -1; size_t i = 0; int returnCode; @@ -939,6 +946,7 @@ hypervInvokeMethod(hypervPrivate *priv, hypervInvokeParamsListPtr params, VIR_FREE(instanceID); hypervFreeObject(priv, (hypervObject *)job); hypervFreeInvokeParams(params); + *paramsPtr = NULL; return result; } diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h index ee16657768..20749f2a39 100644 --- a/src/hyperv/hyperv_wmi.h +++ b/src/hyperv/hyperv_wmi.h @@ -154,8 +154,9 @@ int hypervAddEmbeddedParam(hypervInvokeParamsListPtr params, hypervPrivate *priv void hypervFreeEmbeddedParam(virHashTablePtr p); -int hypervInvokeMethod(hypervPrivate *priv, hypervInvokeParamsListPtr params, - WsXmlDocH *res); +int hypervInvokeMethod(hypervPrivate *priv, + hypervInvokeParamsListPtr *paramsPtr, + WsXmlDocH *res); /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * CIM/Msvm_ReturnCode -- 2.26.2
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hyperv/hyperv_wmi.c | 92 ++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 78d15e2a43..779f4265b5 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -791,14 +791,14 @@ hypervInvokeMethod(hypervPrivate *priv, if (hypervCreateInvokeXmlDoc(params, ¶msDocRoot) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Could not create XML document")); + _("Could not create XML document")); goto cleanup; } methodNode = xml_parser_get_root(paramsDocRoot); if (!methodNode) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Could not get root of XML document")); + _("Could not get root of XML document")); goto cleanup; } @@ -807,25 +807,25 @@ hypervInvokeMethod(hypervPrivate *priv, p = &(params->params[i]); switch (p->type) { - case HYPERV_SIMPLE_PARAM: - if (hypervSerializeSimpleParam(p, params->resourceUri, - &methodNode) < 0) - goto cleanup; - break; - case HYPERV_EPR_PARAM: - if (hypervSerializeEprParam(p, priv, params->resourceUri, - &methodNode) < 0) - goto cleanup; - break; - case HYPERV_EMBEDDED_PARAM: - if (hypervSerializeEmbeddedParam(p, params->resourceUri, - &methodNode) < 0) - goto cleanup; - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unknown parameter type")); + case HYPERV_SIMPLE_PARAM: + if (hypervSerializeSimpleParam(p, params->resourceUri, + &methodNode) < 0) goto cleanup; + break; + case HYPERV_EPR_PARAM: + if (hypervSerializeEprParam(p, priv, params->resourceUri, + &methodNode) < 0) + goto cleanup; + break; + case HYPERV_EMBEDDED_PARAM: + if (hypervSerializeEmbeddedParam(p, params->resourceUri, + &methodNode) < 0) + goto cleanup; + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unknown parameter type")); + goto cleanup; } } @@ -840,7 +840,7 @@ hypervInvokeMethod(hypervPrivate *priv, /* do the invoke */ response = wsmc_action_invoke(priv->client, params->resourceUri, options, - params->method, paramsDocRoot); + params->method, paramsDocRoot); /* check return code of invocation */ returnValue_xpath = g_strdup_printf("/s:Envelope/s:Body/p:%s_OUTPUT/p:ReturnValue", @@ -888,43 +888,43 @@ hypervInvokeMethod(hypervPrivate *priv, virBufferEscapeSQL(&query, "where InstanceID = \"%s\"", instanceID); if (hypervGetWmiClassList(priv, Msvm_ConcreteJob_WmiInfo, &query, - (hypervObject **)&job) < 0 || job == NULL) + (hypervObject **)&job) < 0 || job == NULL) goto cleanup; jobState = job->data.common->JobState; switch (jobState) { - case MSVM_CONCRETEJOB_JOBSTATE_NEW: - case MSVM_CONCRETEJOB_JOBSTATE_STARTING: - case MSVM_CONCRETEJOB_JOBSTATE_RUNNING: - case MSVM_CONCRETEJOB_JOBSTATE_SHUTTING_DOWN: - hypervFreeObject(priv, (hypervObject *)job); - job = NULL; - g_usleep(100 * 1000); /* sleep 100 ms */ - timeout -= 100; - continue; - case MSVM_CONCRETEJOB_JOBSTATE_COMPLETED: - completed = true; - break; - case MSVM_CONCRETEJOB_JOBSTATE_TERMINATED: - case MSVM_CONCRETEJOB_JOBSTATE_KILLED: - case MSVM_CONCRETEJOB_JOBSTATE_EXCEPTION: - case MSVM_CONCRETEJOB_JOBSTATE_SERVICE: - goto cleanup; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unknown invocation state")); - goto cleanup; + case MSVM_CONCRETEJOB_JOBSTATE_NEW: + case MSVM_CONCRETEJOB_JOBSTATE_STARTING: + case MSVM_CONCRETEJOB_JOBSTATE_RUNNING: + case MSVM_CONCRETEJOB_JOBSTATE_SHUTTING_DOWN: + hypervFreeObject(priv, (hypervObject *)job); + job = NULL; + g_usleep(100 * 1000); /* sleep 100 ms */ + timeout -= 100; + continue; + case MSVM_CONCRETEJOB_JOBSTATE_COMPLETED: + completed = true; + break; + case MSVM_CONCRETEJOB_JOBSTATE_TERMINATED: + case MSVM_CONCRETEJOB_JOBSTATE_KILLED: + case MSVM_CONCRETEJOB_JOBSTATE_EXCEPTION: + case MSVM_CONCRETEJOB_JOBSTATE_SERVICE: + goto cleanup; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unknown invocation state")); + goto cleanup; } } if (!completed && timeout < 0) { virReportError(VIR_ERR_OPERATION_TIMEOUT, - _("Timeout waiting for %s invocation"), params->method); + _("Timeout waiting for %s invocation"), params->method); goto cleanup; } } else if (returnCode != CIM_RETURNCODE_COMPLETED_WITH_NO_ERROR) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invocation of %s returned an error: %s (%d)"), - params->method, hypervReturnCodeToString(returnCode), - returnCode); + params->method, hypervReturnCodeToString(returnCode), + returnCode); goto cleanup; } -- 2.26.2
Now, that hypervInvokeMethod() clears the passed pointer we don't need a special cleanup label ('params_cleanup') that handles non-obvious ownership transfer. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hyperv/hyperv_driver.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index eeb65bd4d7..dd08e573c0 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1765,7 +1765,7 @@ hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, if (hypervAddEprParam(params, "ComputerSystem", priv, &eprQuery, Msvm_ComputerSystem_WmiInfo) < 0) - goto params_cleanup; + goto cleanup; } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) { params = hypervCreateInvokeParamsList(priv, "ModifyResourceSettings", MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR, @@ -1779,31 +1779,31 @@ hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, memResource = hypervCreateEmbeddedParam(priv, Msvm_MemorySettingData_WmiInfo); if (!memResource) - goto params_cleanup; + goto cleanup; if (hypervSetEmbeddedProperty(memResource, "VirtualQuantity", memory_str) < 0) { hypervFreeEmbeddedParam(memResource); - goto params_cleanup; + goto cleanup; } if (hypervSetEmbeddedProperty(memResource, "InstanceID", memsd->data.common->InstanceID) < 0) { hypervFreeEmbeddedParam(memResource); - goto params_cleanup; + goto cleanup; } if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { if (hypervAddEmbeddedParam(params, priv, "ResourceSettingData", memResource, Msvm_MemorySettingData_WmiInfo) < 0) { hypervFreeEmbeddedParam(memResource); - goto params_cleanup; + goto cleanup; } } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) { if (hypervAddEmbeddedParam(params, priv, "ResourceSettings", memResource, Msvm_MemorySettingData_WmiInfo) < 0) { hypervFreeEmbeddedParam(memResource); - goto params_cleanup; + goto cleanup; } } @@ -1813,14 +1813,12 @@ hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, } result = 0; - goto cleanup; - params_cleanup: - hypervFreeInvokeParams(params); cleanup: VIR_FREE(memory_str); hypervFreeObject(priv, (hypervObject *)vssd); hypervFreeObject(priv, (hypervObject *)memsd); + hypervFreeInvokeParams(params); return result; } -- 2.26.2
Upon successful return hypervAddEmbeddedParam() transfers ownership of @table argument to @params. But because it takes only simple pointer (which hides this ownership transfer) it doesn't clear the @table pointer. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hyperv/hyperv_driver.c | 4 ++-- src/hyperv/hyperv_wmi.c | 17 ++++++++++++----- src/hyperv/hyperv_wmi.h | 7 +++++-- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index dd08e573c0..cae284db0b 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1794,14 +1794,14 @@ hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { if (hypervAddEmbeddedParam(params, priv, "ResourceSettingData", - memResource, Msvm_MemorySettingData_WmiInfo) < 0) { + &memResource, Msvm_MemorySettingData_WmiInfo) < 0) { hypervFreeEmbeddedParam(memResource); goto cleanup; } } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) { if (hypervAddEmbeddedParam(params, priv, "ResourceSettings", - memResource, Msvm_MemorySettingData_WmiInfo) < 0) { + &memResource, Msvm_MemorySettingData_WmiInfo) < 0) { hypervFreeEmbeddedParam(memResource); goto cleanup; } diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 779f4265b5..41579858de 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -362,15 +362,22 @@ hypervSetEmbeddedProperty(virHashTablePtr table, const char *name, char *value) * @params: Params list to add to * @priv: hypervPrivate object associated with the connection * @name: Name of the parameter - * @table: table of properties to add + * @table: pointer to 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. + * an invocation list. + * + * Upon successfull return the @table is consumed and the pointer is cleared out. + * + * Returns -1 on failure, 0 on success. */ int -hypervAddEmbeddedParam(hypervInvokeParamsListPtr params, hypervPrivate *priv, - const char *name, virHashTablePtr table, hypervWmiClassInfoListPtr info) +hypervAddEmbeddedParam(hypervInvokeParamsListPtr params, + hypervPrivate *priv, + const char *name, + virHashTablePtr *table, + hypervWmiClassInfoListPtr info) { hypervParamPtr p = NULL; hypervWmiClassInfoPtr classInfo = NULL; @@ -385,7 +392,7 @@ hypervAddEmbeddedParam(hypervInvokeParamsListPtr params, hypervPrivate *priv, p = ¶ms->params[params->nbParams]; p->type = HYPERV_EMBEDDED_PARAM; p->embedded.name = name; - p->embedded.table = table; + p->embedded.table = g_steal_pointer(table); p->embedded.info = classInfo; params->nbParams++; diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h index 20749f2a39..fb19a7f375 100644 --- a/src/hyperv/hyperv_wmi.h +++ b/src/hyperv/hyperv_wmi.h @@ -149,8 +149,11 @@ virHashTablePtr hypervCreateEmbeddedParam(hypervPrivate *priv, int hypervSetEmbeddedProperty(virHashTablePtr table, const char *name, char *value); -int hypervAddEmbeddedParam(hypervInvokeParamsListPtr params, hypervPrivate *priv, - const char *name, virHashTablePtr table, hypervWmiClassInfoListPtr info); +int hypervAddEmbeddedParam(hypervInvokeParamsListPtr params, + hypervPrivate *priv, + const char *name, + virHashTablePtr *table, + hypervWmiClassInfoListPtr info); void hypervFreeEmbeddedParam(virHashTablePtr p); -- 2.26.2
Now, that ownership transfer of hypervSetEmbeddedProperty() is clear, we can use automatic freeing of the hash table. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hyperv/hyperv_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index cae284db0b..9ec2b879fc 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1735,7 +1735,7 @@ hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, Msvm_VirtualSystemSettingData *vssd = NULL; Msvm_MemorySettingData *memsd = NULL; g_auto(virBuffer) eprQuery = VIR_BUFFER_INITIALIZER; - virHashTablePtr memResource = NULL; + g_autoptr(virHashTable) memResource = NULL; virCheckFlags(0, -1); @@ -1781,21 +1781,17 @@ hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, if (!memResource) goto cleanup; - if (hypervSetEmbeddedProperty(memResource, "VirtualQuantity", memory_str) < 0) { - hypervFreeEmbeddedParam(memResource); + if (hypervSetEmbeddedProperty(memResource, "VirtualQuantity", memory_str) < 0) goto cleanup; - } if (hypervSetEmbeddedProperty(memResource, "InstanceID", memsd->data.common->InstanceID) < 0) { - hypervFreeEmbeddedParam(memResource); goto cleanup; } if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { if (hypervAddEmbeddedParam(params, priv, "ResourceSettingData", &memResource, Msvm_MemorySettingData_WmiInfo) < 0) { - hypervFreeEmbeddedParam(memResource); goto cleanup; } -- 2.26.2
On Oct 15, 2020, at 10:00 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
*** BLURB HERE ***
Michal Prívozník (5): hyperv: Make it obvious that hypervInvokeMethod() consumes an argument hyperv: Reindent hypervInvokeMethod() body hyperv: Drop needless label in hypervDomainSetMemoryFlags() hyperv: Make it obvious that hypervAddEmbeddedParam() consumes an argument hyperv: Simplify @memResource freeing in hypervDomainSetMemoryFlags()
src/hyperv/hyperv_driver.c | 34 ++++------ src/hyperv/hyperv_wmi.c | 129 +++++++++++++++++++++---------------- src/hyperv/hyperv_wmi.h | 12 ++-- 3 files changed, 94 insertions(+), 81 deletions(-)
This whole patchset looks good. Reviewed-by: Matt Coleman <matt@datto.com> -- Matt
On 10/17/20 2:54 AM, Matt Coleman wrote:
On Oct 15, 2020, at 10:00 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
*** BLURB HERE ***
Michal Prívozník (5): hyperv: Make it obvious that hypervInvokeMethod() consumes an argument hyperv: Reindent hypervInvokeMethod() body hyperv: Drop needless label in hypervDomainSetMemoryFlags() hyperv: Make it obvious that hypervAddEmbeddedParam() consumes an argument hyperv: Simplify @memResource freeing in hypervDomainSetMemoryFlags()
src/hyperv/hyperv_driver.c | 34 ++++------ src/hyperv/hyperv_wmi.c | 129 +++++++++++++++++++++---------------- src/hyperv/hyperv_wmi.h | 12 ++-- 3 files changed, 94 insertions(+), 81 deletions(-)
This whole patchset looks good.
Reviewed-by: Matt Coleman <matt@datto.com>
Thanks pushed. Michal
participants (3)
-
Matt Coleman -
Michal Privoznik -
Michal Prívozník