[libvirt] [PATCH 0/3] virsh: Fix virshDomainInterfaceSourceCompleter
*** BLURB HERE *** Michal Prívozník (3): virsh: Use VIR_ENUM_* instead of open coding string -> enum conversion virsh: Fix virshDomainInterfaceSourceCompleter cmdDomIfAddr: Move domain lookup down a few lines tools/virsh-completer-domain.c | 17 ++++++++++------- tools/virsh-domain-monitor.c | 31 +++++++++++++++---------------- tools/virsh-domain-monitor.h | 2 ++ tools/virsh.h | 1 + 4 files changed, 28 insertions(+), 23 deletions(-) -- 2.24.1
There are more occurrences, but I'm converting --source argument of domifaddr command only, because I will need it in next commit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 9e6bc99bf2..de4abbaee7 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -2358,6 +2358,13 @@ static const vshCmdOptDef opts_domifaddr[] = { {.name = NULL} }; +VIR_ENUM_DECL(virshDomainInterfaceAddressesSource); +VIR_ENUM_IMPL(virshDomainInterfaceAddressesSource, + VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST, + "lease", + "agent", + "arp"); + static bool cmdDomIfAddr(vshControl *ctl, const vshCmd *cmd) { @@ -2379,17 +2386,10 @@ cmdDomIfAddr(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "source", &sourcestr) < 0) goto cleanup; - if (sourcestr) { - if (STREQ(sourcestr, "lease")) { - source = VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE; - } else if (STREQ(sourcestr, "agent")) { - source = VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT; - } else if (STREQ(sourcestr, "arp")) { - source = VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_ARP; - } else { - vshError(ctl, _("Unknown data source '%s'"), sourcestr); - goto cleanup; - } + if (sourcestr && + (source = virshDomainInterfaceAddressesSourceTypeFromString(sourcestr)) < 0) { + vshError(ctl, _("Unknown data source '%s'"), sourcestr); + goto cleanup; } if ((ifaces_count = virDomainInterfaceAddresses(dom, &ifaces, source, 0)) < 0) { -- 2.24.1
On Tue, Jan 07, 2020 at 09:41:52AM +0100, Michal Privoznik wrote:
There are more occurrences, but I'm converting --source argument of domifaddr command only, because I will need it in next commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Introduced in v5.10.0-449-gcf44ec5577 it used virshCommaStringListComplete() to generate list of options. But this is not correct because the '--source' argument of the 'domifaddr' doesn't accept a string list (for instance "arp,agent,lease") rather than a single string. Therefore, the completer must return these strings separately and thus must refrain from using virshCommaStringListComplete(). At the same time, now that we have strings we need declared as an enum we can use TypeToString() instead of copying strings. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-completer-domain.c | 17 ++++++++++------- tools/virsh-domain-monitor.c | 1 - tools/virsh-domain-monitor.h | 2 ++ tools/virsh.h | 1 + 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 9423d2efb3..6da603048e 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -24,6 +24,7 @@ #include "viralloc.h" #include "virmacaddr.h" #include "virsh-domain.h" +#include "virsh-domain-monitor.h" #include "virsh-util.h" #include "virsh.h" #include "virstring.h" @@ -299,17 +300,19 @@ virshDomainShutdownModeCompleter(vshControl *ctl, char ** -virshDomainInterfaceAddrSourceCompleter(vshControl *ctl, - const vshCmd *cmd, +virshDomainInterfaceAddrSourceCompleter(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, unsigned int flags) { - const char *sources[] = {"lease", "agent", "arp", NULL}; - const char *source = NULL; + char **ret = NULL; + size_t i; virCheckFlags(0, NULL); - if (vshCommandOptStringQuiet(ctl, cmd, "source", &source) < 0) - return NULL; + ret = g_new0(typeof(*ret), VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST + 1); - return virshCommaStringListComplete(source, sources); + for (i = 0; i < VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST; i++) + ret[i] = g_strdup(virshDomainInterfaceAddressesSourceTypeToString(i)); + + return ret; } diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index de4abbaee7..97301f71f9 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -2358,7 +2358,6 @@ static const vshCmdOptDef opts_domifaddr[] = { {.name = NULL} }; -VIR_ENUM_DECL(virshDomainInterfaceAddressesSource); VIR_ENUM_IMPL(virshDomainInterfaceAddressesSource, VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST, "lease", diff --git a/tools/virsh-domain-monitor.h b/tools/virsh-domain-monitor.h index 11a9156ae2..0de47c50c4 100644 --- a/tools/virsh-domain-monitor.h +++ b/tools/virsh-domain-monitor.h @@ -26,4 +26,6 @@ char *virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool title, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; +VIR_ENUM_DECL(virshDomainInterfaceAddressesSource); + extern const vshCmdDef domMonitoringCmds[]; diff --git a/tools/virsh.h b/tools/virsh.h index d84659124a..903a2e53b6 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -31,6 +31,7 @@ #include "virpolkit.h" #include "vsh.h" #include "virsh-completer.h" +#include "virenum.h" #define VIRSH_PROMPT_RW "virsh # " #define VIRSH_PROMPT_RO "virsh > " -- 2.24.1
On Tue, Jan 07, 2020 at 09:41:53AM +0100, Michal Privoznik wrote:
Introduced in v5.10.0-449-gcf44ec5577 it used virshCommaStringListComplete() to generate list of options. But this is not correct because the '--source' argument of the 'domifaddr' doesn't accept a string list (for instance "arp,agent,lease") rather than a single string. Therefore, the completer must return these strings separately and thus must refrain from using virshCommaStringListComplete().
At the same time, now that we have strings we need declared as an enum we can use TypeToString() instead of copying strings.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-completer-domain.c | 17 ++++++++++------- tools/virsh-domain-monitor.c | 1 - tools/virsh-domain-monitor.h | 2 ++ tools/virsh.h | 1 + 4 files changed, 13 insertions(+), 8 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
The 'domifaddr' command accepts several arguments. Let's validate them first and look up domain to work with only after to save some RPC cycles should validation fail. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 97301f71f9..e357635757 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -2377,20 +2377,20 @@ cmdDomIfAddr(vshControl *ctl, const vshCmd *cmd) const char *sourcestr = NULL; int source = VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE; - if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) - return false; - if (vshCommandOptStringReq(ctl, cmd, "interface", &ifacestr) < 0) - goto cleanup; + return false; if (vshCommandOptStringReq(ctl, cmd, "source", &sourcestr) < 0) - goto cleanup; + return false; if (sourcestr && (source = virshDomainInterfaceAddressesSourceTypeFromString(sourcestr)) < 0) { vshError(ctl, _("Unknown data source '%s'"), sourcestr); - goto cleanup; + return false; } + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + if ((ifaces_count = virDomainInterfaceAddresses(dom, &ifaces, source, 0)) < 0) { vshError(ctl, _("Failed to query for interfaces addresses")); goto cleanup; -- 2.24.1
On Tue, Jan 07, 2020 at 09:41:54AM +0100, Michal Privoznik wrote:
The 'domifaddr' command accepts several arguments. Let's validate them first and look up domain to work with only after to save some RPC cycles should validation fail.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik