[libvirt] [PATCH 00/14] virsh: introduce a password completer
Ján Tomko (14): virsh: fix indentation of info_managed_save_edit virsh-completer: fix typo virsh-completer: reword documentation virsh-completer: switch to using tmp instead of ret virsh-completer: add a cleanup label everywhere virsh-completer: unify cleanup of items in name completers virsh-completer: virStringListFree where possible virsh-completer: use VIR_AUTOSTRINGLIST for tmp use VIR_AUTOFREE for xmlNodePtr* variables virsh-completer: use VIR_AUTOPTR for xml* variables virsh-completer: use VIR_AUTOFREE for char* variables virsh-completer: remove excessive labels virsh-completer: introduce virshPagesizeNodeToString virsh-completer: introduce virshPasswordCompleter tools/virsh-completer.c | 611 +++++++++++++++++++--------------------- tools/virsh-completer.h | 4 + tools/virsh-domain.c | 15 +- 3 files changed, 295 insertions(+), 335 deletions(-) -- 2.20.1
Use four spaces instead of three. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index afcd0a5f8d..e8d5404acf 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4805,13 +4805,13 @@ cmdManagedSaveRemove(vshControl *ctl, const vshCmd *cmd) * "managedsave-edit" command */ static const vshCmdInfo info_managed_save_edit[] = { - {.name = "help", - .data = N_("edit XML for a domain's managed save state file") - }, - {.name = "desc", - .data = N_("Edit the domain XML associated with the managed save state file") - }, - {.name = NULL} + {.name = "help", + .data = N_("edit XML for a domain's managed save state file") + }, + {.name = "desc", + .data = N_("Edit the domain XML associated with the managed save state file") + }, + {.name = NULL} }; static const vshCmdOptDef opts_managed_save_edit[] = { -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:18 +0200, Ján Tomko wrote:
Use four spaces instead of three.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
ACK
Use the posessive determiner instead of a contracted auxiliary. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index e9ef9b99f9..760ed2088f 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -58,7 +58,7 @@ * * The @flags contains a .completer_flags value defined for each * use or 0 if no .completer_flags were specified. If a completer - * is generic enough @flags can be used to alter it's behaviour. + * is generic enough @flags can be used to alter its behaviour. * For instance, a completer to fetch names of domains can use * @flags to return names of only domains in a particular state * that the command accepts. -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:19 +0200, Ján Tomko wrote:
Use the posessive determiner instead of a contracted auxiliary.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
Use less strict wording that does not threaten the reader with circumstances. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 760ed2088f..464a2751fa 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -63,9 +63,9 @@ * @flags to return names of only domains in a particular state * that the command accepts. * - * Under no circumstances should a completer output anything. - * Neither to stdout nor to stderr. This would harm the user - * experience. + * A completer may not harm the user experience by outputting + * anything or, through inaction, allow the user experience + * to be harmed by output. */ -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:20 +0200, Ján Tomko wrote:
Use less strict wording that does not threaten the reader with circumstances.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
NACK
Construct the potential return value in an array called 'tmp' and only assign it to 'ret' if we're going to return it. This will allow us to unify the error and success paths. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 195 +++++++++++++++++++++++++--------------- 1 file changed, 125 insertions(+), 70 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 464a2751fa..0750945b84 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -79,6 +79,7 @@ virshDomainNameCompleter(vshControl *ctl, int ndomains = 0; size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | VIR_CONNECT_LIST_DOMAINS_INACTIVE | @@ -95,19 +96,21 @@ virshDomainNameCompleter(vshControl *ctl, if ((ndomains = virConnectListAllDomains(priv->conn, &domains, flags)) < 0) return NULL; - if (VIR_ALLOC_N(ret, ndomains + 1) < 0) + if (VIR_ALLOC_N(tmp, ndomains + 1) < 0) goto error; for (i = 0; i < ndomains; i++) { const char *name = virDomainGetName(domains[i]); - if (VIR_STRDUP(ret[i], name) < 0) + if (VIR_STRDUP(tmp[i], name) < 0) goto error; virshDomainFree(domains[i]); } VIR_FREE(domains); + VIR_STEAL_PTR(ret, tmp); + return ret; error: @@ -115,8 +118,8 @@ virshDomainNameCompleter(vshControl *ctl, virshDomainFree(domains[i]); VIR_FREE(domains); for (i = 0; i < ndomains; i++) - VIR_FREE(ret[i]); - VIR_FREE(ret); + VIR_FREE(tmp[i]); + VIR_FREE(tmp); return NULL; } @@ -232,6 +235,7 @@ virshStoragePoolNameCompleter(vshControl *ctl, int npools = 0; size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE | VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE | @@ -244,19 +248,21 @@ virshStoragePoolNameCompleter(vshControl *ctl, if ((npools = virConnectListAllStoragePools(priv->conn, &pools, flags)) < 0) return NULL; - if (VIR_ALLOC_N(ret, npools + 1) < 0) + if (VIR_ALLOC_N(tmp, npools + 1) < 0) goto error; for (i = 0; i < npools; i++) { const char *name = virStoragePoolGetName(pools[i]); - if (VIR_STRDUP(ret[i], name) < 0) + if (VIR_STRDUP(tmp[i], name) < 0) goto error; virStoragePoolFree(pools[i]); } VIR_FREE(pools); + VIR_STEAL_PTR(ret, tmp); + return ret; error: @@ -264,8 +270,8 @@ virshStoragePoolNameCompleter(vshControl *ctl, virStoragePoolFree(pools[i]); VIR_FREE(pools); for (i = 0; i < npools; i++) - VIR_FREE(ret[i]); - VIR_FREE(ret); + VIR_FREE(tmp[i]); + VIR_FREE(tmp); return NULL; } @@ -282,6 +288,7 @@ virshStorageVolNameCompleter(vshControl *ctl, int nvols = 0; size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); @@ -295,20 +302,23 @@ virshStorageVolNameCompleter(vshControl *ctl, goto error; nvols = rc; - if (VIR_ALLOC_N(ret, nvols + 1) < 0) + if (VIR_ALLOC_N(tmp, nvols + 1) < 0) goto error; for (i = 0; i < nvols; i++) { const char *name = virStorageVolGetName(vols[i]); - if (VIR_STRDUP(ret[i], name) < 0) + if (VIR_STRDUP(tmp[i], name) < 0) goto error; virStorageVolFree(vols[i]); } + VIR_FREE(vols); virStoragePoolFree(pool); + VIR_STEAL_PTR(ret, tmp); + return ret; error: @@ -316,8 +326,8 @@ virshStorageVolNameCompleter(vshControl *ctl, virStorageVolFree(vols[i]); VIR_FREE(vols); for (i = 0; i < nvols; i++) - VIR_FREE(ret[i]); - VIR_FREE(ret); + VIR_FREE(tmp[i]); + VIR_FREE(tmp); virStoragePoolFree(pool); return NULL; } @@ -333,6 +343,7 @@ virshInterfaceNameCompleter(vshControl *ctl, int nifaces = 0; size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(VIR_CONNECT_LIST_INTERFACES_ACTIVE | VIR_CONNECT_LIST_INTERFACES_INACTIVE, @@ -344,19 +355,21 @@ virshInterfaceNameCompleter(vshControl *ctl, if ((nifaces = virConnectListAllInterfaces(priv->conn, &ifaces, flags)) < 0) return NULL; - if (VIR_ALLOC_N(ret, nifaces + 1) < 0) + if (VIR_ALLOC_N(tmp, nifaces + 1) < 0) goto error; for (i = 0; i < nifaces; i++) { const char *name = virInterfaceGetName(ifaces[i]); - if (VIR_STRDUP(ret[i], name) < 0) + if (VIR_STRDUP(tmp[i], name) < 0) goto error; virInterfaceFree(ifaces[i]); } VIR_FREE(ifaces); + VIR_STEAL_PTR(ret, tmp); + return ret; error: @@ -364,8 +377,8 @@ virshInterfaceNameCompleter(vshControl *ctl, virInterfaceFree(ifaces[i]); VIR_FREE(ifaces); for (i = 0; i < nifaces; i++) - VIR_FREE(ret[i]); - VIR_FREE(ret); + VIR_FREE(tmp[i]); + VIR_FREE(tmp); return NULL; } @@ -380,6 +393,7 @@ virshNetworkNameCompleter(vshControl *ctl, int nnets = 0; size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(VIR_CONNECT_LIST_NETWORKS_INACTIVE | VIR_CONNECT_LIST_NETWORKS_ACTIVE | @@ -392,19 +406,21 @@ virshNetworkNameCompleter(vshControl *ctl, if ((nnets = virConnectListAllNetworks(priv->conn, &nets, flags)) < 0) return NULL; - if (VIR_ALLOC_N(ret, nnets + 1) < 0) + if (VIR_ALLOC_N(tmp, nnets + 1) < 0) goto error; for (i = 0; i < nnets; i++) { const char *name = virNetworkGetName(nets[i]); - if (VIR_STRDUP(ret[i], name) < 0) + if (VIR_STRDUP(tmp[i], name) < 0) goto error; virNetworkFree(nets[i]); } VIR_FREE(nets); + VIR_STEAL_PTR(ret, tmp); + return ret; error: @@ -412,8 +428,8 @@ virshNetworkNameCompleter(vshControl *ctl, virNetworkFree(nets[i]); VIR_FREE(nets); for (i = 0; i < nnets; i++) - VIR_FREE(ret[i]); - VIR_FREE(ret); + VIR_FREE(tmp[i]); + VIR_FREE(tmp); return NULL; } @@ -425,21 +441,24 @@ virshNetworkEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, { size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); - if (VIR_ALLOC_N(ret, VIR_NETWORK_EVENT_ID_LAST + 1) < 0) + if (VIR_ALLOC_N(tmp, VIR_NETWORK_EVENT_ID_LAST + 1) < 0) goto error; for (i = 0; i < VIR_NETWORK_EVENT_ID_LAST; i++) { - if (VIR_STRDUP(ret[i], virshNetworkEventCallbacks[i].name) < 0) + if (VIR_STRDUP(tmp[i], virshNetworkEventCallbacks[i].name) < 0) goto error; } + VIR_STEAL_PTR(ret, tmp); + return ret; error: - virStringListFree(ret); + virStringListFree(tmp); return NULL; } @@ -454,6 +473,7 @@ virshNodeDeviceNameCompleter(vshControl *ctl, int ndevs = 0; size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); @@ -463,19 +483,21 @@ virshNodeDeviceNameCompleter(vshControl *ctl, if ((ndevs = virConnectListAllNodeDevices(priv->conn, &devs, flags)) < 0) return NULL; - if (VIR_ALLOC_N(ret, ndevs + 1) < 0) + if (VIR_ALLOC_N(tmp, ndevs + 1) < 0) goto error; for (i = 0; i < ndevs; i++) { const char *name = virNodeDeviceGetName(devs[i]); - if (VIR_STRDUP(ret[i], name) < 0) + if (VIR_STRDUP(tmp[i], name) < 0) goto error; virNodeDeviceFree(devs[i]); } VIR_FREE(devs); + VIR_STEAL_PTR(ret, tmp); + return ret; error: @@ -483,8 +505,8 @@ virshNodeDeviceNameCompleter(vshControl *ctl, virNodeDeviceFree(devs[i]); VIR_FREE(devs); for (i = 0; i < ndevs; i++) - VIR_FREE(ret[i]); - VIR_FREE(ret); + VIR_FREE(tmp[i]); + VIR_FREE(tmp); return NULL; } @@ -499,6 +521,7 @@ virshNWFilterNameCompleter(vshControl *ctl, int nnwfilters = 0; size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); @@ -508,19 +531,21 @@ virshNWFilterNameCompleter(vshControl *ctl, if ((nnwfilters = virConnectListAllNWFilters(priv->conn, &nwfilters, flags)) < 0) return NULL; - if (VIR_ALLOC_N(ret, nnwfilters + 1) < 0) + if (VIR_ALLOC_N(tmp, nnwfilters + 1) < 0) goto error; for (i = 0; i < nnwfilters; i++) { const char *name = virNWFilterGetName(nwfilters[i]); - if (VIR_STRDUP(ret[i], name) < 0) + if (VIR_STRDUP(tmp[i], name) < 0) goto error; virNWFilterFree(nwfilters[i]); } VIR_FREE(nwfilters); + VIR_STEAL_PTR(ret, tmp); + return ret; error: @@ -528,8 +553,8 @@ virshNWFilterNameCompleter(vshControl *ctl, virNWFilterFree(nwfilters[i]); VIR_FREE(nwfilters); for (i = 0; i < nnwfilters; i++) - VIR_FREE(ret[i]); - VIR_FREE(ret); + VIR_FREE(tmp[i]); + VIR_FREE(tmp); return NULL; } @@ -544,6 +569,7 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, int nbindings = 0; size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); @@ -553,19 +579,21 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, if ((nbindings = virConnectListAllNWFilterBindings(priv->conn, &bindings, flags)) < 0) return NULL; - if (VIR_ALLOC_N(ret, nbindings + 1) < 0) + if (VIR_ALLOC_N(tmp, nbindings + 1) < 0) goto error; for (i = 0; i < nbindings; i++) { const char *name = virNWFilterBindingGetPortDev(bindings[i]); - if (VIR_STRDUP(ret[i], name) < 0) + if (VIR_STRDUP(tmp[i], name) < 0) goto error; virNWFilterBindingFree(bindings[i]); } VIR_FREE(bindings); + VIR_STEAL_PTR(ret, tmp); + return ret; error: @@ -573,8 +601,8 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, virNWFilterBindingFree(bindings[i]); VIR_FREE(bindings); for (i = 0; i < nbindings; i++) - VIR_FREE(ret[i]); - VIR_FREE(ret); + VIR_FREE(tmp[i]); + VIR_FREE(tmp); return NULL; } @@ -589,6 +617,7 @@ virshSecretUUIDCompleter(vshControl *ctl, int nsecrets = 0; size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); @@ -598,20 +627,22 @@ virshSecretUUIDCompleter(vshControl *ctl, if ((nsecrets = virConnectListAllSecrets(priv->conn, &secrets, flags)) < 0) return NULL; - if (VIR_ALLOC_N(ret, nsecrets + 1) < 0) + if (VIR_ALLOC_N(tmp, nsecrets + 1) < 0) goto error; for (i = 0; i < nsecrets; i++) { char uuid[VIR_UUID_STRING_BUFLEN]; if (virSecretGetUUIDString(secrets[i], uuid) < 0 || - VIR_STRDUP(ret[i], uuid) < 0) + VIR_STRDUP(tmp[i], uuid) < 0) goto error; virSecretFree(secrets[i]); } VIR_FREE(secrets); + VIR_STEAL_PTR(ret, tmp); + return ret; error: @@ -619,8 +650,8 @@ virshSecretUUIDCompleter(vshControl *ctl, virSecretFree(secrets[i]); VIR_FREE(secrets); for (i = 0; i < nsecrets; i++) - VIR_FREE(ret[i]); - VIR_FREE(ret); + VIR_FREE(tmp[i]); + VIR_FREE(tmp); return NULL; } @@ -637,6 +668,7 @@ virshSnapshotNameCompleter(vshControl *ctl, int nsnapshots = 0; size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); @@ -650,13 +682,13 @@ virshSnapshotNameCompleter(vshControl *ctl, goto error; nsnapshots = rc; - if (VIR_ALLOC_N(ret, nsnapshots + 1) < 0) + if (VIR_ALLOC_N(tmp, nsnapshots + 1) < 0) goto error; for (i = 0; i < nsnapshots; i++) { const char *name = virDomainSnapshotGetName(snapshots[i]); - if (VIR_STRDUP(ret[i], name) < 0) + if (VIR_STRDUP(tmp[i], name) < 0) goto error; virshDomainSnapshotFree(snapshots[i]); @@ -664,6 +696,8 @@ virshSnapshotNameCompleter(vshControl *ctl, VIR_FREE(snapshots); virshDomainFree(dom); + VIR_STEAL_PTR(ret, tmp); + return ret; error: @@ -671,8 +705,8 @@ virshSnapshotNameCompleter(vshControl *ctl, virshDomainSnapshotFree(snapshots[i]); VIR_FREE(snapshots); for (i = 0; i < nsnapshots; i++) - VIR_FREE(ret[i]); - VIR_FREE(ret); + VIR_FREE(tmp[i]); + VIR_FREE(tmp); virshDomainFree(dom); return NULL; } @@ -698,6 +732,7 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, char *cap_xml = NULL; char **ret = NULL; char *unit = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); @@ -724,7 +759,7 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, if (npages <= 0) goto error; - if (VIR_ALLOC_N(ret, npages + 1) < 0) + if (VIR_ALLOC_N(tmp, npages + 1) < 0) goto error; for (i = 0; i < npages; i++) { @@ -737,10 +772,12 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, if (virScaleInteger(&byteval, unit, 1024, UINT_MAX) < 0) goto error; size = vshPrettyCapacity(byteval, &suffix); - if (virAsprintf(&ret[i], "%.0f%s", size, suffix) < 0) + if (virAsprintf(&tmp[i], "%.0f%s", size, suffix) < 0) goto error; } + VIR_STEAL_PTR(ret, tmp); + cleanup: xmlXPathFreeContext(ctxt); VIR_FREE(pages); @@ -753,11 +790,11 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, return ret; error: - if (ret) { + if (tmp) { for (i = 0; i < npages; i++) - VIR_FREE(ret[i]); + VIR_FREE(tmp[i]); } - VIR_FREE(ret); + VIR_FREE(tmp); goto cleanup; } @@ -769,21 +806,24 @@ virshSecretEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, { size_t i; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); - if (VIR_ALLOC_N(ret, VIR_SECRET_EVENT_ID_LAST + 1) < 0) + if (VIR_ALLOC_N(tmp, VIR_SECRET_EVENT_ID_LAST + 1) < 0) goto error; for (i = 0; i < VIR_SECRET_EVENT_ID_LAST; i++) { - if (VIR_STRDUP(ret[i], virshSecretEventCallbacks[i].name) < 0) + if (VIR_STRDUP(tmp[i], virshSecretEventCallbacks[i].name) < 0) goto error; } + VIR_STEAL_PTR(ret, tmp); + return ret; error: - virStringListFree(ret); + virStringListFree(tmp); return NULL; } @@ -795,21 +835,24 @@ virshDomainEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, { size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); - if (VIR_ALLOC_N(ret, VIR_DOMAIN_EVENT_ID_LAST + 1) < 0) + if (VIR_ALLOC_N(tmp, VIR_DOMAIN_EVENT_ID_LAST + 1) < 0) goto error; for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) { - if (VIR_STRDUP(ret[i], virshDomainEventCallbacks[i].name) < 0) + if (VIR_STRDUP(tmp[i], virshDomainEventCallbacks[i].name) < 0) goto error; } + VIR_STEAL_PTR(ret, tmp); + return ret; error: - virStringListFree(ret); + virStringListFree(tmp); return NULL; } @@ -821,21 +864,24 @@ virshPoolEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, { size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); - if (VIR_ALLOC_N(ret, VIR_STORAGE_POOL_EVENT_ID_LAST + 1) < 0) + if (VIR_ALLOC_N(tmp, VIR_STORAGE_POOL_EVENT_ID_LAST + 1) < 0) goto error; for (i = 0; i < VIR_STORAGE_POOL_EVENT_ID_LAST; i++) { - if (VIR_STRDUP(ret[i], virshPoolEventCallbacks[i].name) < 0) + if (VIR_STRDUP(tmp[i], virshPoolEventCallbacks[i].name) < 0) goto error; } + VIR_STEAL_PTR(ret, tmp); + return ret; error: - virStringListFree(ret); + virStringListFree(tmp); return NULL; } @@ -856,6 +902,7 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, xmlNodePtr *interfaces = NULL; char *xpath = NULL; char *state = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); @@ -885,18 +932,20 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, ctxt->node = interfaces[0]; - if (VIR_ALLOC_N(ret, 2) < 0) + if (VIR_ALLOC_N(tmp, 2) < 0) goto error; if ((state = virXPathString("string(./link/@state)", ctxt)) && STREQ(state, "down")) { - if (VIR_STRDUP(ret[0], "up") < 0) + if (VIR_STRDUP(tmp[0], "up") < 0) goto error; } else { - if (VIR_STRDUP(ret[0], "down") < 0) + if (VIR_STRDUP(tmp[0], "down") < 0) goto error; } + VIR_STEAL_PTR(ret, tmp); + cleanup: VIR_FREE(state); VIR_FREE(xpath); @@ -906,8 +955,8 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, return ret; error: - virStringListFree(ret); - ret = NULL; + virStringListFree(tmp); + tmp = NULL; goto cleanup; } @@ -919,21 +968,24 @@ virshNodedevEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, { size_t i = 0; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); - if (VIR_ALLOC_N(ret, VIR_NODE_DEVICE_EVENT_ID_LAST + 1) < 0) + if (VIR_ALLOC_N(tmp, VIR_NODE_DEVICE_EVENT_ID_LAST + 1) < 0) goto error; for (i = 0; i < VIR_NODE_DEVICE_EVENT_ID_LAST; i++) { - if (VIR_STRDUP(ret[i], virshNodedevEventCallbacks[i].name) < 0) + if (VIR_STRDUP(tmp[i], virshNodedevEventCallbacks[i].name) < 0) goto error; } + VIR_STEAL_PTR(ret, tmp); + return ret; error: - virStringListFree(ret); + virStringListFree(tmp); return NULL; } @@ -951,6 +1003,7 @@ virshCellnoCompleter(vshControl *ctl, size_t i = 0; char *cap_xml = NULL; char **ret = NULL; + char **tmp = NULL; virCheckFlags(0, NULL); @@ -967,14 +1020,16 @@ virshCellnoCompleter(vshControl *ctl, if (ncells <= 0) goto error; - if (VIR_ALLOC_N(ret, ncells + 1)) + if (VIR_ALLOC_N(tmp, ncells + 1)) goto error; for (i = 0; i < ncells; i++) { - if (!(ret[i] = virXMLPropString(cells[i], "id"))) + if (!(tmp[i] = virXMLPropString(cells[i], "id"))) goto error; } + VIR_STEAL_PTR(ret, tmp); + cleanup: xmlXPathFreeContext(ctxt); VIR_FREE(cells); @@ -984,11 +1039,11 @@ virshCellnoCompleter(vshControl *ctl, return ret; error: - if (ret) { + if (tmp) { for (i = 0; i < ncells; i++) - VIR_FREE(ret[i]); + VIR_FREE(tmp[i]); } - VIR_FREE(ret); + VIR_FREE(tmp); goto cleanup; } -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:21 +0200, Ján Tomko wrote:
Construct the potential return value in an array called 'tmp' and only assign it to 'ret' if we're going to return it.
This will allow us to unify the error and success paths.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 195 +++++++++++++++++++++++++--------------- 1 file changed, 125 insertions(+), 70 deletions(-)
There's at least 1 unrelated whitespace change. ACK regardless.
Unify the cleanup paths for error and success. Now that 'ret' is only set (from tmp) on the success path, it is safe to jump right before 'return ret' after processing the error block. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 45 +++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 0750945b84..46fedc06d9 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -111,6 +111,7 @@ virshDomainNameCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: @@ -120,7 +121,7 @@ virshDomainNameCompleter(vshControl *ctl, for (i = 0; i < ndomains; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); - return NULL; + goto cleanup; } @@ -263,6 +264,7 @@ virshStoragePoolNameCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: @@ -272,7 +274,7 @@ virshStoragePoolNameCompleter(vshControl *ctl, for (i = 0; i < npools; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); - return NULL; + goto cleanup; } @@ -319,6 +321,7 @@ virshStorageVolNameCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: @@ -329,7 +332,7 @@ virshStorageVolNameCompleter(vshControl *ctl, VIR_FREE(tmp[i]); VIR_FREE(tmp); virStoragePoolFree(pool); - return NULL; + goto cleanup; } @@ -370,6 +373,7 @@ virshInterfaceNameCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: @@ -379,7 +383,7 @@ virshInterfaceNameCompleter(vshControl *ctl, for (i = 0; i < nifaces; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); - return NULL; + goto cleanup; } @@ -421,6 +425,7 @@ virshNetworkNameCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: @@ -430,7 +435,7 @@ virshNetworkNameCompleter(vshControl *ctl, for (i = 0; i < nnets; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); - return NULL; + goto cleanup; } @@ -455,11 +460,12 @@ virshNetworkEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: virStringListFree(tmp); - return NULL; + goto cleanup; } @@ -498,6 +504,7 @@ virshNodeDeviceNameCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: @@ -507,7 +514,7 @@ virshNodeDeviceNameCompleter(vshControl *ctl, for (i = 0; i < ndevs; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); - return NULL; + goto cleanup; } @@ -546,6 +553,7 @@ virshNWFilterNameCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: @@ -555,7 +563,7 @@ virshNWFilterNameCompleter(vshControl *ctl, for (i = 0; i < nnwfilters; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); - return NULL; + goto cleanup; } @@ -594,6 +602,7 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: @@ -603,7 +612,7 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, for (i = 0; i < nbindings; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); - return NULL; + goto cleanup; } @@ -643,6 +652,7 @@ virshSecretUUIDCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: @@ -652,7 +662,7 @@ virshSecretUUIDCompleter(vshControl *ctl, for (i = 0; i < nsecrets; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); - return NULL; + goto cleanup; } @@ -698,6 +708,7 @@ virshSnapshotNameCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: @@ -708,7 +719,7 @@ virshSnapshotNameCompleter(vshControl *ctl, VIR_FREE(tmp[i]); VIR_FREE(tmp); virshDomainFree(dom); - return NULL; + goto cleanup; } char ** @@ -820,11 +831,12 @@ virshSecretEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: virStringListFree(tmp); - return NULL; + goto cleanup; } @@ -849,11 +861,12 @@ virshDomainEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: virStringListFree(tmp); - return NULL; + goto cleanup; } @@ -878,11 +891,12 @@ virshPoolEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: virStringListFree(tmp); - return NULL; + goto cleanup; } @@ -982,11 +996,12 @@ virshNodedevEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); + cleanup: return ret; error: virStringListFree(tmp); - return NULL; + goto cleanup; } -- 2.20.1
Merge the cleanup of fetched items for the success and the error paths. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 97 ++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 65 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 46fedc06d9..9d56659259 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -104,20 +104,17 @@ virshDomainNameCompleter(vshControl *ctl, if (VIR_STRDUP(tmp[i], name) < 0) goto error; - - virshDomainFree(domains[i]); } - VIR_FREE(domains); VIR_STEAL_PTR(ret, tmp); cleanup: + for (i = 0; i < ndomains; i++) + virshDomainFree(domains[i]); + VIR_FREE(domains); return ret; error: - for (; i < ndomains; i++) - virshDomainFree(domains[i]); - VIR_FREE(domains); for (i = 0; i < ndomains; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); @@ -257,20 +254,17 @@ virshStoragePoolNameCompleter(vshControl *ctl, if (VIR_STRDUP(tmp[i], name) < 0) goto error; - - virStoragePoolFree(pools[i]); } - VIR_FREE(pools); VIR_STEAL_PTR(ret, tmp); cleanup: + for (i = 0; i < npools; i++) + virStoragePoolFree(pools[i]); + VIR_FREE(pools); return ret; error: - for (; i < npools; i++) - virStoragePoolFree(pools[i]); - VIR_FREE(pools); for (i = 0; i < npools; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); @@ -312,26 +306,21 @@ virshStorageVolNameCompleter(vshControl *ctl, if (VIR_STRDUP(tmp[i], name) < 0) goto error; - - virStorageVolFree(vols[i]); } - VIR_FREE(vols); - virStoragePoolFree(pool); - VIR_STEAL_PTR(ret, tmp); cleanup: + virStoragePoolFree(pool); + for (i = 0; i < nvols; i++) + virStorageVolFree(vols[i]); + VIR_FREE(vols); return ret; error: - for (; i < nvols; i++) - virStorageVolFree(vols[i]); - VIR_FREE(vols); for (i = 0; i < nvols; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); - virStoragePoolFree(pool); goto cleanup; } @@ -366,20 +355,17 @@ virshInterfaceNameCompleter(vshControl *ctl, if (VIR_STRDUP(tmp[i], name) < 0) goto error; - - virInterfaceFree(ifaces[i]); } - VIR_FREE(ifaces); VIR_STEAL_PTR(ret, tmp); cleanup: + for (i = 0; i < nifaces; i++) + virInterfaceFree(ifaces[i]); + VIR_FREE(ifaces); return ret; error: - for (; i < nifaces; i++) - virInterfaceFree(ifaces[i]); - VIR_FREE(ifaces); for (i = 0; i < nifaces; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); @@ -418,20 +404,17 @@ virshNetworkNameCompleter(vshControl *ctl, if (VIR_STRDUP(tmp[i], name) < 0) goto error; - - virNetworkFree(nets[i]); } - VIR_FREE(nets); VIR_STEAL_PTR(ret, tmp); cleanup: + for (i = 0; i < nnets; i++) + virNetworkFree(nets[i]); + VIR_FREE(nets); return ret; error: - for (; i < nnets; i++) - virNetworkFree(nets[i]); - VIR_FREE(nets); for (i = 0; i < nnets; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); @@ -497,20 +480,17 @@ virshNodeDeviceNameCompleter(vshControl *ctl, if (VIR_STRDUP(tmp[i], name) < 0) goto error; - - virNodeDeviceFree(devs[i]); } - VIR_FREE(devs); VIR_STEAL_PTR(ret, tmp); cleanup: + for (i = 0; i < ndevs; i++) + virNodeDeviceFree(devs[i]); + VIR_FREE(devs); return ret; error: - for (; i < ndevs; i++) - virNodeDeviceFree(devs[i]); - VIR_FREE(devs); for (i = 0; i < ndevs; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); @@ -546,20 +526,17 @@ virshNWFilterNameCompleter(vshControl *ctl, if (VIR_STRDUP(tmp[i], name) < 0) goto error; - - virNWFilterFree(nwfilters[i]); } - VIR_FREE(nwfilters); VIR_STEAL_PTR(ret, tmp); cleanup: + for (i = 0; i < nnwfilters; i++) + virNWFilterFree(nwfilters[i]); + VIR_FREE(nwfilters); return ret; error: - for (; i < nnwfilters; i++) - virNWFilterFree(nwfilters[i]); - VIR_FREE(nwfilters); for (i = 0; i < nnwfilters; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); @@ -595,20 +572,17 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, if (VIR_STRDUP(tmp[i], name) < 0) goto error; - - virNWFilterBindingFree(bindings[i]); } - VIR_FREE(bindings); VIR_STEAL_PTR(ret, tmp); cleanup: + for (i = 0; i < nbindings; i++) + virNWFilterBindingFree(bindings[i]); + VIR_FREE(bindings); return ret; error: - for (; i < nbindings; i++) - virNWFilterBindingFree(bindings[i]); - VIR_FREE(bindings); for (i = 0; i < nbindings; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); @@ -645,20 +619,17 @@ virshSecretUUIDCompleter(vshControl *ctl, if (virSecretGetUUIDString(secrets[i], uuid) < 0 || VIR_STRDUP(tmp[i], uuid) < 0) goto error; - - virSecretFree(secrets[i]); } - VIR_FREE(secrets); VIR_STEAL_PTR(ret, tmp); cleanup: + for (i = 0; i < nsecrets; i++) + virSecretFree(secrets[i]); + VIR_FREE(secrets); return ret; error: - for (; i < nsecrets; i++) - virSecretFree(secrets[i]); - VIR_FREE(secrets); for (i = 0; i < nsecrets; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); @@ -700,25 +671,21 @@ virshSnapshotNameCompleter(vshControl *ctl, if (VIR_STRDUP(tmp[i], name) < 0) goto error; - - virshDomainSnapshotFree(snapshots[i]); } - VIR_FREE(snapshots); - virshDomainFree(dom); VIR_STEAL_PTR(ret, tmp); cleanup: + virshDomainFree(dom); + for (i = 0; i < nsnapshots; i++) + virshDomainSnapshotFree(snapshots[i]); + VIR_FREE(snapshots); return ret; error: - for (; i < nsnapshots; i++) - virshDomainSnapshotFree(snapshots[i]); - VIR_FREE(snapshots); for (i = 0; i < nsnapshots; i++) VIR_FREE(tmp[i]); VIR_FREE(tmp); - virshDomainFree(dom); goto cleanup; } -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:23 +0200, Ján Tomko wrote:
Merge the cleanup of fetched items for the success and the error paths.
Signed-off-by: Ján Tomko <jtomko@redhat.com> ---
I'd probably squash this with the previous commit but I don't mind them being separate. ACK to both.
We've been open-coding virStringListFreeCount for cleaning up the completion list we're building. This had the advantage of zeoring the pointer afterwards, which is no longer needed now that we compile the list in 'tmp' instead of 'ret'. Since all our lists are NULL-terminated anyway, switch to using virStringListFree which has the benefit of being usable with our VIR_AUTOSTRINGLIST macro. Fixes nearly impossible NULL dereferences in virshNWFilterBindingNameCompleter virshNWFilterNameCompleter virshNodeDeviceNameCompleter virshNetworkNameCompleter virshInterfaceNameCompleter virshStoragePoolNameCompleter virshDomainNameCompleter which jumped on the error label after a failed allocation and a possible one in virshStorageVolNameCompleter which jumped there when we fail to fetch the list of volumes. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 67 +++++++++++------------------------------ 1 file changed, 18 insertions(+), 49 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 9d56659259..20b325c020 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -112,12 +112,10 @@ virshDomainNameCompleter(vshControl *ctl, for (i = 0; i < ndomains; i++) virshDomainFree(domains[i]); VIR_FREE(domains); + virStringListFree(tmp); return ret; error: - for (i = 0; i < ndomains; i++) - VIR_FREE(tmp[i]); - VIR_FREE(tmp); goto cleanup; } @@ -262,12 +260,10 @@ virshStoragePoolNameCompleter(vshControl *ctl, for (i = 0; i < npools; i++) virStoragePoolFree(pools[i]); VIR_FREE(pools); + virStringListFree(tmp); return ret; error: - for (i = 0; i < npools; i++) - VIR_FREE(tmp[i]); - VIR_FREE(tmp); goto cleanup; } @@ -315,12 +311,10 @@ virshStorageVolNameCompleter(vshControl *ctl, for (i = 0; i < nvols; i++) virStorageVolFree(vols[i]); VIR_FREE(vols); + virStringListFree(tmp); return ret; error: - for (i = 0; i < nvols; i++) - VIR_FREE(tmp[i]); - VIR_FREE(tmp); goto cleanup; } @@ -363,12 +357,10 @@ virshInterfaceNameCompleter(vshControl *ctl, for (i = 0; i < nifaces; i++) virInterfaceFree(ifaces[i]); VIR_FREE(ifaces); + virStringListFree(tmp); return ret; error: - for (i = 0; i < nifaces; i++) - VIR_FREE(tmp[i]); - VIR_FREE(tmp); goto cleanup; } @@ -412,12 +404,10 @@ virshNetworkNameCompleter(vshControl *ctl, for (i = 0; i < nnets; i++) virNetworkFree(nets[i]); VIR_FREE(nets); + virStringListFree(tmp); return ret; error: - for (i = 0; i < nnets; i++) - VIR_FREE(tmp[i]); - VIR_FREE(tmp); goto cleanup; } @@ -444,10 +434,10 @@ virshNetworkEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); cleanup: + virStringListFree(tmp); return ret; error: - virStringListFree(tmp); goto cleanup; } @@ -488,12 +478,10 @@ virshNodeDeviceNameCompleter(vshControl *ctl, for (i = 0; i < ndevs; i++) virNodeDeviceFree(devs[i]); VIR_FREE(devs); + virStringListFree(tmp); return ret; error: - for (i = 0; i < ndevs; i++) - VIR_FREE(tmp[i]); - VIR_FREE(tmp); goto cleanup; } @@ -534,12 +522,10 @@ virshNWFilterNameCompleter(vshControl *ctl, for (i = 0; i < nnwfilters; i++) virNWFilterFree(nwfilters[i]); VIR_FREE(nwfilters); + virStringListFree(tmp); return ret; error: - for (i = 0; i < nnwfilters; i++) - VIR_FREE(tmp[i]); - VIR_FREE(tmp); goto cleanup; } @@ -580,12 +566,10 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, for (i = 0; i < nbindings; i++) virNWFilterBindingFree(bindings[i]); VIR_FREE(bindings); + virStringListFree(tmp); return ret; error: - for (i = 0; i < nbindings; i++) - VIR_FREE(tmp[i]); - VIR_FREE(tmp); goto cleanup; } @@ -627,12 +611,10 @@ virshSecretUUIDCompleter(vshControl *ctl, for (i = 0; i < nsecrets; i++) virSecretFree(secrets[i]); VIR_FREE(secrets); + virStringListFree(tmp); return ret; error: - for (i = 0; i < nsecrets; i++) - VIR_FREE(tmp[i]); - VIR_FREE(tmp); goto cleanup; } @@ -680,12 +662,10 @@ virshSnapshotNameCompleter(vshControl *ctl, for (i = 0; i < nsnapshots; i++) virshDomainSnapshotFree(snapshots[i]); VIR_FREE(snapshots); + virStringListFree(tmp); return ret; error: - for (i = 0; i < nsnapshots; i++) - VIR_FREE(tmp[i]); - VIR_FREE(tmp); goto cleanup; } @@ -764,15 +744,10 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, VIR_FREE(pagesize); VIR_FREE(cap_xml); VIR_FREE(unit); - + virStringListFree(tmp); return ret; error: - if (tmp) { - for (i = 0; i < npages; i++) - VIR_FREE(tmp[i]); - } - VIR_FREE(tmp); goto cleanup; } @@ -799,10 +774,10 @@ virshSecretEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); cleanup: + virStringListFree(tmp); return ret; error: - virStringListFree(tmp); goto cleanup; } @@ -829,10 +804,10 @@ virshDomainEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); cleanup: + virStringListFree(tmp); return ret; error: - virStringListFree(tmp); goto cleanup; } @@ -859,10 +834,10 @@ virshPoolEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); cleanup: + virStringListFree(tmp); return ret; error: - virStringListFree(tmp); goto cleanup; } @@ -933,11 +908,10 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, VIR_FREE(interfaces); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); + virStringListFree(tmp); return ret; error: - virStringListFree(tmp); - tmp = NULL; goto cleanup; } @@ -964,10 +938,10 @@ virshNodedevEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); cleanup: + virStringListFree(tmp); return ret; error: - virStringListFree(tmp); goto cleanup; } @@ -1017,15 +991,10 @@ virshCellnoCompleter(vshControl *ctl, VIR_FREE(cells); xmlFreeDoc(doc); VIR_FREE(cap_xml); - + virStringListFree(tmp); return ret; error: - if (tmp) { - for (i = 0; i < ncells; i++) - VIR_FREE(tmp[i]); - } - VIR_FREE(tmp); goto cleanup; } -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:24 +0200, Ján Tomko wrote:
We've been open-coding virStringListFreeCount for cleaning up the completion list we're building. This had the advantage of zeoring the pointer afterwards, which is no longer needed now that we compile the list in 'tmp' instead of 'ret'.
But now you are not preserving the holy NULLs on the stack!!.
Since all our lists are NULL-terminated anyway, switch to using virStringListFree which has the benefit of being usable with our VIR_AUTOSTRINGLIST macro.
Fixes nearly impossible NULL dereferences in virshNWFilterBindingNameCompleter virshNWFilterNameCompleter virshNodeDeviceNameCompleter virshNetworkNameCompleter virshInterfaceNameCompleter virshStoragePoolNameCompleter virshDomainNameCompleter which jumped on the error label after a failed allocation and a possible one in virshStorageVolNameCompleter which jumped there when we fail to fetch the list of volumes.
Signed-off-by: Ján Tomko <jtomko@redhat.com> ---
ACK
Now that every function uses virStringListFree, we can use the VIR_AUTOSTRINGLIST to have the compiler call this function automatically when 'tmp' goes out of scope. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 63 ++++++++++++++--------------------------- 1 file changed, 21 insertions(+), 42 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 20b325c020..669eda0f84 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -79,7 +79,7 @@ virshDomainNameCompleter(vshControl *ctl, int ndomains = 0; size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE | VIR_CONNECT_LIST_DOMAINS_INACTIVE | @@ -112,7 +112,6 @@ virshDomainNameCompleter(vshControl *ctl, for (i = 0; i < ndomains; i++) virshDomainFree(domains[i]); VIR_FREE(domains); - virStringListFree(tmp); return ret; error: @@ -133,7 +132,7 @@ virshDomainInterfaceCompleter(vshControl *ctl, size_t i; unsigned int domainXMLFlags = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, NULL); @@ -171,7 +170,6 @@ virshDomainInterfaceCompleter(vshControl *ctl, VIR_FREE(interfaces); xmlFreeDoc(xmldoc); xmlXPathFreeContext(ctxt); - virStringListFree(tmp); return ret; } @@ -187,7 +185,7 @@ virshDomainDiskTargetCompleter(vshControl *ctl, xmlNodePtr *disks = NULL; int ndisks; size_t i; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; char **ret = NULL; virCheckFlags(0, NULL); @@ -216,7 +214,6 @@ virshDomainDiskTargetCompleter(vshControl *ctl, VIR_FREE(disks); xmlFreeDoc(xmldoc); xmlXPathFreeContext(ctxt); - virStringListFree(tmp); return ret; } @@ -231,7 +228,7 @@ virshStoragePoolNameCompleter(vshControl *ctl, int npools = 0; size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(VIR_CONNECT_LIST_STORAGE_POOLS_INACTIVE | VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE | @@ -260,7 +257,6 @@ virshStoragePoolNameCompleter(vshControl *ctl, for (i = 0; i < npools; i++) virStoragePoolFree(pools[i]); VIR_FREE(pools); - virStringListFree(tmp); return ret; error: @@ -280,7 +276,7 @@ virshStorageVolNameCompleter(vshControl *ctl, int nvols = 0; size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -311,7 +307,6 @@ virshStorageVolNameCompleter(vshControl *ctl, for (i = 0; i < nvols; i++) virStorageVolFree(vols[i]); VIR_FREE(vols); - virStringListFree(tmp); return ret; error: @@ -329,7 +324,7 @@ virshInterfaceNameCompleter(vshControl *ctl, int nifaces = 0; size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(VIR_CONNECT_LIST_INTERFACES_ACTIVE | VIR_CONNECT_LIST_INTERFACES_INACTIVE, @@ -357,7 +352,6 @@ virshInterfaceNameCompleter(vshControl *ctl, for (i = 0; i < nifaces; i++) virInterfaceFree(ifaces[i]); VIR_FREE(ifaces); - virStringListFree(tmp); return ret; error: @@ -375,7 +369,7 @@ virshNetworkNameCompleter(vshControl *ctl, int nnets = 0; size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(VIR_CONNECT_LIST_NETWORKS_INACTIVE | VIR_CONNECT_LIST_NETWORKS_ACTIVE | @@ -404,7 +398,6 @@ virshNetworkNameCompleter(vshControl *ctl, for (i = 0; i < nnets; i++) virNetworkFree(nets[i]); VIR_FREE(nets); - virStringListFree(tmp); return ret; error: @@ -419,7 +412,7 @@ virshNetworkEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, { size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -434,7 +427,6 @@ virshNetworkEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); cleanup: - virStringListFree(tmp); return ret; error: @@ -452,7 +444,7 @@ virshNodeDeviceNameCompleter(vshControl *ctl, int ndevs = 0; size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -478,7 +470,6 @@ virshNodeDeviceNameCompleter(vshControl *ctl, for (i = 0; i < ndevs; i++) virNodeDeviceFree(devs[i]); VIR_FREE(devs); - virStringListFree(tmp); return ret; error: @@ -496,7 +487,7 @@ virshNWFilterNameCompleter(vshControl *ctl, int nnwfilters = 0; size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -522,7 +513,6 @@ virshNWFilterNameCompleter(vshControl *ctl, for (i = 0; i < nnwfilters; i++) virNWFilterFree(nwfilters[i]); VIR_FREE(nwfilters); - virStringListFree(tmp); return ret; error: @@ -540,7 +530,7 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, int nbindings = 0; size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -566,7 +556,6 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, for (i = 0; i < nbindings; i++) virNWFilterBindingFree(bindings[i]); VIR_FREE(bindings); - virStringListFree(tmp); return ret; error: @@ -584,7 +573,7 @@ virshSecretUUIDCompleter(vshControl *ctl, int nsecrets = 0; size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -611,7 +600,6 @@ virshSecretUUIDCompleter(vshControl *ctl, for (i = 0; i < nsecrets; i++) virSecretFree(secrets[i]); VIR_FREE(secrets); - virStringListFree(tmp); return ret; error: @@ -631,7 +619,7 @@ virshSnapshotNameCompleter(vshControl *ctl, int nsnapshots = 0; size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -662,7 +650,6 @@ virshSnapshotNameCompleter(vshControl *ctl, for (i = 0; i < nsnapshots; i++) virshDomainSnapshotFree(snapshots[i]); VIR_FREE(snapshots); - virStringListFree(tmp); return ret; error: @@ -690,7 +677,7 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, char *cap_xml = NULL; char **ret = NULL; char *unit = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -744,7 +731,6 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, VIR_FREE(pagesize); VIR_FREE(cap_xml); VIR_FREE(unit); - virStringListFree(tmp); return ret; error: @@ -759,7 +745,7 @@ virshSecretEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, { size_t i; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -774,7 +760,6 @@ virshSecretEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); cleanup: - virStringListFree(tmp); return ret; error: @@ -789,7 +774,7 @@ virshDomainEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, { size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -804,7 +789,6 @@ virshDomainEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); cleanup: - virStringListFree(tmp); return ret; error: @@ -819,7 +803,7 @@ virshPoolEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, { size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -834,7 +818,6 @@ virshPoolEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); cleanup: - virStringListFree(tmp); return ret; error: @@ -858,7 +841,7 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, xmlNodePtr *interfaces = NULL; char *xpath = NULL; char *state = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -908,7 +891,6 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, VIR_FREE(interfaces); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); - virStringListFree(tmp); return ret; error: @@ -923,7 +905,7 @@ virshNodedevEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, { size_t i = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -938,7 +920,6 @@ virshNodedevEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, VIR_STEAL_PTR(ret, tmp); cleanup: - virStringListFree(tmp); return ret; error: @@ -959,7 +940,7 @@ virshCellnoCompleter(vshControl *ctl, size_t i = 0; char *cap_xml = NULL; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -991,7 +972,6 @@ virshCellnoCompleter(vshControl *ctl, VIR_FREE(cells); xmlFreeDoc(doc); VIR_FREE(cap_xml); - virStringListFree(tmp); return ret; error: @@ -1012,7 +992,7 @@ virshDomainDeviceAliasCompleter(vshControl *ctl, size_t i; unsigned int domainXMLFlags = 0; char **ret = NULL; - char **tmp = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -1042,6 +1022,5 @@ virshDomainDeviceAliasCompleter(vshControl *ctl, VIR_FREE(aliases); xmlFreeDoc(xmldoc); xmlXPathFreeContext(ctxt); - virStringListFree(tmp); return ret; } -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:25 +0200, Ján Tomko wrote:
Now that every function uses virStringListFree, we can use the VIR_AUTOSTRINGLIST to have the compiler call this function automatically when 'tmp' goes out of scope.
Again something that could be done in previous patch directly. You are adding a bunch of calls to virStringListFree which vanish here. ACK but really consider squashing this one.
On Wed, Apr 03, 2019 at 09:55:33AM +0200, Peter Krempa wrote:
On Mon, Apr 01, 2019 at 09:33:25 +0200, Ján Tomko wrote:
Now that every function uses virStringListFree, we can use the VIR_AUTOSTRINGLIST to have the compiler call this function automatically when 'tmp' goes out of scope.
Again something that could be done in previous patch directly. You are adding a bunch of calls to virStringListFree which vanish here.
ACK but really consider squashing this one.
Squashed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 669eda0f84..3d107f0504 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -128,7 +128,7 @@ virshDomainInterfaceCompleter(vshControl *ctl, xmlDocPtr xmldoc = NULL; xmlXPathContextPtr ctxt = NULL; int ninterfaces; - xmlNodePtr *interfaces = NULL; + VIR_AUTOFREE(xmlNodePtr *) interfaces = NULL; size_t i; unsigned int domainXMLFlags = 0; char **ret = NULL; @@ -167,7 +167,6 @@ virshDomainInterfaceCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - VIR_FREE(interfaces); xmlFreeDoc(xmldoc); xmlXPathFreeContext(ctxt); return ret; @@ -182,7 +181,7 @@ virshDomainDiskTargetCompleter(vshControl *ctl, virshControlPtr priv = ctl->privData; xmlDocPtr xmldoc = NULL; xmlXPathContextPtr ctxt = NULL; - xmlNodePtr *disks = NULL; + VIR_AUTOFREE(xmlNodePtr *) disks = NULL; int ndisks; size_t i; VIR_AUTOSTRINGLIST tmp = NULL; @@ -211,7 +210,6 @@ virshDomainDiskTargetCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - VIR_FREE(disks); xmlFreeDoc(xmldoc); xmlXPathFreeContext(ctxt); return ret; @@ -665,7 +663,7 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, xmlXPathContextPtr ctxt = NULL; virshControlPtr priv = ctl->privData; unsigned int npages = 0; - xmlNodePtr *pages = NULL; + VIR_AUTOFREE(xmlNodePtr *) pages = NULL; xmlDocPtr doc = NULL; double size = 0; size_t i = 0; @@ -725,7 +723,6 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, cleanup: xmlXPathFreeContext(ctxt); - VIR_FREE(pages); xmlFreeDoc(doc); VIR_FREE(path); VIR_FREE(pagesize); @@ -838,7 +835,7 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, virMacAddr macaddr; char macstr[VIR_MAC_STRING_BUFLEN] = ""; int ninterfaces; - xmlNodePtr *interfaces = NULL; + VIR_AUTOFREE(xmlNodePtr *) interfaces = NULL; char *xpath = NULL; char *state = NULL; VIR_AUTOSTRINGLIST tmp = NULL; @@ -888,7 +885,6 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, cleanup: VIR_FREE(state); VIR_FREE(xpath); - VIR_FREE(interfaces); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); return ret; @@ -935,7 +931,7 @@ virshCellnoCompleter(vshControl *ctl, xmlXPathContextPtr ctxt = NULL; virshControlPtr priv = ctl->privData; unsigned int ncells = 0; - xmlNodePtr *cells = NULL; + VIR_AUTOFREE(xmlNodePtr *) cells = NULL; xmlDocPtr doc = NULL; size_t i = 0; char *cap_xml = NULL; @@ -969,7 +965,6 @@ virshCellnoCompleter(vshControl *ctl, cleanup: xmlXPathFreeContext(ctxt); - VIR_FREE(cells); xmlFreeDoc(doc); VIR_FREE(cap_xml); return ret; @@ -988,7 +983,7 @@ virshDomainDeviceAliasCompleter(vshControl *ctl, xmlDocPtr xmldoc = NULL; xmlXPathContextPtr ctxt = NULL; int naliases; - xmlNodePtr *aliases = NULL; + VIR_AUTOFREE(xmlNodePtr *) aliases = NULL; size_t i; unsigned int domainXMLFlags = 0; char **ret = NULL; @@ -1019,7 +1014,6 @@ virshDomainDeviceAliasCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - VIR_FREE(aliases); xmlFreeDoc(xmldoc); xmlXPathFreeContext(ctxt); return ret; -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:26 +0200, Ján Tomko wrote: Add 'virsh-completer:' into the subject. ACK
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 3d107f0504..dc4fe2b83a 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -125,8 +125,8 @@ virshDomainInterfaceCompleter(vshControl *ctl, unsigned int flags) { virshControlPtr priv = ctl->privData; - xmlDocPtr xmldoc = NULL; - xmlXPathContextPtr ctxt = NULL; + VIR_AUTOPTR(xmlDoc) xmldoc = NULL; + VIR_AUTOPTR(xmlXPathContext) ctxt = NULL; int ninterfaces; VIR_AUTOFREE(xmlNodePtr *) interfaces = NULL; size_t i; @@ -167,8 +167,6 @@ virshDomainInterfaceCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - xmlFreeDoc(xmldoc); - xmlXPathFreeContext(ctxt); return ret; } @@ -179,8 +177,8 @@ virshDomainDiskTargetCompleter(vshControl *ctl, unsigned int flags) { virshControlPtr priv = ctl->privData; - xmlDocPtr xmldoc = NULL; - xmlXPathContextPtr ctxt = NULL; + VIR_AUTOPTR(xmlDoc) xmldoc = NULL; + VIR_AUTOPTR(xmlXPathContext) ctxt = NULL; VIR_AUTOFREE(xmlNodePtr *) disks = NULL; int ndisks; size_t i; @@ -210,8 +208,6 @@ virshDomainDiskTargetCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - xmlFreeDoc(xmldoc); - xmlXPathFreeContext(ctxt); return ret; } @@ -660,11 +656,11 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, unsigned int flags) { unsigned long long byteval = 0; - xmlXPathContextPtr ctxt = NULL; + VIR_AUTOPTR(xmlXPathContext) ctxt = NULL; virshControlPtr priv = ctl->privData; unsigned int npages = 0; VIR_AUTOFREE(xmlNodePtr *) pages = NULL; - xmlDocPtr doc = NULL; + VIR_AUTOPTR(xmlDoc) doc = NULL; double size = 0; size_t i = 0; const char *suffix = NULL; @@ -722,8 +718,6 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - xmlXPathFreeContext(ctxt); - xmlFreeDoc(doc); VIR_FREE(path); VIR_FREE(pagesize); VIR_FREE(cap_xml); @@ -830,8 +824,8 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, virshControlPtr priv = ctl->privData; const char *iface = NULL; char **ret = NULL; - xmlDocPtr xml = NULL; - xmlXPathContextPtr ctxt = NULL; + VIR_AUTOPTR(xmlDoc) xml = NULL; + VIR_AUTOPTR(xmlXPathContext) ctxt = NULL; virMacAddr macaddr; char macstr[VIR_MAC_STRING_BUFLEN] = ""; int ninterfaces; @@ -885,8 +879,6 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, cleanup: VIR_FREE(state); VIR_FREE(xpath); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(xml); return ret; error: @@ -928,11 +920,11 @@ virshCellnoCompleter(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED, unsigned int flags) { - xmlXPathContextPtr ctxt = NULL; + VIR_AUTOPTR(xmlXPathContext) ctxt = NULL; virshControlPtr priv = ctl->privData; unsigned int ncells = 0; VIR_AUTOFREE(xmlNodePtr *) cells = NULL; - xmlDocPtr doc = NULL; + VIR_AUTOPTR(xmlDoc) doc = NULL; size_t i = 0; char *cap_xml = NULL; char **ret = NULL; @@ -964,8 +956,6 @@ virshCellnoCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - xmlXPathFreeContext(ctxt); - xmlFreeDoc(doc); VIR_FREE(cap_xml); return ret; @@ -980,8 +970,8 @@ virshDomainDeviceAliasCompleter(vshControl *ctl, unsigned int flags) { virshControlPtr priv = ctl->privData; - xmlDocPtr xmldoc = NULL; - xmlXPathContextPtr ctxt = NULL; + VIR_AUTOPTR(xmlDoc) xmldoc = NULL; + VIR_AUTOPTR(xmlXPathContext) ctxt = NULL; int naliases; VIR_AUTOFREE(xmlNodePtr *) aliases = NULL; size_t i; @@ -1014,7 +1004,5 @@ virshDomainDeviceAliasCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - xmlFreeDoc(xmldoc); - xmlXPathFreeContext(ctxt); return ret; } -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:27 +0200, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-)
ACK
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index dc4fe2b83a..025eee19e2 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -666,11 +666,11 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, const char *suffix = NULL; const char *cellnum = NULL; bool cellno = vshCommandOptBool(cmd, "cellno"); - char *path = NULL; - char *pagesize = NULL; - char *cap_xml = NULL; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOFREE(char *) pagesize = NULL; + VIR_AUTOFREE(char *) cap_xml = NULL; char **ret = NULL; - char *unit = NULL; + VIR_AUTOFREE(char *) unit = NULL; VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -718,10 +718,6 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - VIR_FREE(path); - VIR_FREE(pagesize); - VIR_FREE(cap_xml); - VIR_FREE(unit); return ret; error: @@ -830,8 +826,8 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, char macstr[VIR_MAC_STRING_BUFLEN] = ""; int ninterfaces; VIR_AUTOFREE(xmlNodePtr *) interfaces = NULL; - char *xpath = NULL; - char *state = NULL; + VIR_AUTOFREE(char *) xpath = NULL; + VIR_AUTOFREE(char *) state = NULL; VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -877,8 +873,6 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - VIR_FREE(state); - VIR_FREE(xpath); return ret; error: @@ -926,7 +920,7 @@ virshCellnoCompleter(vshControl *ctl, VIR_AUTOFREE(xmlNodePtr *) cells = NULL; VIR_AUTOPTR(xmlDoc) doc = NULL; size_t i = 0; - char *cap_xml = NULL; + VIR_AUTOFREE(char *) cap_xml = NULL; char **ret = NULL; VIR_AUTOSTRINGLIST tmp = NULL; @@ -956,7 +950,6 @@ virshCellnoCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); cleanup: - VIR_FREE(cap_xml); return ret; error: -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:28 +0200, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
ACK
Now that we have a shared cleanup section everywhere, delete all the 'error' labels which all contain just 'goto cleanup' anyway. Also remove all the 'cleanup' labels that only 'return ret' - we can simply return NULL instead of jumping to that label. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 207 +++++++++++++--------------------------- 1 file changed, 68 insertions(+), 139 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 025eee19e2..2b9749300a 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -97,13 +97,13 @@ virshDomainNameCompleter(vshControl *ctl, return NULL; if (VIR_ALLOC_N(tmp, ndomains + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < ndomains; i++) { const char *name = virDomainGetName(domains[i]); if (VIR_STRDUP(tmp[i], name) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); @@ -113,9 +113,6 @@ virshDomainNameCompleter(vshControl *ctl, virshDomainFree(domains[i]); VIR_FREE(domains); return ret; - - error: - goto cleanup; } @@ -143,14 +140,14 @@ virshDomainInterfaceCompleter(vshControl *ctl, domainXMLFlags = VIR_DOMAIN_XML_INACTIVE; if (virshDomainGetXML(ctl, cmd, domainXMLFlags, &xmldoc, &ctxt) < 0) - goto cleanup; + return NULL; ninterfaces = virXPathNodeSet("./devices/interface", ctxt, &interfaces); if (ninterfaces < 0) - goto cleanup; + return NULL; if (VIR_ALLOC_N(tmp, ninterfaces + 1) < 0) - goto cleanup; + return NULL; for (i = 0; i < ninterfaces; i++) { ctxt->node = interfaces[i]; @@ -162,11 +159,10 @@ virshDomainInterfaceCompleter(vshControl *ctl, /* In case we are dealing with inactive domain XML there's no * <target dev=''/>. Offer MAC addresses then. */ if (!(tmp[i] = virXPathString("string(./mac/@address)", ctxt))) - goto cleanup; + return NULL; } VIR_STEAL_PTR(ret, tmp); - cleanup: return ret; } @@ -191,23 +187,22 @@ virshDomainDiskTargetCompleter(vshControl *ctl, return NULL; if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0) - goto cleanup; + return NULL; ndisks = virXPathNodeSet("./devices/disk", ctxt, &disks); if (ndisks < 0) - goto cleanup; + return NULL; if (VIR_ALLOC_N(tmp, ndisks + 1) < 0) - goto cleanup; + return NULL; for (i = 0; i < ndisks; i++) { ctxt->node = disks[i]; if (!(tmp[i] = virXPathString("string(./target/@dev)", ctxt))) - goto cleanup; + return NULL; } VIR_STEAL_PTR(ret, tmp); - cleanup: return ret; } @@ -236,13 +231,13 @@ virshStoragePoolNameCompleter(vshControl *ctl, return NULL; if (VIR_ALLOC_N(tmp, npools + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < npools; i++) { const char *name = virStoragePoolGetName(pools[i]); if (VIR_STRDUP(tmp[i], name) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); @@ -252,9 +247,6 @@ virshStoragePoolNameCompleter(vshControl *ctl, virStoragePoolFree(pools[i]); VIR_FREE(pools); return ret; - - error: - goto cleanup; } @@ -281,17 +273,17 @@ virshStorageVolNameCompleter(vshControl *ctl, return NULL; if ((rc = virStoragePoolListAllVolumes(pool, &vols, flags)) < 0) - goto error; + goto cleanup; nvols = rc; if (VIR_ALLOC_N(tmp, nvols + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < nvols; i++) { const char *name = virStorageVolGetName(vols[i]); if (VIR_STRDUP(tmp[i], name) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); @@ -302,9 +294,6 @@ virshStorageVolNameCompleter(vshControl *ctl, virStorageVolFree(vols[i]); VIR_FREE(vols); return ret; - - error: - goto cleanup; } @@ -331,13 +320,13 @@ virshInterfaceNameCompleter(vshControl *ctl, return NULL; if (VIR_ALLOC_N(tmp, nifaces + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < nifaces; i++) { const char *name = virInterfaceGetName(ifaces[i]); if (VIR_STRDUP(tmp[i], name) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); @@ -347,9 +336,6 @@ virshInterfaceNameCompleter(vshControl *ctl, virInterfaceFree(ifaces[i]); VIR_FREE(ifaces); return ret; - - error: - goto cleanup; } @@ -377,13 +363,13 @@ virshNetworkNameCompleter(vshControl *ctl, return NULL; if (VIR_ALLOC_N(tmp, nnets + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < nnets; i++) { const char *name = virNetworkGetName(nets[i]); if (VIR_STRDUP(tmp[i], name) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); @@ -393,9 +379,6 @@ virshNetworkNameCompleter(vshControl *ctl, virNetworkFree(nets[i]); VIR_FREE(nets); return ret; - - error: - goto cleanup; } @@ -411,20 +394,17 @@ virshNetworkEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, virCheckFlags(0, NULL); if (VIR_ALLOC_N(tmp, VIR_NETWORK_EVENT_ID_LAST + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < VIR_NETWORK_EVENT_ID_LAST; i++) { if (VIR_STRDUP(tmp[i], virshNetworkEventCallbacks[i].name) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); cleanup: return ret; - - error: - goto cleanup; } @@ -449,13 +429,13 @@ virshNodeDeviceNameCompleter(vshControl *ctl, return NULL; if (VIR_ALLOC_N(tmp, ndevs + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < ndevs; i++) { const char *name = virNodeDeviceGetName(devs[i]); if (VIR_STRDUP(tmp[i], name) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); @@ -465,9 +445,6 @@ virshNodeDeviceNameCompleter(vshControl *ctl, virNodeDeviceFree(devs[i]); VIR_FREE(devs); return ret; - - error: - goto cleanup; } @@ -492,13 +469,13 @@ virshNWFilterNameCompleter(vshControl *ctl, return NULL; if (VIR_ALLOC_N(tmp, nnwfilters + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < nnwfilters; i++) { const char *name = virNWFilterGetName(nwfilters[i]); if (VIR_STRDUP(tmp[i], name) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); @@ -508,9 +485,6 @@ virshNWFilterNameCompleter(vshControl *ctl, virNWFilterFree(nwfilters[i]); VIR_FREE(nwfilters); return ret; - - error: - goto cleanup; } @@ -535,13 +509,13 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, return NULL; if (VIR_ALLOC_N(tmp, nbindings + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < nbindings; i++) { const char *name = virNWFilterBindingGetPortDev(bindings[i]); if (VIR_STRDUP(tmp[i], name) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); @@ -551,9 +525,6 @@ virshNWFilterBindingNameCompleter(vshControl *ctl, virNWFilterBindingFree(bindings[i]); VIR_FREE(bindings); return ret; - - error: - goto cleanup; } @@ -578,14 +549,14 @@ virshSecretUUIDCompleter(vshControl *ctl, return NULL; if (VIR_ALLOC_N(tmp, nsecrets + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < nsecrets; i++) { char uuid[VIR_UUID_STRING_BUFLEN]; if (virSecretGetUUIDString(secrets[i], uuid) < 0 || VIR_STRDUP(tmp[i], uuid) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); @@ -595,9 +566,6 @@ virshSecretUUIDCompleter(vshControl *ctl, virSecretFree(secrets[i]); VIR_FREE(secrets); return ret; - - error: - goto cleanup; } @@ -624,17 +592,17 @@ virshSnapshotNameCompleter(vshControl *ctl, return NULL; if ((rc = virDomainListAllSnapshots(dom, &snapshots, flags)) < 0) - goto error; + goto cleanup; nsnapshots = rc; if (VIR_ALLOC_N(tmp, nsnapshots + 1) < 0) - goto error; + goto cleanup; for (i = 0; i < nsnapshots; i++) { const char *name = virDomainSnapshotGetName(snapshots[i]); if (VIR_STRDUP(tmp[i], name) < 0) - goto error; + goto cleanup; } VIR_STEAL_PTR(ret, tmp); @@ -645,9 +613,6 @@ virshSnapshotNameCompleter(vshControl *ctl, virshDomainSnapshotFree(snapshots[i]); VIR_FREE(snapshots); return ret; - - error: - goto cleanup; } char ** @@ -676,30 +641,30 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, virCheckFlags(0, NULL); if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) - goto error; + return NULL; if (!(cap_xml = virConnectGetCapabilities(priv->conn))) - goto error; + return NULL; if (!(doc = virXMLParseStringCtxt(cap_xml, _("capabilities"), &ctxt))) - goto error; + return NULL; if (cellno && vshCommandOptStringQuiet(ctl, cmd, "cellno", &cellnum) > 0) { if (virAsprintf(&path, "/capabilities/host/topology/cells/cell[@id=\"%s\"]/pages", cellnum) < 0) - goto error; + return NULL; } else { if (virAsprintf(&path, "/capabilities/host/cpu/pages") < 0) - goto error; + return NULL; } npages = virXPathNodeSet(path, ctxt, &pages); if (npages <= 0) - goto error; + return NULL; if (VIR_ALLOC_N(tmp, npages + 1) < 0) - goto error; + return NULL; for (i = 0; i < npages; i++) { VIR_FREE(pagesize); @@ -707,21 +672,16 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, pagesize = virXMLPropString(pages[i], "size"); unit = virXMLPropString(pages[i], "unit"); if (virStrToLong_ull(pagesize, NULL, 10, &byteval) < 0) - goto error; + return NULL; if (virScaleInteger(&byteval, unit, 1024, UINT_MAX) < 0) - goto error; + return NULL; size = vshPrettyCapacity(byteval, &suffix); if (virAsprintf(&tmp[i], "%.0f%s", size, suffix) < 0) - goto error; + return NULL; } VIR_STEAL_PTR(ret, tmp); - - cleanup: return ret; - - error: - goto cleanup; } @@ -737,20 +697,15 @@ virshSecretEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, virCheckFlags(0, NULL); if (VIR_ALLOC_N(tmp, VIR_SECRET_EVENT_ID_LAST + 1) < 0) - goto error; + return NULL; for (i = 0; i < VIR_SECRET_EVENT_ID_LAST; i++) { if (VIR_STRDUP(tmp[i], virshSecretEventCallbacks[i].name) < 0) - goto error; + return NULL; } VIR_STEAL_PTR(ret, tmp); - - cleanup: return ret; - - error: - goto cleanup; } @@ -766,20 +721,15 @@ virshDomainEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, virCheckFlags(0, NULL); if (VIR_ALLOC_N(tmp, VIR_DOMAIN_EVENT_ID_LAST + 1) < 0) - goto error; + return NULL; for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) { if (VIR_STRDUP(tmp[i], virshDomainEventCallbacks[i].name) < 0) - goto error; + return NULL; } VIR_STEAL_PTR(ret, tmp); - - cleanup: return ret; - - error: - goto cleanup; } @@ -795,20 +745,15 @@ virshPoolEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, virCheckFlags(0, NULL); if (VIR_ALLOC_N(tmp, VIR_STORAGE_POOL_EVENT_ID_LAST + 1) < 0) - goto error; + return NULL; for (i = 0; i < VIR_STORAGE_POOL_EVENT_ID_LAST; i++) { if (VIR_STRDUP(tmp[i], virshPoolEventCallbacks[i].name) < 0) - goto error; + return NULL; } VIR_STEAL_PTR(ret, tmp); - - cleanup: return ret; - - error: - goto cleanup; } @@ -836,10 +781,10 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, return NULL; if (virshDomainGetXML(ctl, cmd, flags, &xml, &ctxt) < 0) - goto cleanup; + return NULL; if (vshCommandOptStringReq(ctl, cmd, "interface", &iface) < 0) - goto cleanup; + return NULL; /* normalize the mac addr */ if (virMacAddrParse(iface, &macaddr) == 0) @@ -848,35 +793,30 @@ virshDomainInterfaceStateCompleter(vshControl *ctl, if (virAsprintf(&xpath, "/domain/devices/interface[(mac/@address = '%s') or " " (target/@dev = '%s')]", macstr, iface) < 0) - goto cleanup; + return NULL; if ((ninterfaces = virXPathNodeSet(xpath, ctxt, &interfaces)) < 0) - goto cleanup; + return NULL; if (ninterfaces != 1) - goto cleanup; + return NULL; ctxt->node = interfaces[0]; if (VIR_ALLOC_N(tmp, 2) < 0) - goto error; + return NULL; if ((state = virXPathString("string(./link/@state)", ctxt)) && STREQ(state, "down")) { if (VIR_STRDUP(tmp[0], "up") < 0) - goto error; + return NULL; } else { if (VIR_STRDUP(tmp[0], "down") < 0) - goto error; + return NULL; } VIR_STEAL_PTR(ret, tmp); - - cleanup: return ret; - - error: - goto cleanup; } @@ -892,20 +832,15 @@ virshNodedevEventNameCompleter(vshControl *ctl ATTRIBUTE_UNUSED, virCheckFlags(0, NULL); if (VIR_ALLOC_N(tmp, VIR_NODE_DEVICE_EVENT_ID_LAST + 1) < 0) - goto error; + return NULL; for (i = 0; i < VIR_NODE_DEVICE_EVENT_ID_LAST; i++) { if (VIR_STRDUP(tmp[i], virshNodedevEventCallbacks[i].name) < 0) - goto error; + return NULL; } VIR_STEAL_PTR(ret, tmp); - - cleanup: return ret; - - error: - goto cleanup; } @@ -927,33 +862,28 @@ virshCellnoCompleter(vshControl *ctl, virCheckFlags(0, NULL); if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) - goto error; + return NULL; if (!(cap_xml = virConnectGetCapabilities(priv->conn))) - goto error; + return NULL; if (!(doc = virXMLParseStringCtxt(cap_xml, _("capabilities"), &ctxt))) - goto error; + return NULL; ncells = virXPathNodeSet("/capabilities/host/topology/cells/cell", ctxt, &cells); if (ncells <= 0) - goto error; + return NULL; if (VIR_ALLOC_N(tmp, ncells + 1)) - goto error; + return NULL; for (i = 0; i < ncells; i++) { if (!(tmp[i] = virXMLPropString(cells[i], "id"))) - goto error; + return NULL; } VIR_STEAL_PTR(ret, tmp); - - cleanup: return ret; - - error: - goto cleanup; } @@ -981,21 +911,20 @@ virshDomainDeviceAliasCompleter(vshControl *ctl, domainXMLFlags = VIR_DOMAIN_XML_INACTIVE; if (virshDomainGetXML(ctl, cmd, domainXMLFlags, &xmldoc, &ctxt) < 0) - goto cleanup; + return NULL; naliases = virXPathNodeSet("./devices//alias/@name", ctxt, &aliases); if (naliases < 0) - goto cleanup; + return NULL; if (VIR_ALLOC_N(tmp, naliases + 1) < 0) - goto cleanup; + return NULL; for (i = 0; i < naliases; i++) { if (!(tmp[i] = virXMLNodeContentString(aliases[i]))) - goto cleanup; + return NULL; } VIR_STEAL_PTR(ret, tmp); - cleanup: return ret; } -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:29 +0200, Ján Tomko wrote:
Now that we have a shared cleanup section everywhere, delete all the 'error' labels which all contain just 'goto cleanup' anyway.
Also remove all the 'cleanup' labels that only 'return ret' - we can simply return NULL instead of jumping to that label.
Signed-off-by: Ján Tomko <jtomko@redhat.com> ---
ACK
A helper function that takes a XML node with a "size" and "unit" attributes and converts it into a human-readable string. Reduce the size and number of variables in the parent function. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 2b9749300a..5985f09272 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -615,27 +615,44 @@ virshSnapshotNameCompleter(vshControl *ctl, return ret; } +static char * +virshPagesizeNodeToString(xmlNodePtr node) +{ + VIR_AUTOFREE(char *) pagesize = NULL; + VIR_AUTOFREE(char *) unit = NULL; + unsigned long long byteval = 0; + const char *suffix = NULL; + double size = 0; + char *ret; + + pagesize = virXMLPropString(node, "size"); + unit = virXMLPropString(node, "unit"); + if (virStrToLong_ull(pagesize, NULL, 10, &byteval) < 0) + return NULL; + if (virScaleInteger(&byteval, unit, 1024, UINT_MAX) < 0) + return NULL; + size = vshPrettyCapacity(byteval, &suffix); + if (virAsprintf(&ret, "%.0f%s", size, suffix) < 0) + return NULL; + return ret; +} + char ** virshAllocpagesPagesizeCompleter(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED, unsigned int flags) { - unsigned long long byteval = 0; VIR_AUTOPTR(xmlXPathContext) ctxt = NULL; virshControlPtr priv = ctl->privData; unsigned int npages = 0; VIR_AUTOFREE(xmlNodePtr *) pages = NULL; VIR_AUTOPTR(xmlDoc) doc = NULL; - double size = 0; size_t i = 0; - const char *suffix = NULL; const char *cellnum = NULL; bool cellno = vshCommandOptBool(cmd, "cellno"); VIR_AUTOFREE(char *) path = NULL; - VIR_AUTOFREE(char *) pagesize = NULL; VIR_AUTOFREE(char *) cap_xml = NULL; char **ret = NULL; - VIR_AUTOFREE(char *) unit = NULL; VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -667,16 +684,7 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, return NULL; for (i = 0; i < npages; i++) { - VIR_FREE(pagesize); - VIR_FREE(unit); - pagesize = virXMLPropString(pages[i], "size"); - unit = virXMLPropString(pages[i], "unit"); - if (virStrToLong_ull(pagesize, NULL, 10, &byteval) < 0) - return NULL; - if (virScaleInteger(&byteval, unit, 1024, UINT_MAX) < 0) - return NULL; - size = vshPrettyCapacity(byteval, &suffix); - if (virAsprintf(&tmp[i], "%.0f%s", size, suffix) < 0) + if (!(tmp[i] = virshPagesizeNodeToString(pages[i]))) return NULL; } -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:30 +0200, Ján Tomko wrote:
A helper function that takes a XML node with a "size" and "unit" attributes and converts it into a human-readable string.
Reduce the size and number of variables in the parent function.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)
ACK
Suggest some passwords to the user. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 58 +++++++++++++++++++++++++++++++++++++++++ tools/virsh-completer.h | 4 +++ tools/virsh-domain.c | 1 + 3 files changed, 63 insertions(+) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 5985f09272..0687670d37 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -32,6 +32,7 @@ #include "virutil.h" #include "viralloc.h" #include "virmacaddr.h" +#include "virrandom.h" #include "virstring.h" #include "virxml.h" @@ -936,3 +937,60 @@ virshDomainDeviceAliasCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); return ret; } + + +const char *builtin_passwords[] = { + "hunter2", /* ******* */ + "nbusr123", /* Keď nevieš, tak nefušuj */ + "4ezgi4", +}; + + +char ** +virshPasswordCompleter(vshControl *ctl ATTRIBUTE_UNUSED, + const vshCmd *cmd ATTRIBUTE_UNUSED, + unsigned int flags) +{ + VIR_AUTOFREE(char *) base64 = NULL; + VIR_AUTOFREE(unsigned char *) rand = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; + const size_t optimal_passlen = 8; /* ought to be enough */ + const char *prefix = NULL; + const size_t num = 1; + char **ret = NULL; + size_t missing; + size_t i; + + virCheckFlags(0, NULL); + + if (VIR_ALLOC_N(tmp, num + ARRAY_CARDINALITY(builtin_passwords) + 1) < 0) + return NULL; + + ignore_value(vshCommandOptStringQuiet(ctl, cmd, "password", &prefix)); + if (STREQ_NULLABLE(prefix, " ")) + prefix = NULL; + + missing = optimal_passlen - MIN(strlen(NULLSTR_EMPTY(prefix)), optimal_passlen); + + if (VIR_ALLOC_N(rand, 7) < 0) + return NULL; + + if (virRandomBytes(rand, 6) < 0) + return NULL; + + if (!(base64 = virStringEncodeBase64(rand, 6))) + return NULL; + + base64[missing] = '\0'; + + if (virAsprintf(&tmp[0], "%s%s", NULLSTR_EMPTY(prefix), base64) < 0) + return NULL; + + for (i = 0; i < ARRAY_CARDINALITY(builtin_passwords); i++) { + if (VIR_STRDUP(tmp[i + 1], builtin_passwords[i]) < 0) + return NULL; + } + + VIR_STEAL_PTR(ret, tmp); + return ret; +} diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h index 2e2e1edafb..d47a5f4da6 100644 --- a/tools/virsh-completer.h +++ b/tools/virsh-completer.h @@ -110,4 +110,8 @@ char ** virshDomainDeviceAliasCompleter(vshControl *ctl, char ** virshCellnoCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** virshPasswordCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); #endif /* LIBVIRT_VIRSH_COMPLETER_H */ diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e8d5404acf..d8978f5bd1 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5732,6 +5732,7 @@ static const vshCmdOptDef opts_set_user_password[] = { {.name = "password", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, + .completer = virshPasswordCompleter, .help = N_("the new password") }, {.name = "encrypted", -- 2.20.1
On Mon, Apr 01, 2019 at 09:33:31AM +0200, Ján Tomko wrote:
Suggest some passwords to the user.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 58 +++++++++++++++++++++++++++++++++++++++++ tools/virsh-completer.h | 4 +++ tools/virsh-domain.c | 1 + 3 files changed, 63 insertions(+)
diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 5985f09272..0687670d37 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -32,6 +32,7 @@ #include "virutil.h" #include "viralloc.h" #include "virmacaddr.h" +#include "virrandom.h" #include "virstring.h" #include "virxml.h"
@@ -936,3 +937,60 @@ virshDomainDeviceAliasCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); return ret; } + + +const char *builtin_passwords[] = { + "hunter2", /* ******* */ + "nbusr123", /* Keď nevieš, tak nefušuj */ + "4ezgi4", +};
This is quite a limited list of paswords. I think it would be useful to expand it with the password dump from haveibeenpwned.com The main problem is that the overhead of a static array with 500,000,000 passwords might make libvirt packages too large. RPM used to have problems with packages larger than 2 GB, so not sure how well it will handle 11 GB RPMs. There could be a negative impact on memory usage when running libvirt, though virt hosts usually have lots of RAM, so reserving 11 GB for virsh shouldn't be too big a problem.
+ + +char ** +virshPasswordCompleter(vshControl *ctl ATTRIBUTE_UNUSED, + const vshCmd *cmd ATTRIBUTE_UNUSED, + unsigned int flags) +{ + VIR_AUTOFREE(char *) base64 = NULL; + VIR_AUTOFREE(unsigned char *) rand = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; + const size_t optimal_passlen = 8; /* ought to be enough */ + const char *prefix = NULL; + const size_t num = 1; + char **ret = NULL; + size_t missing; + size_t i; + + virCheckFlags(0, NULL); + + if (VIR_ALLOC_N(tmp, num + ARRAY_CARDINALITY(builtin_passwords) + 1) < 0) + return NULL; + + ignore_value(vshCommandOptStringQuiet(ctl, cmd, "password", &prefix)); + if (STREQ_NULLABLE(prefix, " ")) + prefix = NULL; + + missing = optimal_passlen - MIN(strlen(NULLSTR_EMPTY(prefix)), optimal_passlen); + + if (VIR_ALLOC_N(rand, 7) < 0) + return NULL; + + if (virRandomBytes(rand, 6) < 0) + return NULL; + + if (!(base64 = virStringEncodeBase64(rand, 6))) + return NULL; + + base64[missing] = '\0'; + + if (virAsprintf(&tmp[0], "%s%s", NULLSTR_EMPTY(prefix), base64) < 0) + return NULL; + + for (i = 0; i < ARRAY_CARDINALITY(builtin_passwords); i++) { + if (VIR_STRDUP(tmp[i + 1], builtin_passwords[i]) < 0) + return NULL;
Hmm, so an 11 GB static password list will need another 11GB of heap allocation. This is getting quite inefficient at scale. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
+ + +const char *builtin_passwords[] = { + "hunter2", /* ******* */ + "nbusr123", /* Keď nevieš, tak nefušuj */
^^^^you didn't :D :D :D :D, epic! For those who are wondering what ^this gem is, Slovakia's National Security Authority (slovak acronym NBU) used this ultrahard password on their servers until there was an incident where hackers used that pw to (with the login combo of "nbusr") break essentially into every machine on the subnet, downloaded almost 20GB of classified documents, emails, backups, DB dumps, etc. Erik
On 4/1/19 3:33 AM, Ján Tomko wrote:
Suggest some passwords to the user.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer.c | 58 +++++++++++++++++++++++++++++++++++++++++ tools/virsh-completer.h | 4 +++ tools/virsh-domain.c | 1 + 3 files changed, 63 insertions(+)
diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 5985f09272..0687670d37 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -32,6 +32,7 @@ #include "virutil.h" #include "viralloc.h" #include "virmacaddr.h" +#include "virrandom.h" #include "virstring.h" #include "virxml.h"
@@ -936,3 +937,60 @@ virshDomainDeviceAliasCompleter(vshControl *ctl, VIR_STEAL_PTR(ret, tmp); return ret; } + + +const char *builtin_passwords[] = { + "*******", /* ******* */ + "********", /* Keď nevieš, tak nefušuj */ + "******", +};
I hope that git push doesn't perform the same redaction as git send-email did (or perhaps the server end of git is equipped to reverse it?) (This "feature" of git send-email makes it hard for us to judge the usefulness of the list of recommendations BTW.)
participants (5)
-
Daniel P. Berrangé -
Erik Skultety -
Ján Tomko -
Laine Stump -
Peter Krempa