[PATCH 0/7] qemu_migration_params: Simplify code by using automatic memory handling
I've cleaned up the migration params code before actually going to add new features. Peter Krempa (7): qemuMigrationParamsNew: Use new memory allocation to simplify code qemuMigrationParamsFromFlags: Use 'g_autoptr' to remove 'error:' label qemuMigrationParamsFromJSON: Unify return value handling with other functions qemuMigrationParamsToJSON: Refactor variable cleanup qemuMigrationCapsToJSON: Refactor variable cleanup qemuMigrationParamsParse: Refactor variable cleanup qemuMigrationCapsCheck: Refactor variable cleanup src/qemu/qemu_migration_params.c | 158 +++++++++++-------------------- 1 file changed, 56 insertions(+), 102 deletions(-) -- 2.26.2
Use automatic memory cleaning and allocate via g_new0. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 04434e9557..f466c3c4f6 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -244,20 +244,14 @@ qemuMigrationParamsGetAlwaysOnCaps(qemuMigrationParty party) qemuMigrationParamsPtr qemuMigrationParamsNew(void) { - qemuMigrationParamsPtr params; + g_autoptr(qemuMigrationParams) params = NULL; - if (VIR_ALLOC(params) < 0) - return NULL; - - params->caps = virBitmapNew(QEMU_MIGRATION_CAP_LAST); - if (!params->caps) - goto error; + params = g_new0(qemuMigrationParams, 1); - return params; + if (!(params->caps = virBitmapNew(QEMU_MIGRATION_CAP_LAST))) + return NULL; - error: - qemuMigrationParamsFree(params); - return NULL; + return g_steal_pointer(¶ms); } -- 2.26.2
On a Monday in 2020, Peter Krempa wrote:
Use automatic memory cleaning and allocate via g_new0.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index f466c3c4f6..03d8d4fb49 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -534,7 +534,7 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params, unsigned long flags, qemuMigrationParty party) { - qemuMigrationParamsPtr migParams; + g_autoptr(qemuMigrationParams) migParams = NULL; size_t i; if (!(migParams = qemuMigrationParamsNew())) @@ -565,14 +565,14 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params, if (qemuMigrationParamsGetTPInt(migParams, item->param, params, nparams, item->typedParam, item->unit) < 0) - goto error; + return NULL; break; case QEMU_MIGRATION_PARAM_TYPE_ULL: if (qemuMigrationParamsGetTPULL(migParams, item->param, params, nparams, item->typedParam, item->unit) < 0) - goto error; + return NULL; break; case QEMU_MIGRATION_PARAM_TYPE_BOOL: @@ -581,7 +581,7 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params, case QEMU_MIGRATION_PARAM_TYPE_STRING: if (qemuMigrationParamsGetTPString(migParams, item->param, params, nparams, item->typedParam) < 0) - goto error; + return NULL; break; } } @@ -591,24 +591,20 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params, !(flags & VIR_MIGRATE_AUTO_CONVERGE)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Turn auto convergence on to tune it")); - goto error; + return NULL; } if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].set && !(flags & VIR_MIGRATE_PARALLEL)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Turn parallel migration on to tune it")); - goto error; + return NULL; } if (qemuMigrationParamsSetCompression(params, nparams, flags, migParams) < 0) - goto error; - - return migParams; + return NULL; - error: - qemuMigrationParamsFree(migParams); - return NULL; + return g_steal_pointer(&migParams); } -- 2.26.2
On a Monday in 2020, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This function doesn't have an overly verbose cleanup section as there isn't any error code path. Unify it with the rest of the functions which will simplify adding a possible error path. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 03d8d4fb49..3ed55c2ab4 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -670,7 +670,7 @@ qemuMigrationParamsDump(qemuMigrationParamsPtr migParams, qemuMigrationParamsPtr qemuMigrationParamsFromJSON(virJSONValuePtr params) { - qemuMigrationParamsPtr migParams; + g_autoptr(qemuMigrationParams) migParams = NULL; qemuMigrationParamValuePtr pv; const char *name; const char *str; @@ -680,7 +680,7 @@ qemuMigrationParamsFromJSON(virJSONValuePtr params) return NULL; if (!params) - return migParams; + return g_steal_pointer(&migParams); for (i = 0; i < QEMU_MIGRATION_PARAM_LAST; i++) { name = qemuMigrationParamTypeToString(i); @@ -711,7 +711,7 @@ qemuMigrationParamsFromJSON(virJSONValuePtr params) } } - return migParams; + return g_steal_pointer(&migParams); } -- 2.26.2
On a Monday in 2020, Peter Krempa wrote:
This function doesn't have an overly verbose cleanup section as there isn't any error code path. Unify it with the rest of the functions which will simplify adding a possible error path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use automatic memory allocation and move variables into correct scope to simplify the code and remove the need for a 'error:' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 3ed55c2ab4..c22362735a 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -718,20 +718,17 @@ qemuMigrationParamsFromJSON(virJSONValuePtr params) virJSONValuePtr qemuMigrationParamsToJSON(qemuMigrationParamsPtr migParams) { - virJSONValuePtr params = virJSONValueNewObject(); - qemuMigrationParamValuePtr pv; - const char *name; + g_autoptr(virJSONValue) params = virJSONValueNewObject(); size_t i; - int rc; for (i = 0; i < QEMU_MIGRATION_PARAM_LAST; i++) { - name = qemuMigrationParamTypeToString(i); - pv = &migParams->params[i]; + const char *name = qemuMigrationParamTypeToString(i); + qemuMigrationParamValuePtr pv = &migParams->params[i]; + int rc = 0; if (!pv->set) continue; - rc = 0; switch (qemuMigrationParamTypes[i]) { case QEMU_MIGRATION_PARAM_TYPE_INT: rc = virJSONValueObjectAppendNumberInt(params, name, pv->value.i); @@ -751,14 +748,10 @@ qemuMigrationParamsToJSON(qemuMigrationParamsPtr migParams) } if (rc < 0) - goto error; + return NULL; } - return params; - - error: - virJSONValueFree(params); - return NULL; + return g_steal_pointer(¶ms); } -- 2.26.2
On a Monday in 2020, Peter Krempa wrote:
Use automatic memory allocation and move variables into correct scope to simplify the code and remove the need for a 'error:' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use automatic memory allocation and move variables into correct scope to simplify the code and remove the need for a 'error:' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index c22362735a..9b6601367a 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -759,14 +759,12 @@ virJSONValuePtr qemuMigrationCapsToJSON(virBitmapPtr caps, virBitmapPtr states) { - virJSONValuePtr json = NULL; - virJSONValuePtr cap = NULL; + g_autoptr(virJSONValue) json = virJSONValueNewArray(); qemuMigrationCapability bit; - const char *name; - - json = virJSONValueNewArray(); for (bit = 0; bit < QEMU_MIGRATION_CAP_LAST; bit++) { + g_autoptr(virJSONValue) cap = virJSONValueNewObject(); + const char *name = qemuMigrationCapabilityTypeToString(bit); bool supported = false; bool state = false; @@ -776,27 +774,19 @@ qemuMigrationCapsToJSON(virBitmapPtr caps, ignore_value(virBitmapGetBit(states, bit, &state)); - cap = virJSONValueNewObject(); - - name = qemuMigrationCapabilityTypeToString(bit); if (virJSONValueObjectAppendString(cap, "capability", name) < 0) - goto error; + return NULL; if (virJSONValueObjectAppendBoolean(cap, "state", state) < 0) - goto error; + return NULL; if (virJSONValueArrayAppend(json, cap) < 0) - goto error; + return NULL; cap = NULL; } - return json; - - error: - virJSONValueFree(json); - virJSONValueFree(cap); - return NULL; + return g_steal_pointer(&json); } -- 2.26.2
On a Monday in 2020, Peter Krempa wrote:
Use automatic memory allocation and move variables into correct scope to simplify the code and remove the need for a 'error:' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index c22362735a..9b6601367a 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -759,14 +759,12 @@ virJSONValuePtr qemuMigrationCapsToJSON(virBitmapPtr caps, virBitmapPtr states) { - virJSONValuePtr json = NULL; - virJSONValuePtr cap = NULL; + g_autoptr(virJSONValue) json = virJSONValueNewArray(); qemuMigrationCapability bit; - const char *name; - - json = virJSONValueNewArray();
for (bit = 0; bit < QEMU_MIGRATION_CAP_LAST; bit++) { + g_autoptr(virJSONValue) cap = virJSONValueNewObject(); + const char *name = qemuMigrationCapabilityTypeToString(bit);
I don't like allocating the JSON object and converting the string if we might 'continue' without using it, but I doubt it will have a measurable effect.
bool supported = false; bool state = false;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Mon, Aug 24, 2020 at 15:27:40 +0200, Ján Tomko wrote:
On a Monday in 2020, Peter Krempa wrote:
Use automatic memory allocation and move variables into correct scope to simplify the code and remove the need for a 'error:' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index c22362735a..9b6601367a 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -759,14 +759,12 @@ virJSONValuePtr qemuMigrationCapsToJSON(virBitmapPtr caps, virBitmapPtr states) { - virJSONValuePtr json = NULL; - virJSONValuePtr cap = NULL; + g_autoptr(virJSONValue) json = virJSONValueNewArray(); qemuMigrationCapability bit; - const char *name; - - json = virJSONValueNewArray();
for (bit = 0; bit < QEMU_MIGRATION_CAP_LAST; bit++) { + g_autoptr(virJSONValue) cap = virJSONValueNewObject(); + const char *name = qemuMigrationCapabilityTypeToString(bit);
I don't like allocating the JSON object and converting the string if we might 'continue' without using it, but I doubt it will have a measurable effect.
I have an idea how to fix it, but it will not go well under this patch. I'll follow up with another one changing the code a bit.
Use automatic memory allocation and move variables into correct scope to simplify the code and remove the need for a 'cleanup:' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 40 ++++++++++++-------------------- 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 9b6601367a..38a5a91f2a 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -1265,44 +1265,42 @@ int qemuMigrationParamsParse(xmlXPathContextPtr ctxt, qemuMigrationParamsPtr *migParams) { - qemuMigrationParamsPtr params = NULL; + g_autoptr(qemuMigrationParams) params = NULL; qemuMigrationParamValuePtr pv; - xmlNodePtr *nodes = NULL; - char *name = NULL; - char *value = NULL; - int param; + g_autofree xmlNodePtr *nodes = NULL; size_t i; int rc; int n; - int ret = -1; *migParams = NULL; if ((rc = virXPathBoolean("boolean(./migParams)", ctxt)) < 0) - goto cleanup; + return -1; - if (rc == 0) { - ret = 0; - goto cleanup; - } + if (rc == 0) + return 0; if ((n = virXPathNodeSet("./migParams[1]/param", ctxt, &nodes)) < 0) return -1; if (!(params = qemuMigrationParamsNew())) - goto cleanup; + return -1; for (i = 0; i < n; i++) { + g_autofree char *name = NULL; + g_autofree char *value = NULL; + int param; + if (!(name = virXMLPropString(nodes[i], "name"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing migration parameter name")); - goto cleanup; + return -1; } if ((param = qemuMigrationParamTypeFromString(name)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown migration parameter '%s'"), name); - goto cleanup; + return -1; } pv = ¶ms->params[param]; @@ -1310,7 +1308,7 @@ qemuMigrationParamsParse(xmlXPathContextPtr ctxt, virReportError(VIR_ERR_INTERNAL_ERROR, _("missing value for migration parameter '%s'"), name); - goto cleanup; + return -1; } rc = 0; @@ -1336,23 +1334,15 @@ qemuMigrationParamsParse(xmlXPathContextPtr ctxt, virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid value '%s' for migration parameter '%s'"), value, name); - goto cleanup; + return -1; } pv->set = true; - VIR_FREE(name); - VIR_FREE(value); } *migParams = g_steal_pointer(¶ms); - ret = 0; - cleanup: - qemuMigrationParamsFree(params); - VIR_FREE(nodes); - VIR_FREE(name); - VIR_FREE(value); - return ret; + return 0; } -- 2.26.2
On a Monday in 2020, Peter Krempa wrote:
Use automatic memory allocation and move variables into correct scope to simplify the code and remove the need for a 'cleanup:' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 40 ++++++++++++-------------------- 1 file changed, 15 insertions(+), 25 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use automatic memory allocation to simplify the code and remove the need for a 'cleanup:' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 33 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 38a5a91f2a..c785f87ab8 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -1352,11 +1352,10 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver, int asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; - virBitmapPtr migEvent = NULL; - virJSONValuePtr json = NULL; - char **caps = NULL; + g_autoptr(virBitmap) migEvent = NULL; + g_autoptr(virJSONValue) json = NULL; + VIR_AUTOSTRINGLIST caps = NULL; char **capStr; - int ret = -1; int rc; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) @@ -1365,16 +1364,14 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver, rc = qemuMonitorGetMigrationCapabilities(priv->mon, &caps); if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) - goto cleanup; + return -1; - if (!caps) { - ret = 0; - goto cleanup; - } + if (!caps) + return 0; priv->migrationCaps = virBitmapNew(QEMU_MIGRATION_CAP_LAST); if (!priv->migrationCaps) - goto cleanup; + return -1; for (capStr = caps; *capStr; capStr++) { int cap = qemuMigrationCapabilityTypeFromString(*capStr); @@ -1390,21 +1387,21 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) { migEvent = virBitmapNew(QEMU_MIGRATION_CAP_LAST); if (!migEvent) - goto cleanup; + return -1; ignore_value(virBitmapSetBit(migEvent, QEMU_MIGRATION_CAP_EVENTS)); if (!(json = qemuMigrationCapsToJSON(migEvent, migEvent))) - goto cleanup; + return -1; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - goto cleanup; + return -1; rc = qemuMonitorSetMigrationCapabilities(priv->mon, json); json = NULL; if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; + return -1; if (rc < 0) { virResetLastError(); @@ -1420,13 +1417,7 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver, ignore_value(virBitmapClearBit(priv->migrationCaps, QEMU_MIGRATION_CAP_EVENTS)); - ret = 0; - - cleanup: - virBitmapFree(migEvent); - virJSONValueFree(json); - g_strfreev(caps); - return ret; + return 0; } -- 2.26.2
On a Monday in 2020, Peter Krempa wrote:
Use automatic memory allocation to simplify the code and remove the need for a 'cleanup:' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 33 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 38a5a91f2a..c785f87ab8 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -1352,11 +1352,10 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver, int asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; - virBitmapPtr migEvent = NULL; - virJSONValuePtr json = NULL; - char **caps = NULL; + g_autoptr(virBitmap) migEvent = NULL; + g_autoptr(virJSONValue) json = NULL;
+ VIR_AUTOSTRINGLIST caps = NULL;
You can do: g_auto(GStrv) caps = NULL; here. Either way: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa