[PATCH 00/13] Clean up string list freeing
Switch remaining users of virStringListFreeCount to g_auto(GStrv) and clean up some usage of string lists. Depends on the refactor of the virtual function code I've posted earlier. Pipeline: https://gitlab.com/pipo.sk/libvirt/-/pipelines/348627403 Peter Krempa (13): testQemuAgentSSHKeys: Refactor cleanup remote: dispatch: Don't use virStringListFreeCount for NULL terminated lists qemu: firmware: Store machine types as a NULL-terminated string list qemu: domain: Store passthrough environment variables in a struct qemu: domain: Store capability overrides in NULL-terminated string list qemu: domain: Store passthrough arguments in NULL-terminated string list network: bridge: Store dnsmasq passthrough options in NULL-terminated string list qemuNamespacePrepareOneItem: Restructure code to avoid temporary variables qemuNamespacePrepareOneItem: Don't pass count of elements qemuNamespaceMknodPaths: Remove 'ndevMountsPath' qemuDomainUnshareNamespace: Use automatic memory clearing for string lists lxcContainerSetReadOnly: Refactor cleanup handling util: virstring: Remove unused virStringListFreeCount src/libvirt_private.syms | 1 - src/lxc/lxc_container.c | 27 ++++----- src/network/bridge_driver.c | 23 ++++---- src/qemu/qemu_command.c | 9 +-- src/qemu/qemu_domain.c | 91 +++++++++++++++-------------- src/qemu/qemu_domain.h | 12 ++-- src/qemu/qemu_firmware.c | 4 +- src/qemu/qemu_namespace.c | 46 ++++++--------- src/qemu/qemu_process.c | 13 ++--- src/remote/remote_daemon_dispatch.c | 8 +-- src/util/virstring.c | 23 -------- src/util/virstring.h | 3 - tests/qemuagenttest.c | 54 ++++++++--------- 13 files changed, 135 insertions(+), 179 deletions(-) -- 2.31.1
Use automatic memory freeing for the 'qemuMonitorTest' object and the list of keys so that the cleanup section can be removed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuagenttest.c | 54 ++++++++++++++++++------------------------- 1 file changed, 23 insertions(+), 31 deletions(-) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index e0d2575c45..a447c93494 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -39,16 +39,15 @@ static int testQemuAgentSSHKeys(const void *data) { virDomainXMLOption *xmlopt = (virDomainXMLOption *)data; - qemuMonitorTest *test = qemuMonitorTestNewAgent(xmlopt); - char **keys = NULL; + g_autoptr(qemuMonitorTest) test = qemuMonitorTestNewAgent(xmlopt); + g_auto(GStrv) keys = NULL; int nkeys = 0; - int ret = -1; if (!test) return -1; if (qemuMonitorTestAddAgentSyncResponse(test) < 0) - goto cleanup; + return -1; if (qemuMonitorTestAddItem(test, "guest-ssh-get-authorized-keys", "{\"return\": {" @@ -57,59 +56,52 @@ testQemuAgentSSHKeys(const void *data) " \"algo2 key2 comments2\"" " ]" "}}") < 0) - goto cleanup; + return -1; if (qemuMonitorTestAddAgentSyncResponse(test) < 0) - goto cleanup; + return -1; if (qemuMonitorTestAddItem(test, "guest-ssh-add-authorized-keys", "{ \"return\" : {} }") < 0) - goto cleanup; + return -1; if (qemuMonitorTestAddAgentSyncResponse(test) < 0) - goto cleanup; + return -1; if (qemuMonitorTestAddItem(test, "guest-ssh-remove-authorized-keys", "{ \"return\" : {} }") < 0) - goto cleanup; + return -1; if ((nkeys = qemuAgentSSHGetAuthorizedKeys(qemuMonitorTestGetAgent(test), "user", &keys)) < 0) - goto cleanup; + return -1; if (nkeys != 2) { virReportError(VIR_ERR_INTERNAL_ERROR, "expected 2 keys, got %d", nkeys); - ret = -1; - goto cleanup; + return -1; } if (STRNEQ(keys[1], "algo2 key2 comments2")) { virReportError(VIR_ERR_INTERNAL_ERROR, "Unexpected key returned: %s", keys[1]); - ret = -1; - goto cleanup; + return -1; } - if ((ret = qemuAgentSSHAddAuthorizedKeys(qemuMonitorTestGetAgent(test), - "user", - (const char **) keys, - nkeys, - true)) < 0) - goto cleanup; - - if ((ret = qemuAgentSSHRemoveAuthorizedKeys(qemuMonitorTestGetAgent(test), - "user", - (const char **) keys, - nkeys)) < 0) - goto cleanup; + if (qemuAgentSSHAddAuthorizedKeys(qemuMonitorTestGetAgent(test), + "user", + (const char **) keys, + nkeys, + true) < 0) + return -1; - ret = 0; + if (qemuAgentSSHRemoveAuthorizedKeys(qemuMonitorTestGetAgent(test), + "user", + (const char **) keys, + nkeys) < 0) + return -1; - cleanup: - virStringListFreeCount(keys, nkeys); - qemuMonitorTestFree(test); - return ret; + return 0; } -- 2.31.1
Both virDomainAuthorizedSSHKeysGet and virDomainGetMessages return a NULL-terminated string-list, so we can use g_auto(GStrv) to clear the used memory on failures. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/remote_daemon_dispatch.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 99ae25c5f7..d43306d4c9 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -7253,7 +7253,7 @@ remoteDispatchDomainAuthorizedSshKeysGet(virNetServer *server G_GNUC_UNUSED, int rv = -1; virConnectPtr conn = remoteGetHypervisorConn(client); int nkeys = 0; - char **keys = NULL; + g_auto(GStrv) keys = NULL; virDomainPtr dom = NULL; if (!conn) @@ -7281,8 +7281,6 @@ remoteDispatchDomainAuthorizedSshKeysGet(virNetServer *server G_GNUC_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (nkeys > 0) - virStringListFreeCount(keys, nkeys); virObjectUnref(dom); return rv; @@ -7335,7 +7333,7 @@ remoteDispatchDomainGetMessages(virNetServer *server G_GNUC_UNUSED, int rv = -1; virConnectPtr conn = remoteGetHypervisorConn(client); int nmsgs = 0; - char **msgs = NULL; + g_auto(GStrv) msgs = NULL; virDomainPtr dom = NULL; if (!conn) @@ -7362,8 +7360,6 @@ remoteDispatchDomainGetMessages(virNetServer *server G_GNUC_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (nmsgs > 0) - virStringListFreeCount(msgs, nmsgs); virObjectUnref(dom); return rv; -- 2.31.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_firmware.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 17380b7573..529ab8d68e 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -239,7 +239,7 @@ qemuFirmwareTargetFree(qemuFirmwareTarget *target) if (!target) return; - virStringListFreeCount(target->machines, target->nmachines); + g_strfreev(target->machines); g_free(target); } @@ -534,7 +534,7 @@ qemuFirmwareTargetParse(const char *path, nmachines = virJSONValueArraySize(machines); - t->machines = g_new0(char *, nmachines); + t->machines = g_new0(char *, nmachines + 1); for (j = 0; j < nmachines; j++) { virJSONValue *machine = virJSONValueArrayGet(machines, j); -- 2.31.1
Previously they were stored in two separate arrays. This way it's obvious when referencing the same one. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 45 +++++++++++++++++++++++------------------ src/qemu/qemu_domain.h | 9 +++++++-- 3 files changed, 34 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4381ea7d8b..4bcb5a3146 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10638,8 +10638,8 @@ qemuBuildCommandLine(virQEMUDriver *driver, for (i = 0; i < qemuxmlns->num_args; i++) virCommandAddArg(cmd, qemuxmlns->args[i]); for (i = 0; i < qemuxmlns->num_env; i++) - virCommandAddEnvPair(cmd, qemuxmlns->env_name[i], - NULLSTR_EMPTY(qemuxmlns->env_value[i])); + virCommandAddEnvPair(cmd, qemuxmlns->env[i].name, + NULLSTR_EMPTY(qemuxmlns->env[i].value)); } if (qemuBuildSeccompSandboxCommandLine(cmd, cfg, qemuCaps) < 0) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6f8c93ea0c..21668cae4c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3319,12 +3319,17 @@ virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = { static void qemuDomainXmlNsDefFree(qemuDomainXmlNsDef *def) { + size_t i; + if (!def) return; + for (i = 0; i < def->num_env; i++) { + g_free(def->env[i].name); + g_free(def->env[i].value); + } + virStringListFreeCount(def->args, def->num_args); - virStringListFreeCount(def->env_name, def->num_env); - virStringListFreeCount(def->env_value, def->num_env); virStringListFreeCount(def->capsadd, def->ncapsadd); virStringListFreeCount(def->capsdel, def->ncapsdel); @@ -3372,15 +3377,21 @@ qemuDomainDefNamespaceParseCommandlineArgs(qemuDomainXmlNsDef *nsdef, static int -qemuDomainDefNamespaceParseCommandlineEnvNameValidate(const char *envname) +qemuDomainDefNamespaceParseCommandlineEnvValidate(qemuDomainXmlNsEnvTuple *env) { - if (!g_ascii_isalpha(envname[0]) && envname[0] != '_') { + if (!env->name) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("No qemu environment name specified")); + return -1; + } + + if (!g_ascii_isalpha(env->name[0]) && env->name[0] != '_') { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid environment name, it must begin with a letter or underscore")); return -1; } - if (strspn(envname, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_") != strlen(envname)) { + if (strspn(env->name, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_") != strlen(env->name)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid environment name, it must contain only alphanumerics and underscore")); return -1; @@ -3404,22 +3415,17 @@ qemuDomainDefNamespaceParseCommandlineEnv(qemuDomainXmlNsDef *nsdef, if (nnodes == 0) return 0; - nsdef->env_name = g_new0(char *, nnodes); - nsdef->env_value = g_new0(char *, nnodes); + nsdef->env = g_new0(qemuDomainXmlNsEnvTuple, nnodes); for (i = 0; i < nnodes; i++) { - if (!(nsdef->env_name[nsdef->num_env] = virXMLPropString(nodes[i], "name"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("No qemu environment name specified")); - return -1; - } + qemuDomainXmlNsEnvTuple *env = nsdef->env + i; - if (qemuDomainDefNamespaceParseCommandlineEnvNameValidate(nsdef->env_name[nsdef->num_env]) < 0) - return -1; - - nsdef->env_value[nsdef->num_env] = virXMLPropString(nodes[i], "value"); - /* a NULL value for command is allowed, since it might be empty */ + env->name = virXMLPropString(nodes[i], "name"); + env->value = virXMLPropString(nodes[i], "value"); nsdef->num_env++; + + if (qemuDomainDefNamespaceParseCommandlineEnvValidate(env) < 0) + return -1; } return 0; @@ -3513,9 +3519,8 @@ qemuDomainDefNamespaceFormatXMLCommandline(virBuffer *buf, virBufferEscapeString(buf, "<qemu:arg value='%s'/>\n", cmd->args[i]); for (i = 0; i < cmd->num_env; i++) { - virBufferAsprintf(buf, "<qemu:env name='%s'", cmd->env_name[i]); - if (cmd->env_value[i]) - virBufferEscapeString(buf, " value='%s'", cmd->env_value[i]); + virBufferAsprintf(buf, "<qemu:env name='%s'", cmd->env[i].name); + virBufferEscapeString(buf, " value='%s'", cmd->env[i].value); virBufferAddLit(buf, "/>\n"); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index acf6ca5ab6..5f2814271d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -457,14 +457,19 @@ struct _qemuDomainSaveCookie { G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuDomainSaveCookie, virObjectUnref); +typedef struct _qemuDomainXmlNsEnvTuple qemuDomainXmlNsEnvTuple; +struct _qemuDomainXmlNsEnvTuple { + char *name; + char *value; +}; + typedef struct _qemuDomainXmlNsDef qemuDomainXmlNsDef; struct _qemuDomainXmlNsDef { size_t num_args; char **args; unsigned int num_env; - char **env_name; - char **env_value; + qemuDomainXmlNsEnvTuple *env; size_t ncapsadd; char **capsadd; -- 2.31.1
On Thu, Aug 05, 2021 at 17:34:20 +0200, Peter Krempa wrote:
Previously they were stored in two separate arrays. This way it's obvious when referencing the same one.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 45 +++++++++++++++++++++++------------------ src/qemu/qemu_domain.h | 9 +++++++-- 3 files changed, 34 insertions(+), 24 deletions(-)
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6f8c93ea0c..21668cae4c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3319,12 +3319,17 @@ virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = { static void qemuDomainXmlNsDefFree(qemuDomainXmlNsDef *def) { + size_t i; + if (!def) return;
+ for (i = 0; i < def->num_env; i++) { + g_free(def->env[i].name); + g_free(def->env[i].value); + } + virStringListFreeCount(def->args, def->num_args);
Consider the following squashed in: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f5d4db33bf..bc448e48a4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3328,6 +3328,7 @@ qemuDomainXmlNsDefFree(qemuDomainXmlNsDef *def) g_free(def->env[i].name); g_free(def->env[i].value); } + g_free(def->env); g_strfreev(def->args); g_strfreev(def->capsadd);
We always process the full list so there's no value in storing the count separately. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 28 ++++++++++++++-------------- src/qemu/qemu_domain.h | 2 -- src/qemu/qemu_process.c | 13 ++++++------- 3 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 21668cae4c..b2cbd3d6ba 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3330,8 +3330,8 @@ qemuDomainXmlNsDefFree(qemuDomainXmlNsDef *def) } virStringListFreeCount(def->args, def->num_args); - virStringListFreeCount(def->capsadd, def->ncapsadd); - virStringListFreeCount(def->capsdel, def->ncapsdel); + g_strfreev(def->capsadd); + g_strfreev(def->capsdel); g_free(def->deprecationBehavior); @@ -3447,10 +3447,10 @@ qemuDomainDefNamespaceParseCaps(qemuDomainXmlNsDef *nsdef, return -1; if (nnodesadd > 0) { - nsdef->capsadd = g_new0(char *, nnodesadd); + nsdef->capsadd = g_new0(char *, nnodesadd + 1); for (i = 0; i < nnodesadd; i++) { - if (!(nsdef->capsadd[nsdef->ncapsadd++] = virXMLPropString(nodesadd[i], "capability"))) { + if (!(nsdef->capsadd[i] = virXMLPropString(nodesadd[i], "capability"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing capability name")); return -1; @@ -3459,10 +3459,10 @@ qemuDomainDefNamespaceParseCaps(qemuDomainXmlNsDef *nsdef, } if (nnodesdel > 0) { - nsdef->capsdel = g_new0(char *, nnodesdel); + nsdef->capsdel = g_new0(char *, nnodesdel + 1); for (i = 0; i < nnodesdel; i++) { - if (!(nsdef->capsdel[nsdef->ncapsdel++] = virXMLPropString(nodesdel[i], "capability"))) { + if (!(nsdef->capsdel[i] = virXMLPropString(nodesdel[i], "capability"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing capability name")); return -1; @@ -3491,7 +3491,7 @@ qemuDomainDefNamespaceParse(xmlXPathContextPtr ctxt, nsdata->deprecationBehavior = virXPathString("string(./qemu:deprecation/@behavior)", ctxt); if (nsdata->num_args > 0 || nsdata->num_env > 0 || - nsdata->ncapsadd > 0 || nsdata->ncapsdel > 0 || + nsdata->capsadd || nsdata->capsdel || nsdata->deprecationBehavior) *data = g_steal_pointer(&nsdata); @@ -3533,19 +3533,19 @@ static void qemuDomainDefNamespaceFormatXMLCaps(virBuffer *buf, qemuDomainXmlNsDef *xmlns) { - size_t i; + GStrv n; - if (!xmlns->ncapsadd && !xmlns->ncapsdel) + if (!xmlns->capsadd && !xmlns->capsdel) return; virBufferAddLit(buf, "<qemu:capabilities>\n"); virBufferAdjustIndent(buf, 2); - for (i = 0; i < xmlns->ncapsadd; i++) - virBufferEscapeString(buf, "<qemu:add capability='%s'/>\n", xmlns->capsadd[i]); + for (n = xmlns->capsadd; n && *n; n++) + virBufferEscapeString(buf, "<qemu:add capability='%s'/>\n", *n); - for (i = 0; i < xmlns->ncapsdel; i++) - virBufferEscapeString(buf, "<qemu:del capability='%s'/>\n", xmlns->capsdel[i]); + for (n = xmlns->capsdel; n && *n; n++) + virBufferEscapeString(buf, "<qemu:del capability='%s'/>\n", *n); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</qemu:capabilities>\n"); @@ -6616,7 +6616,7 @@ void qemuDomainObjCheckTaint(virQEMUDriver *driver, qemuDomainXmlNsDef *qemuxmlns = obj->def->namespaceData; if (qemuxmlns->num_args || qemuxmlns->num_env) qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CUSTOM_ARGV, logCtxt); - if (qemuxmlns->ncapsadd > 0 || qemuxmlns->ncapsdel > 0) + if (qemuxmlns->capsadd || qemuxmlns->capsdel) custom_hypervisor_feat = true; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 5f2814271d..aa3ed78094 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -471,10 +471,8 @@ struct _qemuDomainXmlNsDef { unsigned int num_env; qemuDomainXmlNsEnvTuple *env; - size_t ncapsadd; char **capsadd; - size_t ncapsdel; char **capsdel; /* We deliberately keep this as a string so that it's parsed only when diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 81af4f1a44..4264191a9a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5484,7 +5484,6 @@ qemuProcessStartUpdateCustomCaps(virDomainObj *vm) qemuDomainXmlNsDef *nsdef = vm->def->namespaceData; char **next; int tmp; - size_t i; if (cfg->capabilityfilters) { for (next = cfg->capabilityfilters; *next; next++) { @@ -5500,22 +5499,22 @@ qemuProcessStartUpdateCustomCaps(virDomainObj *vm) } if (nsdef) { - for (i = 0; i < nsdef->ncapsadd; i++) { - if ((tmp = virQEMUCapsTypeFromString(nsdef->capsadd[i])) < 0) { + for (next = nsdef->capsadd; next && *next; next++) { + if ((tmp = virQEMUCapsTypeFromString(*next)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid qemu namespace capability '%s'"), - nsdef->capsadd[i]); + *next); return -1; } virQEMUCapsSet(priv->qemuCaps, tmp); } - for (i = 0; i < nsdef->ncapsdel; i++) { - if ((tmp = virQEMUCapsTypeFromString(nsdef->capsdel[i])) < 0) { + for (next = nsdef->capsdel; next && *next; next++) { + if ((tmp = virQEMUCapsTypeFromString(*next)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid qemu namespace capability '%s'"), - nsdef->capsdel[i]); + *next); return -1; } -- 2.31.1
We always process the full list so there's no value in storing the count separately. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 5 +++-- src/qemu/qemu_domain.c | 18 +++++++++--------- src/qemu/qemu_domain.h | 1 - 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4bcb5a3146..ecfed19432 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10633,10 +10633,11 @@ qemuBuildCommandLine(virQEMUDriver *driver, if (def->namespaceData) { qemuDomainXmlNsDef *qemuxmlns; + GStrv n; qemuxmlns = def->namespaceData; - for (i = 0; i < qemuxmlns->num_args; i++) - virCommandAddArg(cmd, qemuxmlns->args[i]); + for (n = qemuxmlns->args; n && *n; n++) + virCommandAddArg(cmd, *n); for (i = 0; i < qemuxmlns->num_env; i++) virCommandAddEnvPair(cmd, qemuxmlns->env[i].name, NULLSTR_EMPTY(qemuxmlns->env[i].value)); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b2cbd3d6ba..f5d4db33bf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3329,7 +3329,7 @@ qemuDomainXmlNsDefFree(qemuDomainXmlNsDef *def) g_free(def->env[i].value); } - virStringListFreeCount(def->args, def->num_args); + g_strfreev(def->args); g_strfreev(def->capsadd); g_strfreev(def->capsdel); @@ -3362,10 +3362,10 @@ qemuDomainDefNamespaceParseCommandlineArgs(qemuDomainXmlNsDef *nsdef, if (nnodes == 0) return 0; - nsdef->args = g_new0(char *, nnodes); + nsdef->args = g_new0(char *, nnodes + 1); for (i = 0; i < nnodes; i++) { - if (!(nsdef->args[nsdef->num_args++] = virXMLPropString(nodes[i], "value"))) { + if (!(nsdef->args[i] = virXMLPropString(nodes[i], "value"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No qemu command-line argument specified")); return -1; @@ -3490,7 +3490,7 @@ qemuDomainDefNamespaceParse(xmlXPathContextPtr ctxt, nsdata->deprecationBehavior = virXPathString("string(./qemu:deprecation/@behavior)", ctxt); - if (nsdata->num_args > 0 || nsdata->num_env > 0 || + if (nsdata->args || nsdata->num_env > 0 || nsdata->capsadd || nsdata->capsdel || nsdata->deprecationBehavior) *data = g_steal_pointer(&nsdata); @@ -3507,17 +3507,17 @@ static void qemuDomainDefNamespaceFormatXMLCommandline(virBuffer *buf, qemuDomainXmlNsDef *cmd) { + GStrv n; size_t i; - if (!cmd->num_args && !cmd->num_env) + if (!cmd->args && !cmd->num_env) return; virBufferAddLit(buf, "<qemu:commandline>\n"); virBufferAdjustIndent(buf, 2); - for (i = 0; i < cmd->num_args; i++) - virBufferEscapeString(buf, "<qemu:arg value='%s'/>\n", - cmd->args[i]); + for (n = cmd->args; n && *n; n++) + virBufferEscapeString(buf, "<qemu:arg value='%s'/>\n", *n); for (i = 0; i < cmd->num_env; i++) { virBufferAsprintf(buf, "<qemu:env name='%s'", cmd->env[i].name); virBufferEscapeString(buf, " value='%s'", cmd->env[i].value); @@ -6614,7 +6614,7 @@ void qemuDomainObjCheckTaint(virQEMUDriver *driver, if (obj->def->namespaceData) { qemuDomainXmlNsDef *qemuxmlns = obj->def->namespaceData; - if (qemuxmlns->num_args || qemuxmlns->num_env) + if (qemuxmlns->args || qemuxmlns->num_env) qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CUSTOM_ARGV, logCtxt); if (qemuxmlns->capsadd || qemuxmlns->capsdel) custom_hypervisor_feat = true; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index aa3ed78094..3cfa6cd44e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -465,7 +465,6 @@ struct _qemuDomainXmlNsEnvTuple { typedef struct _qemuDomainXmlNsDef qemuDomainXmlNsDef; struct _qemuDomainXmlNsDef { - size_t num_args; char **args; unsigned int num_env; -- 2.31.1
We always process the full list so there's no value in storing the count separately. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/network/bridge_driver.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2555119892..acbc6be965 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -145,7 +145,6 @@ extern virXMLNamespace networkDnsmasqXMLNamespace; typedef struct _networkDnsmasqXmlNsDef networkDnsmasqXmlNsDef; struct _networkDnsmasqXmlNsDef { - size_t noptions; char **options; }; @@ -157,7 +156,7 @@ networkDnsmasqDefNamespaceFree(void *nsdata) if (!def) return; - virStringListFreeCount(def->options, def->noptions); + g_strfreev(def->options); g_free(def); } @@ -179,10 +178,10 @@ networkDnsmasqDefNamespaceParseOptions(networkDnsmasqXmlNsDef *nsdef, if (nnodes == 0) return 0; - nsdef->options = g_new0(char *, nnodes); + nsdef->options = g_new0(char *, nnodes + 1); for (i = 0; i < nnodes; i++) { - if (!(nsdef->options[nsdef->noptions++] = virXMLPropString(nodes[i], "value"))) { + if (!(nsdef->options[i] = virXMLPropString(nodes[i], "value"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No dnsmasq options value specified")); return -1; @@ -202,7 +201,7 @@ networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt, if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt)) return -1; - if (nsdata->noptions > 0) + if (nsdata->options) *data = g_steal_pointer(&nsdata); return 0; @@ -214,17 +213,16 @@ networkDnsmasqDefNamespaceFormatXML(virBuffer *buf, void *nsdata) { networkDnsmasqXmlNsDef *def = nsdata; - size_t i; + GStrv n; - if (!def->noptions) + if (!def->options) return 0; virBufferAddLit(buf, "<dnsmasq:options>\n"); virBufferAdjustIndent(buf, 2); - for (i = 0; i < def->noptions; i++) { - virBufferEscapeString(buf, "<dnsmasq:option value='%s'/>\n", - def->options[i]); + for (n = def->options; *n; n++) { + virBufferEscapeString(buf, "<dnsmasq:option value='%s'/>\n", *n); } virBufferAdjustIndent(buf, -2); @@ -1554,8 +1552,9 @@ networkDnsmasqConfContents(virNetworkObj *obj, if (def->namespaceData) { networkDnsmasqXmlNsDef *dnsmasqxmlns = def->namespaceData; - for (i = 0; i < dnsmasqxmlns->noptions; i++) - virBufferAsprintf(&configbuf, "%s\n", dnsmasqxmlns->options[i]); + GStrv n; + for (n = dnsmasqxmlns->options; n && *n; n++) + virBufferAsprintf(&configbuf, "%s\n", *n); } if (!(*configstr = virBufferContentAndReset(&configbuf))) -- 2.31.1
The value of 'next' is copied into 'item.file' so we can move the update to the 'next' pointer earlier and move the VIR_APPEND_ELEMENT call to where we figure out that we need to append the value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_namespace.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index ca59b5d95b..01a5edc585 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -1197,7 +1197,6 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodData *data, while (1) { g_auto(qemuNamespaceMknodItem) item = { 0 }; bool isLink; - bool addToData = false; int rc; rc = qemuNamespaceMknodItemInit(&item, cfg, vm, next); @@ -1210,25 +1209,21 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodData *data, } isLink = S_ISLNK(item.sb.st_mode); + g_free(next); + next = g_strdup(item.target); - if (STRPREFIX(next, QEMU_DEVPREFIX)) { + if (STRPREFIX(item.file, QEMU_DEVPREFIX)) { for (i = 0; i < ndevMountsPath; i++) { if (STREQ(devMountsPath[i], "/dev")) continue; - if (STRPREFIX(next, devMountsPath[i])) + if (STRPREFIX(item.file, devMountsPath[i])) break; } if (i == ndevMountsPath) - addToData = true; + VIR_APPEND_ELEMENT(data->items, data->nitems, item); } - g_free(next); - next = g_strdup(item.target); - - if (addToData) - VIR_APPEND_ELEMENT(data->items, data->nitems, item); - if (!isLink) break; -- 2.31.1
The only caller is passing a NULL terminated string list as 'devMountsPath' thus we don't need to get the count of elements. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_namespace.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 01a5edc585..06169922b9 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -1187,12 +1187,10 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodData *data, virQEMUDriverConfig *cfg, virDomainObj *vm, const char *file, - char * const *devMountsPath, - size_t ndevMountsPath) + GStrv devMountsPath) { long ttl = sysconf(_SC_SYMLOOP_MAX); g_autofree char *next = g_strdup(file); - size_t i; while (1) { g_auto(qemuNamespaceMknodItem) item = { 0 }; @@ -1213,14 +1211,19 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodData *data, next = g_strdup(item.target); if (STRPREFIX(item.file, QEMU_DEVPREFIX)) { - for (i = 0; i < ndevMountsPath; i++) { - if (STREQ(devMountsPath[i], "/dev")) + GStrv n; + bool found = false; + + for (n = devMountsPath; n && *n; n++) { + if (STREQ(*n, "/dev")) continue; - if (STRPREFIX(item.file, devMountsPath[i])) + if (STRPREFIX(item.file, *n)) { + found = true; break; + } } - if (i == ndevMountsPath) + if (!found) VIR_APPEND_ELEMENT(data->items, data->nitems, item); } @@ -1269,8 +1272,7 @@ qemuNamespaceMknodPaths(virDomainObj *vm, for (next = paths; next; next = next->next) { const char *path = next->data; - if (qemuNamespacePrepareOneItem(&data, cfg, vm, path, - devMountsPath, ndevMountsPath) < 0) + if (qemuNamespacePrepareOneItem(&data, cfg, vm, path, devMountsPath) < 0) goto cleanup; } -- 2.31.1
'devMountsPath' can be converted to an auto-cleared stringlist and thus asking for the number of entries is not necessary. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_namespace.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 06169922b9..b2e6ecb71a 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -1250,8 +1250,7 @@ qemuNamespaceMknodPaths(virDomainObj *vm, qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; g_autoptr(virQEMUDriverConfig) cfg = NULL; - char **devMountsPath = NULL; - size_t ndevMountsPath = 0; + g_auto(GStrv) devMountsPath = NULL; qemuNamespaceMknodData data = { 0 }; size_t i; int ret = -1; @@ -1261,9 +1260,7 @@ qemuNamespaceMknodPaths(virDomainObj *vm, return 0; cfg = virQEMUDriverGetConfig(driver); - if (qemuDomainGetPreservedMounts(cfg, vm, - &devMountsPath, NULL, - &ndevMountsPath) < 0) + if (qemuDomainGetPreservedMounts(cfg, vm, &devMountsPath, NULL, NULL) < 0) return -1; data.driver = driver; @@ -1304,7 +1301,6 @@ qemuNamespaceMknodPaths(virDomainObj *vm, } } qemuNamespaceMknodDataClear(&data); - virStringListFreeCount(devMountsPath, ndevMountsPath); return ret; } -- 2.31.1
'devMountsPath' and 'devMountsSavePath' are NULL terminated. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_namespace.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index b2e6ecb71a..728d77fc61 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -682,7 +682,8 @@ qemuDomainUnshareNamespace(virQEMUDriverConfig *cfg, virDomainObj *vm) { const char *devPath = NULL; - char **devMountsPath = NULL, **devMountsSavePath = NULL; + g_auto(GStrv) devMountsPath = NULL; + g_auto(GStrv) devMountsSavePath = NULL; size_t ndevMountsPath = 0, i; int ret = -1; @@ -791,8 +792,6 @@ qemuDomainUnshareNamespace(virQEMUDriverConfig *cfg, else unlink(devMountsSavePath[i]); } - virStringListFreeCount(devMountsPath, ndevMountsPath); - virStringListFreeCount(devMountsSavePath, ndevMountsPath); return ret; } -- 2.31.1
Turn 'mounts' into a proper GStrv after sorting so that automatic cleanup can be used and shuffle around the cleanup steps so that jumps can be avoided in favor of direct return of error code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_container.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 1cadfe70e0..d788e77196 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -773,8 +773,7 @@ static int lxcContainerSetReadOnly(void) FILE *procmnt; struct mntent mntent; char mntbuf[1024]; - int ret = -1; - char **mounts = NULL; + g_auto(GStrv) mounts = NULL; size_t nmounts = 0; size_t i; @@ -797,13 +796,16 @@ static int lxcContainerSetReadOnly(void) VIR_APPEND_ELEMENT(mounts, nmounts, tmp); } - if (!mounts) { - ret = 0; - goto cleanup; - } + endmntent(procmnt); + + if (!mounts) + return 0; + + qsort(mounts, nmounts, sizeof(mounts[0]), virStringSortRevCompare); - qsort(mounts, nmounts, sizeof(mounts[0]), - virStringSortRevCompare); + /* turn 'mounts' into a proper GStrv */ + VIR_EXPAND_N(mounts, nmounts, 1); + nmounts--; for (i = 0; i < nmounts; i++) { VIR_DEBUG("Bind readonly %s", mounts[i]); @@ -811,16 +813,11 @@ static int lxcContainerSetReadOnly(void) virReportSystemError(errno, _("Failed to make mount %s readonly"), mounts[i]); - goto cleanup; + return -1; } } - ret = 0; - cleanup: - virStringListFreeCount(mounts, nmounts); - endmntent(procmnt); - return ret; - + return 0; } -- 2.31.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virstring.c | 23 ----------------------- src/util/virstring.h | 3 --- 3 files changed, 27 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fd02b27c51..e177591fa8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3288,7 +3288,6 @@ virStringHasControlChars; virStringHasSuffix; virStringIsEmpty; virStringIsPrintable; -virStringListFreeCount; virStringListMerge; virStringMatch; virStringMatchesNameSuffix; diff --git a/src/util/virstring.c b/src/util/virstring.c index a2c07e5c17..f416fed3c5 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -70,29 +70,6 @@ virStringListMerge(char ***dst, } -/** - * virStringListFreeCount: - * @strings: array of strings to free - * @count: number of elements in the array - * - * Frees a string array of @count length. - */ -void -virStringListFreeCount(char **strings, - size_t count) -{ - size_t i; - - if (!strings) - return; - - for (i = 0; i < count; i++) - g_free(strings[i]); - - g_free(strings); -} - - /* Like strtol, but produce an "int" result, and check more carefully. Return 0 upon success; return -1 to indicate failure. When END_PTR is NULL, the byte after the final valid digit must be NUL. diff --git a/src/util/virstring.h b/src/util/virstring.h index 7cc1d8c55f..ca81889c9b 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -25,9 +25,6 @@ int virStringListMerge(char ***dst, char ***src); -void virStringListFreeCount(char **strings, - size_t count); - int virStrToLong_i(char const *s, char **end_ptr, int base, -- 2.31.1
On a %A in %Y, Peter Krempa wrote:
Switch remaining users of virStringListFreeCount to g_auto(GStrv) and clean up some usage of string lists.
Depends on the refactor of the virtual function code I've posted earlier.
Pipeline:
https://gitlab.com/pipo.sk/libvirt/-/pipelines/348627403
Peter Krempa (13): testQemuAgentSSHKeys: Refactor cleanup remote: dispatch: Don't use virStringListFreeCount for NULL terminated lists qemu: firmware: Store machine types as a NULL-terminated string list qemu: domain: Store passthrough environment variables in a struct qemu: domain: Store capability overrides in NULL-terminated string list qemu: domain: Store passthrough arguments in NULL-terminated string list network: bridge: Store dnsmasq passthrough options in NULL-terminated string list qemuNamespacePrepareOneItem: Restructure code to avoid temporary variables qemuNamespacePrepareOneItem: Don't pass count of elements qemuNamespaceMknodPaths: Remove 'ndevMountsPath' qemuDomainUnshareNamespace: Use automatic memory clearing for string lists lxcContainerSetReadOnly: Refactor cleanup handling util: virstring: Remove unused virStringListFreeCount
src/libvirt_private.syms | 1 - src/lxc/lxc_container.c | 27 ++++----- src/network/bridge_driver.c | 23 ++++---- src/qemu/qemu_command.c | 9 +-- src/qemu/qemu_domain.c | 91 +++++++++++++++-------------- src/qemu/qemu_domain.h | 12 ++-- src/qemu/qemu_firmware.c | 4 +- src/qemu/qemu_namespace.c | 46 ++++++--------- src/qemu/qemu_process.c | 13 ++--- src/remote/remote_daemon_dispatch.c | 8 +-- src/util/virstring.c | 23 -------- src/util/virstring.h | 3 - tests/qemuagenttest.c | 54 ++++++++--------- 13 files changed, 135 insertions(+), 179 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Jano Tomko -
Peter Krempa