[libvirt] [PATCH 00/19] virsh completion improvement
Lin Ma (19): virsh: Add current vcpu list completion to guestvcpus command virsh: Add logical CPU IDs completion for nodecpustats command virsh: Add serial/parallel device name completion to console command virsh: Rename virshInterfaceNameCompleter to virshInterfaceCompleter virsh: create a macro with a _FULL suffix for common --interface option virsh: Add interface name completion to iface-bridge command virsh: Add interface name completion to iface-mac command virsh: Add interface mac completion to iface-name command virdomainobjlist: Add vnc into filter group to check the vnc existence of guest virsh: Only return domains that have VNC display to vncdisplay command vsh: Fix completion error in case of multiple mac addresses virsh: Add mac completion to net-dhcp-leases command virsh-domain: Introduce 2 macros for domain options 'interface' and 'mac' virsh-domain: Apply macro VIRSH_DOMAIN_OPT_INTERFACE for interface option virsh-domain: Apply macro VIRSH_DOMAIN_OPT_MAC for mac option virsh: Move/add some of function declarations to virsh-domain.h virsh: Add signal name completion to send-process-signal command virsh: Add lifecycle type completion to set-lifecycle-action command virsh: Add lifecycle action completion to set-lifecycle-action command include/libvirt/libvirt-domain.h | 3 + src/conf/virdomainobjlist.c | 15 +++ src/conf/virdomainobjlist.h | 7 +- tools/bash-completion/vsh | 1 + tools/virsh-completer-domain.c | 153 +++++++++++++++++++++++++++++- tools/virsh-completer-domain.h | 20 ++++ tools/virsh-completer-host.c | 35 +++++++ tools/virsh-completer-host.h | 4 + tools/virsh-completer-interface.c | 19 ++-- tools/virsh-completer-interface.h | 10 +- tools/virsh-completer-network.c | 45 +++++++++ tools/virsh-completer-network.h | 4 + tools/virsh-domain-monitor.c | 20 +--- tools/virsh-domain-monitor.h | 1 + tools/virsh-domain.c | 30 ++---- tools/virsh-domain.h | 22 +++++ tools/virsh-host.c | 1 + tools/virsh-interface.c | 39 ++++---- tools/virsh-network.c | 15 ++- 19 files changed, 367 insertions(+), 77 deletions(-) -- 2.26.0
Unlike the setvcpu command, The option --cpulist of guestvcpus command means the current vcpus list, rather than the maxvcpus list. Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-domain.c | 36 ++++++++++++++++++++++++++++++++++ tools/virsh-completer-domain.h | 4 ++++ tools/virsh-domain.c | 1 + 3 files changed, 41 insertions(+) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index c657627ac1..6f67c91746 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -581,3 +581,39 @@ virshDomainCpulistCompleter(vshControl *ctl, return virshCommaStringListComplete(cpuid, (const char **)cpulist); } + +char ** +virshDomainVcpulistViaAgentCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags) +{ + virDomainPtr dom = NULL; + int nvcpus; + unsigned int id; + VIR_AUTOSTRINGLIST cpulist = NULL; + const char *vcpuid = NULL; + char **ret = NULL; + + virCheckFlags(0, NULL); + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return NULL; + + if (vshCommandOptStringQuiet(ctl, cmd, "cpulist", &vcpuid) < 0) + goto cleanup; + + /* retrieve vcpu count from the guest instead of the hypervisor */ + if ((nvcpus = virDomainGetVcpusFlags(dom, VIR_DOMAIN_VCPU_GUEST)) < 0) + goto cleanup; + + cpulist = g_new0(char *, nvcpus + 1); + + for (id = 0; id < nvcpus; id++) + cpulist[id] = g_strdup_printf("%u", id); + + ret = virshCommaStringListComplete(vcpuid, (const char **)cpulist); + + cleanup: + virshDomainFree(dom); + return ret; +} diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h index d38efd5ea8..d5021f6aa6 100644 --- a/tools/virsh-completer-domain.h +++ b/tools/virsh-completer-domain.h @@ -90,3 +90,7 @@ char ** virshDomainVcpulistCompleter(vshControl *ctl, char ** virshDomainCpulistCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** virshDomainVcpulistViaAgentCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ef347585e8..c051b047ea 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7436,6 +7436,7 @@ static const vshCmdOptDef opts_guestvcpus[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "cpulist", .type = VSH_OT_STRING, + .completer = virshDomainVcpulistViaAgentCompleter, .help = N_("list of cpus to enable or disable") }, {.name = "enable", -- 2.26.0
On 11/2/20 9:26 AM, Lin Ma wrote:
Unlike the setvcpu command, The option --cpulist of guestvcpus command means the current vcpus list, rather than the maxvcpus list.
Not really. It depends what mode the command operates in. If it's --enable then offlined vCPUs should be listed, if it's --disable then online vCPUs should be listed. Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-host.c | 35 +++++++++++++++++++++++++++++++++++ tools/virsh-completer-host.h | 4 ++++ tools/virsh-host.c | 1 + 3 files changed, 40 insertions(+) diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c index 339390aa00..a74578d2e2 100644 --- a/tools/virsh-completer-host.c +++ b/tools/virsh-completer-host.c @@ -136,3 +136,38 @@ virshCellnoCompleter(vshControl *ctl, return g_steal_pointer(&tmp); } + + +char ** +virshCpuCompleter(vshControl *ctl, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + int i, cpuid = 0, cpunum, offset = 0; + unsigned int online; + g_autofree unsigned char *cpumap = NULL; + char **ret = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; + virshControlPtr priv = ctl->privData; + + virCheckFlags(0, NULL); + + if ((cpunum = virNodeGetCPUMap(priv->conn, &cpumap, &online, 0)) < 0) + return NULL; + + tmp = g_new0(char *, online + 1); + + for (i = 0; i < cpunum; i++) { + if (VIR_CPU_USED(cpumap, cpuid) == 0) { + offset += 1; + cpuid += 1; + continue; + } else { + tmp[i - offset] = g_strdup_printf("%u", cpuid++); + } + } + + ret = g_steal_pointer(&tmp); + + return ret; +} diff --git a/tools/virsh-completer-host.h b/tools/virsh-completer-host.h index 921beb7a2d..777783b908 100644 --- a/tools/virsh-completer-host.h +++ b/tools/virsh-completer-host.h @@ -29,3 +29,7 @@ char ** virshAllocpagesPagesizeCompleter(vshControl *ctl, char ** virshCellnoCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** virshCpuCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-host.c b/tools/virsh-host.c index cda2ef4ac1..4774f76ed8 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -751,6 +751,7 @@ static const vshCmdInfo info_nodecpustats[] = { static const vshCmdOptDef opts_node_cpustats[] = { {.name = "cpu", .type = VSH_OT_INT, + .completer = virshCpuCompleter, .help = N_("prints specified cpu statistics only.") }, {.name = "percent", -- 2.26.0
On 11/2/20 9:26 AM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-host.c | 35 +++++++++++++++++++++++++++++++++++ tools/virsh-completer-host.h | 4 ++++ tools/virsh-host.c | 1 + 3 files changed, 40 insertions(+)
diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c index 339390aa00..a74578d2e2 100644 --- a/tools/virsh-completer-host.c +++ b/tools/virsh-completer-host.c @@ -136,3 +136,38 @@ virshCellnoCompleter(vshControl *ctl,
return g_steal_pointer(&tmp); } + + +char ** +virshCpuCompleter(vshControl *ctl, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + int i, cpuid = 0, cpunum, offset = 0; + unsigned int online; + g_autofree unsigned char *cpumap = NULL; + char **ret = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; + virshControlPtr priv = ctl->privData; + + virCheckFlags(0, NULL); + + if ((cpunum = virNodeGetCPUMap(priv->conn, &cpumap, &online, 0)) < 0) + return NULL; + + tmp = g_new0(char *, online + 1); + + for (i = 0; i < cpunum; i++) { + if (VIR_CPU_USED(cpumap, cpuid) == 0) { + offset += 1; + cpuid += 1; + continue; + } else { + tmp[i - offset] = g_strdup_printf("%u", cpuid++); + } + }
This loop is needlessly complicated.
+ + ret = g_steal_pointer(&tmp);
@ret can be dropped.
+ + return ret; +}
ACK with this squashed in: diff --git i/tools/virsh-completer-host.c w/tools/virsh-completer-host.c index a74578d2e2..685fa23fd4 100644 --- i/tools/virsh-completer-host.c +++ w/tools/virsh-completer-host.c @@ -143,12 +143,13 @@ virshCpuCompleter(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED, unsigned int flags) { - int i, cpuid = 0, cpunum, offset = 0; + virshControlPtr priv = ctl->privData; + VIR_AUTOSTRINGLIST tmp = NULL; + size_t i; + int cpunum; + size_t offset = 0; unsigned int online; g_autofree unsigned char *cpumap = NULL; - char **ret = NULL; - VIR_AUTOSTRINGLIST tmp = NULL; - virshControlPtr priv = ctl->privData; virCheckFlags(0, NULL); @@ -158,16 +159,11 @@ virshCpuCompleter(vshControl *ctl, tmp = g_new0(char *, online + 1); for (i = 0; i < cpunum; i++) { - if (VIR_CPU_USED(cpumap, cpuid) == 0) { - offset += 1; - cpuid += 1; + if (VIR_CPU_USED(cpumap, i) == 0) continue; - } else { - tmp[i - offset] = g_strdup_printf("%u", cpuid++); - } + + tmp[offset++] = g_strdup_printf("%zu", i); } - ret = g_steal_pointer(&tmp); - - return ret; + return g_steal_pointer(&tmp); } Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-domain.c | 54 ++++++++++++++++++++++++++++++++++ tools/virsh-completer-domain.h | 4 +++ tools/virsh-domain.c | 1 + 3 files changed, 59 insertions(+) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 6f67c91746..ab81a0dcfe 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -617,3 +617,57 @@ virshDomainVcpulistViaAgentCompleter(vshControl *ctl, virshDomainFree(dom); return ret; } + +char ** +virshDomainConsoleCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags) +{ + virshControlPtr priv = ctl->privData; + g_autoptr(xmlDoc) xmldoc = NULL; + g_autoptr(xmlXPathContext) ctxt = NULL; + int nserials, nparallels, offset = 0, head; + g_autofree xmlNodePtr *serials = NULL; + g_autofree xmlNodePtr *parallels = NULL; + size_t i; + VIR_AUTOSTRINGLIST tmp = NULL; + + virCheckFlags(0, NULL); + + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) + return NULL; + + if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0) + return NULL; + + nserials = virXPathNodeSet("./devices/serial", ctxt, &serials); + if (nserials < 0) + return NULL; + + nparallels = virXPathNodeSet("./devices/parallel", ctxt, ¶llels); + if (nparallels < 0) + return NULL; + + tmp = g_new0(char *, nserials + nparallels + 1); + + for (i = 0; i < nserials; i++) { + ctxt->node = serials[i]; + if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty")) + offset += 1; + else + tmp[i - offset] = virXPathString("string(./alias/@name)", ctxt); + } + + head = i - offset; + offset = 0; + + for (i = 0; i < nparallels; i++) { + ctxt->node = parallels[i]; + if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty")) + offset += 1; + else + tmp[head + i - offset] = virXPathString("string(./alias/@name)", ctxt); + } + + return g_steal_pointer(&tmp); +} diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h index d5021f6aa6..02fea2fe94 100644 --- a/tools/virsh-completer-domain.h +++ b/tools/virsh-completer-domain.h @@ -94,3 +94,7 @@ char ** virshDomainCpulistCompleter(vshControl *ctl, char ** virshDomainVcpulistViaAgentCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** virshDomainConsoleCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c051b047ea..7189c8c826 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2960,6 +2960,7 @@ static const vshCmdOptDef opts_console[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "devname", /* sc_prohibit_devname */ .type = VSH_OT_STRING, + .completer = virshDomainConsoleCompleter, .help = N_("character device name") }, {.name = "force", -- 2.26.0
On 11/2/20 9:26 AM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-domain.c | 54 ++++++++++++++++++++++++++++++++++ tools/virsh-completer-domain.h | 4 +++ tools/virsh-domain.c | 1 + 3 files changed, 59 insertions(+)
diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 6f67c91746..ab81a0dcfe 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -617,3 +617,57 @@ virshDomainVcpulistViaAgentCompleter(vshControl *ctl, virshDomainFree(dom); return ret; } + +char ** +virshDomainConsoleCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags) +{ + virshControlPtr priv = ctl->privData; + g_autoptr(xmlDoc) xmldoc = NULL; + g_autoptr(xmlXPathContext) ctxt = NULL; + int nserials, nparallels, offset = 0, head; + g_autofree xmlNodePtr *serials = NULL; + g_autofree xmlNodePtr *parallels = NULL; + size_t i; + VIR_AUTOSTRINGLIST tmp = NULL; + + virCheckFlags(0, NULL); + + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) + return NULL; + + if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0) + return NULL; + + nserials = virXPathNodeSet("./devices/serial", ctxt, &serials); + if (nserials < 0) + return NULL; + + nparallels = virXPathNodeSet("./devices/parallel", ctxt, ¶llels); + if (nparallels < 0) + return NULL; + + tmp = g_new0(char *, nserials + nparallels + 1); + + for (i = 0; i < nserials; i++) { + ctxt->node = serials[i]; + if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty")) + offset += 1; + else + tmp[i - offset] = virXPathString("string(./alias/@name)", ctxt);
This is needlessly complicated way to address an item in array.
+ } + + head = i - offset; + offset = 0; + + for (i = 0; i < nparallels; i++) { + ctxt->node = parallels[i]; + if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty")) + offset += 1; + else + tmp[head + i - offset] = virXPathString("string(./alias/@name)", ctxt);
And so it this.
+ } + + return g_steal_pointer(&tmp); +}
ACK with this squashed in: diff --git i/tools/virsh-completer-domain.c w/tools/virsh-completer-domain.c index ab81a0dcfe..221c891a30 100644 --- i/tools/virsh-completer-domain.c +++ w/tools/virsh-completer-domain.c @@ -626,10 +626,12 @@ virshDomainConsoleCompleter(vshControl *ctl, virshControlPtr priv = ctl->privData; g_autoptr(xmlDoc) xmldoc = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; - int nserials, nparallels, offset = 0, head; + int nserials; + int nparallels; g_autofree xmlNodePtr *serials = NULL; g_autofree xmlNodePtr *parallels = NULL; size_t i; + size_t offset = 0; VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(0, NULL); @@ -652,21 +654,20 @@ virshDomainConsoleCompleter(vshControl *ctl, for (i = 0; i < nserials; i++) { ctxt->node = serials[i]; + if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty")) - offset += 1; - else - tmp[i - offset] = virXPathString("string(./alias/@name)", ctxt); + continue; + + tmp[offset++] = virXPathString("string(./alias/@name)", ctxt); } - head = i - offset; - offset = 0; - for (i = 0; i < nparallels; i++) { ctxt->node = parallels[i]; + if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty")) - offset += 1; - else - tmp[head + i - offset] = virXPathString("string(./alias/@name)", ctxt); + continue; + + tmp[offset++] = virXPathString("string(./alias/@name)", ctxt); } return g_steal_pointer(&tmp); Michal
Rename the function virshInterfaceNameCompleter to virshInterfaceCompleter to make it a bit more generic. The upcoming patch invokes it for mac completion. Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-interface.c | 6 +++--- tools/virsh-completer-interface.h | 6 +++--- tools/virsh-interface.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/virsh-completer-interface.c b/tools/virsh-completer-interface.c index 8028db8746..777bb22b0b 100644 --- a/tools/virsh-completer-interface.c +++ b/tools/virsh-completer-interface.c @@ -26,9 +26,9 @@ #include "virstring.h" char ** -virshInterfaceNameCompleter(vshControl *ctl, - const vshCmd *cmd G_GNUC_UNUSED, - unsigned int flags) +virshInterfaceCompleter(vshControl *ctl, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) { virshControlPtr priv = ctl->privData; virInterfacePtr *ifaces = NULL; diff --git a/tools/virsh-completer-interface.h b/tools/virsh-completer-interface.h index 893dee5a6b..2b382222d7 100644 --- a/tools/virsh-completer-interface.h +++ b/tools/virsh-completer-interface.h @@ -22,6 +22,6 @@ #include "vsh.h" -char ** virshInterfaceNameCompleter(vshControl *ctl, - const vshCmd *cmd, - unsigned int flags); +char ** virshInterfaceCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 8cdbc6e85f..bd57648779 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -23,7 +23,7 @@ .type = VSH_OT_DATA, \ .flags = VSH_OFLAG_REQ, \ .help = N_("interface name or MAC address"), \ - .completer = virshInterfaceNameCompleter, \ + .completer = virshInterfaceCompleter, \ .completer_flags = cflags, \ } -- 2.26.0
On 11/2/20 9:26 AM, Lin Ma wrote:
Rename the function virshInterfaceNameCompleter to virshInterfaceCompleter to make it a bit more generic. The upcoming patch invokes it for mac completion.
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-interface.c | 6 +++--- tools/virsh-completer-interface.h | 6 +++--- tools/virsh-interface.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/virsh-completer-interface.c b/tools/virsh-completer-interface.c index 8028db8746..777bb22b0b 100644 --- a/tools/virsh-completer-interface.c +++ b/tools/virsh-completer-interface.c @@ -26,9 +26,9 @@ #include "virstring.h"
char ** -virshInterfaceNameCompleter(vshControl *ctl, - const vshCmd *cmd G_GNUC_UNUSED, - unsigned int flags) +virshInterfaceCompleter(vshControl *ctl, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags)
Looking into the future patches, I think we want a different approach. In one of future patches you will switch this to return MAC addresses too. Fair enough, points for code re-use. But The way it's implemented is confusing and in fact wrong (see my review to 08/19). How about: 1) Moving this body into a static helper, which would accept one argument more: callback to get get the desired string 2) This virshInterfaceNameCompleter() would then effectively end up a single line call of that callback with virInterfaceGetName() passed as the callback, 3) New virshInterfaceMac() would be defined with virInterfaceGetMACString() passed as the callback. Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-interface.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index bd57648779..6c47767754 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -18,15 +18,18 @@ * <http://www.gnu.org/licenses/>. */ -#define VIRSH_COMMON_OPT_INTERFACE(cflags) \ +#define VIRSH_COMMON_OPT_INTERFACE(_helpstr, cflags) \ {.name = "interface", \ .type = VSH_OT_DATA, \ .flags = VSH_OFLAG_REQ, \ - .help = N_("interface name or MAC address"), \ + .help = _helpstr, \ .completer = virshInterfaceCompleter, \ .completer_flags = cflags, \ } +#define VIRSH_COMMON_OPT_INTERFACE_FULL(cflags) \ + VIRSH_COMMON_OPT_INTERFACE(N_("interface name or MAC address"), cflags) + #include <config.h> #include "virsh-interface.h" @@ -101,7 +104,7 @@ static const vshCmdInfo info_interface_edit[] = { }; static const vshCmdOptDef opts_interface_edit[] = { - VIRSH_COMMON_OPT_INTERFACE(0), + VIRSH_COMMON_OPT_INTERFACE_FULL(0), {.name = NULL} }; @@ -473,7 +476,7 @@ static const vshCmdInfo info_interface_dumpxml[] = { }; static const vshCmdOptDef opts_interface_dumpxml[] = { - VIRSH_COMMON_OPT_INTERFACE(0), + VIRSH_COMMON_OPT_INTERFACE_FULL(0), {.name = "inactive", .type = VSH_OT_BOOL, .help = N_("show inactive defined XML") @@ -570,7 +573,7 @@ static const vshCmdInfo info_interface_undefine[] = { }; static const vshCmdOptDef opts_interface_undefine[] = { - VIRSH_COMMON_OPT_INTERFACE(0), + VIRSH_COMMON_OPT_INTERFACE_FULL(0), {.name = NULL} }; @@ -609,7 +612,7 @@ static const vshCmdInfo info_interface_start[] = { }; static const vshCmdOptDef opts_interface_start[] = { - VIRSH_COMMON_OPT_INTERFACE(VIR_CONNECT_LIST_INTERFACES_INACTIVE), + VIRSH_COMMON_OPT_INTERFACE_FULL(VIR_CONNECT_LIST_INTERFACES_INACTIVE), {.name = NULL} }; @@ -648,7 +651,7 @@ static const vshCmdInfo info_interface_destroy[] = { }; static const vshCmdOptDef opts_interface_destroy[] = { - VIRSH_COMMON_OPT_INTERFACE(VIR_CONNECT_LIST_INTERFACES_ACTIVE), + VIRSH_COMMON_OPT_INTERFACE_FULL(VIR_CONNECT_LIST_INTERFACES_ACTIVE), {.name = NULL} }; -- 2.26.0
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-interface.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 6c47767754..13e120d7a0 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -785,11 +785,7 @@ static const vshCmdInfo info_interface_bridge[] = { }; static const vshCmdOptDef opts_interface_bridge[] = { - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("existing interface name") - }, + VIRSH_COMMON_OPT_INTERFACE(N_("existing interface name"), 0), {.name = "bridge", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, -- 2.26.0
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-interface.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 13e120d7a0..df97b74c4c 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -440,11 +440,7 @@ static const vshCmdInfo info_interface_mac[] = { }; static const vshCmdOptDef opts_interface_mac[] = { - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("interface name") - }, + VIRSH_COMMON_OPT_INTERFACE(N_("interface name"), 0), {.name = NULL} }; -- 2.26.0
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-interface.c | 13 +++++++++---- tools/virsh-completer-interface.h | 4 ++++ tools/virsh-interface.c | 8 +++----- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/tools/virsh-completer-interface.c b/tools/virsh-completer-interface.c index 777bb22b0b..c24a2cea6c 100644 --- a/tools/virsh-completer-interface.c +++ b/tools/virsh-completer-interface.c @@ -38,7 +38,8 @@ virshInterfaceCompleter(vshControl *ctl, VIR_AUTOSTRINGLIST tmp = NULL; virCheckFlags(VIR_CONNECT_LIST_INTERFACES_ACTIVE | - VIR_CONNECT_LIST_INTERFACES_INACTIVE, + VIR_CONNECT_LIST_INTERFACES_INACTIVE | + VIRSH_INTERFACE_COMPLETER_MAC, NULL); if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) @@ -50,9 +51,13 @@ virshInterfaceCompleter(vshControl *ctl, tmp = g_new0(char *, nifaces + 1); for (i = 0; i < nifaces; i++) { - const char *name = virInterfaceGetName(ifaces[i]); - - tmp[i] = g_strdup(name); + if (!(flags & VIRSH_INTERFACE_COMPLETER_MAC)) { + const char *name = virInterfaceGetName(ifaces[i]); + tmp[i] = g_strdup(name); + } else { + const char *mac = virInterfaceGetMACString(ifaces[i]); + tmp[i] = g_strdup(mac); + } } ret = g_steal_pointer(&tmp); diff --git a/tools/virsh-completer-interface.h b/tools/virsh-completer-interface.h index 2b382222d7..71bc44c4b9 100644 --- a/tools/virsh-completer-interface.h +++ b/tools/virsh-completer-interface.h @@ -22,6 +22,10 @@ #include "vsh.h" +enum { + VIRSH_INTERFACE_COMPLETER_MAC = 1 << 0, +}; + char ** virshInterfaceCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index df97b74c4c..1dabc6e406 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -404,11 +404,9 @@ static const vshCmdInfo info_interface_name[] = { }; static const vshCmdOptDef opts_interface_name[] = { - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("interface mac") - }, + VIRSH_COMMON_OPT_INTERFACE(N_("interface mac"), + VIR_CONNECT_LIST_INTERFACES_ACTIVE | + VIRSH_INTERFACE_COMPLETER_MAC), {.name = NULL} }; -- 2.26.0
On 11/2/20 9:26 AM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-interface.c | 13 +++++++++---- tools/virsh-completer-interface.h | 4 ++++ tools/virsh-interface.c | 8 +++----- 3 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-completer-interface.c b/tools/virsh-completer-interface.c index 777bb22b0b..c24a2cea6c 100644 --- a/tools/virsh-completer-interface.c +++ b/tools/virsh-completer-interface.c @@ -38,7 +38,8 @@ virshInterfaceCompleter(vshControl *ctl, VIR_AUTOSTRINGLIST tmp = NULL;
virCheckFlags(VIR_CONNECT_LIST_INTERFACES_ACTIVE | - VIR_CONNECT_LIST_INTERFACES_INACTIVE, + VIR_CONNECT_LIST_INTERFACES_INACTIVE | + VIRSH_INTERFACE_COMPLETER_MAC, NULL);
if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) @@ -50,9 +51,13 @@ virshInterfaceCompleter(vshControl *ctl, tmp = g_new0(char *, nifaces + 1);
for (i = 0; i < nifaces; i++) { - const char *name = virInterfaceGetName(ifaces[i]); - - tmp[i] = g_strdup(name); + if (!(flags & VIRSH_INTERFACE_COMPLETER_MAC)) { + const char *name = virInterfaceGetName(ifaces[i]); + tmp[i] = g_strdup(name); + } else { + const char *mac = virInterfaceGetMACString(ifaces[i]); + tmp[i] = g_strdup(mac); + } }
ret = g_steal_pointer(&tmp); diff --git a/tools/virsh-completer-interface.h b/tools/virsh-completer-interface.h index 2b382222d7..71bc44c4b9 100644 --- a/tools/virsh-completer-interface.h +++ b/tools/virsh-completer-interface.h @@ -22,6 +22,10 @@
#include "vsh.h"
+enum { + VIRSH_INTERFACE_COMPLETER_MAC = 1 << 0, +};
This is not correct. VIRSH_INTERFACE_COMPLETER_MAC has value of 1 after this. But so does VIR_CONNECT_LIST_INTERFACES_INACTIVE and therefore there is no way to differentiate the two. After this commit 'virsh iface-start --interface' starts printing MAC addresses instead of names. If you accept my advice in 04/19 you can avoid this new flag completely. Michal
Signed-off-by: Lin Ma <lma@suse.com> --- include/libvirt/libvirt-domain.h | 3 +++ src/conf/virdomainobjlist.c | 15 +++++++++++++++ src/conf/virdomainobjlist.h | 7 ++++++- 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index b3310729bf..c138e8299c 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1877,6 +1877,9 @@ typedef enum { VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT = 1 << 14, VIR_CONNECT_LIST_DOMAINS_NO_CHECKPOINT = 1 << 15, + + VIR_CONNECT_LIST_DOMAINS_HAS_VNC = 1 << 16, + VIR_CONNECT_LIST_DOMAINS_NO_VNC = 1 << 17, } virConnectListAllDomainsFlags; int virConnectListAllDomains (virConnectPtr conn, diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index e9a4b271df..5931669b5d 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -912,6 +912,21 @@ virDomainObjMatchFilter(virDomainObjPtr vm, return false; } + /* filter by vnc existence */ + if (MATCH(VIR_CONNECT_LIST_DOMAINS_FILTERS_VNC)) { + int i; + bool hasVnc = false; + for (i = 0; i < vm->def->ngraphics; ++i) { + if (vm->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { + hasVnc = true; + break; + } + } + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_VNC) && hasVnc) || + (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_VNC) && !hasVnc))) + return false; + } + return true; } #undef MATCH diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index 6150e13aa4..3a86e24100 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -124,6 +124,10 @@ int virDomainObjListForEach(virDomainObjListPtr doms, (VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT | \ VIR_CONNECT_LIST_DOMAINS_NO_CHECKPOINT) +#define VIR_CONNECT_LIST_DOMAINS_FILTERS_VNC \ + (VIR_CONNECT_LIST_DOMAINS_HAS_VNC | \ + VIR_CONNECT_LIST_DOMAINS_NO_VNC) + #define VIR_CONNECT_LIST_DOMAINS_FILTERS_ALL \ (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE | \ VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT | \ @@ -131,7 +135,8 @@ int virDomainObjListForEach(virDomainObjListPtr doms, VIR_CONNECT_LIST_DOMAINS_FILTERS_MANAGEDSAVE | \ VIR_CONNECT_LIST_DOMAINS_FILTERS_AUTOSTART | \ VIR_CONNECT_LIST_DOMAINS_FILTERS_SNAPSHOT | \ - VIR_CONNECT_LIST_DOMAINS_FILTERS_CHECKPOINT) + VIR_CONNECT_LIST_DOMAINS_FILTERS_CHECKPOINT | \ + VIR_CONNECT_LIST_DOMAINS_FILTERS_VNC) int virDomainObjListCollect(virDomainObjListPtr doms, virConnectPtr conn, -- 2.26.0
On Mon, Nov 02, 2020 at 16:26:11 +0800, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- include/libvirt/libvirt-domain.h | 3 +++ src/conf/virdomainobjlist.c | 15 +++++++++++++++ src/conf/virdomainobjlist.h | 7 ++++++- 3 files changed, 24 insertions(+), 1 deletion(-)
I'm not persuaded (and the non-existing commit message doesn't help) that we should add arbitrary filters here. Specifically there's a rather limited amount of flags we can use for filtering and thus we should not add them without proper consideration. Specifically anything that is related to VM config is for me off limits to be used as filtering as there are many options and they may change over time. This filtering can be done client side. It will be expensive, but command line completers are not really intensively used code paths. NACK
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-domain.c | 3 ++- tools/virsh-domain.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index ab81a0dcfe..305711151f 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -52,7 +52,8 @@ virshDomainNameCompleter(vshControl *ctl, VIR_CONNECT_LIST_DOMAINS_SHUTOFF | VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE | VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | - VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT, + VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT | + VIR_CONNECT_LIST_DOMAINS_HAS_VNC, NULL); if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 7189c8c826..5b9970fdf5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11759,7 +11759,8 @@ static const vshCmdInfo info_vncdisplay[] = { }; static const vshCmdOptDef opts_vncdisplay[] = { - VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), + VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_HAS_VNC), {.name = NULL} }; -- 2.26.0
On 11/2/20 9:26 AM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-domain.c | 3 ++- tools/virsh-domain.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index ab81a0dcfe..305711151f 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -52,7 +52,8 @@ virshDomainNameCompleter(vshControl *ctl, VIR_CONNECT_LIST_DOMAINS_SHUTOFF | VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE | VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT | - VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT, + VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT | + VIR_CONNECT_LIST_DOMAINS_HAS_VNC, NULL);
if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 7189c8c826..5b9970fdf5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11759,7 +11759,8 @@ static const vshCmdInfo info_vncdisplay[] = { };
static const vshCmdOptDef opts_vncdisplay[] = { - VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), + VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_HAS_VNC), {.name = NULL} };
As Peter says, this too big overkill. If we'd go the full length then we will need HAS_.* to cover each device/element in domain XML. I'm not in favor of such approach. Michal
We know that the bash completer automatically handle colon by preceding it with an escape character backslash. While our bash autompletion file vsh completes multiple items, In case there're multiple items which have same prefix and the content of completion items contain colon(say mac address), The vsh needs to correctly hands the backslash which are added by bash completer, Otherwise the completion won't be successful. This patch fixes this problem. e.g.: # virsh domiflist --domain VM Interface Type Source Model MAC ------------------------------------------------------------- vnet0 network default virtio 52:54:00:fb:7b:f5 vnet1 bridge br0 virtio 52:54:00:80:1b:21 Before: # virsh detach-interface --domain VM --mac <TAB> # virsh detach-interface --domain VM --mac 52\:54\:00\:<TAB><TAB> After: # virsh detach-interface --domain VM --mac <TAB> # virsh detach-interface --domain VM --mac 52\:54\:00\:<TAB><TAB> 52:54:00:80:1b:21 52:54:00:fb:7b:f5 # virsh detach-interface --domain VM --mac 52\:54\:00\: Signed-off-by: Lin Ma <lma@suse.com> --- tools/bash-completion/vsh | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/bash-completion/vsh b/tools/bash-completion/vsh index 8493cad28b..fb38e8616f 100644 --- a/tools/bash-completion/vsh +++ b/tools/bash-completion/vsh @@ -39,6 +39,7 @@ _vsh_complete() fi INPUT=( "${COMP_WORDS[@]:$i:$COMP_CWORD}" ) + INPUT[-1]=${INPUT[-1]//\\:/:} # Uncomment these lines for easy debug. # echo; -- 2.26.0
On 11/2/20 9:26 AM, Lin Ma wrote:
We know that the bash completer automatically handle colon by preceding it with an escape character backslash. While our bash autompletion file vsh completes multiple items, In case there're multiple items which have same prefix and the content of completion items contain colon(say mac address), The vsh needs to correctly hands the backslash which are added by bash completer, Otherwise the completion won't be successful. This patch fixes this problem.
e.g.:
# virsh domiflist --domain VM Interface Type Source Model MAC ------------------------------------------------------------- vnet0 network default virtio 52:54:00:fb:7b:f5 vnet1 bridge br0 virtio 52:54:00:80:1b:21
Before: # virsh detach-interface --domain VM --mac <TAB> # virsh detach-interface --domain VM --mac 52\:54\:00\:<TAB><TAB>
After: # virsh detach-interface --domain VM --mac <TAB> # virsh detach-interface --domain VM --mac 52\:54\:00\:<TAB><TAB> 52:54:00:80:1b:21 52:54:00:fb:7b:f5 # virsh detach-interface --domain VM --mac 52\:54\:00\:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/bash-completion/vsh | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/bash-completion/vsh b/tools/bash-completion/vsh index 8493cad28b..fb38e8616f 100644 --- a/tools/bash-completion/vsh +++ b/tools/bash-completion/vsh @@ -39,6 +39,7 @@ _vsh_complete() fi
INPUT=( "${COMP_WORDS[@]:$i:$COMP_CWORD}" ) + INPUT[-1]=${INPUT[-1]//\\:/:}
# Uncomment these lines for easy debug. # echo;
Ooops, yes. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-network.c | 45 +++++++++++++++++++++++++++++++++ tools/virsh-completer-network.h | 4 +++ tools/virsh-network.c | 15 +++++++---- 3 files changed, 59 insertions(+), 5 deletions(-) diff --git a/tools/virsh-completer-network.c b/tools/virsh-completer-network.c index 73f7115ab2..58bc126693 100644 --- a/tools/virsh-completer-network.c +++ b/tools/virsh-completer-network.c @@ -174,3 +174,48 @@ virshNetworkUUIDCompleter(vshControl *ctl, g_free(nets); return ret; } + + +char ** +virshNetworkDhcpMacCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags) +{ + virshControlPtr priv = ctl->privData; + virNetworkDHCPLeasePtr *leases = NULL; + virNetworkPtr network = NULL; + int nleases; + size_t i = 0; + char **ret = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; + + virCheckFlags(0, NULL); + + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) + return NULL; + + if (!(network = virshCommandOptNetwork(ctl, cmd, NULL))) + return NULL; + + if ((nleases = virNetworkGetDHCPLeases(network, NULL, &leases, flags)) < 0) { + goto cleanup; + } + + tmp = g_new0(char *, nleases + 1); + + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeasePtr lease = leases[i]; + tmp[i] = g_strdup(lease->mac); + } + + ret = g_steal_pointer(&tmp); + + cleanup: + if (leases) { + for (i = 0; i < nleases; i++) + virNetworkDHCPLeaseFree(leases[i]); + VIR_FREE(leases); + } + virNetworkFree(network); + return ret; +} diff --git a/tools/virsh-completer-network.h b/tools/virsh-completer-network.h index 8910e4525c..80df5c468e 100644 --- a/tools/virsh-completer-network.h +++ b/tools/virsh-completer-network.h @@ -37,3 +37,7 @@ char ** virshNetworkPortUUIDCompleter(vshControl *ctl, char ** virshNetworkUUIDCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** virshNetworkDhcpMacCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 745afc537d..30b231f7d6 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -61,6 +61,15 @@ .completer_flags = cflags, \ } +#define VIRSH_COMMON_OPT_NETWORK_DHCP_MAC(_helpstr, cflags) \ + {.name = "mac", \ + .type = VSH_OT_STRING, \ + .flags = VSH_OFLAG_NONE, \ + .help = _helpstr, \ + .completer = virshNetworkDhcpMacCompleter, \ + .completer_flags = cflags, \ + } + virNetworkPtr virshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd, @@ -1373,11 +1382,7 @@ static const vshCmdInfo info_network_dhcp_leases[] = { static const vshCmdOptDef opts_network_dhcp_leases[] = { VIRSH_COMMON_OPT_NETWORK_FULL(VIR_CONNECT_LIST_NETWORKS_ACTIVE), - {.name = "mac", - .type = VSH_OT_STRING, - .flags = VSH_OFLAG_NONE, - .help = N_("MAC address") - }, + VIRSH_COMMON_OPT_NETWORK_DHCP_MAC(N_("MAC address"), 0), {.name = NULL} }; -- 2.26.0
On 11/2/20 9:26 AM, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-network.c | 45 +++++++++++++++++++++++++++++++++ tools/virsh-completer-network.h | 4 +++ tools/virsh-network.c | 15 +++++++---- 3 files changed, 59 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-completer-network.c b/tools/virsh-completer-network.c index 73f7115ab2..58bc126693 100644 --- a/tools/virsh-completer-network.c +++ b/tools/virsh-completer-network.c @@ -174,3 +174,48 @@ virshNetworkUUIDCompleter(vshControl *ctl, g_free(nets); return ret; } + + +char ** +virshNetworkDhcpMacCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags) +{ + virshControlPtr priv = ctl->privData; + virNetworkDHCPLeasePtr *leases = NULL; + virNetworkPtr network = NULL; + int nleases; + size_t i = 0; + char **ret = NULL; + VIR_AUTOSTRINGLIST tmp = NULL; + + virCheckFlags(0, NULL); + + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) + return NULL; + + if (!(network = virshCommandOptNetwork(ctl, cmd, NULL))) + return NULL; + + if ((nleases = virNetworkGetDHCPLeases(network, NULL, &leases, flags)) < 0) { + goto cleanup; + } +
These curly braces look redundant, superfluous.
+ tmp = g_new0(char *, nleases + 1); + + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeasePtr lease = leases[i]; + tmp[i] = g_strdup(lease->mac); + } + + ret = g_steal_pointer(&tmp); + + cleanup: + if (leases) { + for (i = 0; i < nleases; i++) + virNetworkDHCPLeaseFree(leases[i]); + VIR_FREE(leases); + } + virNetworkFree(network); + return ret; +}
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
The macro VIRSH_DOMAIN_OPT_INTERFACE for domain option '--interface', The macro VIRSH_DOMAIN_OPT_MAC for domain option '--mac'. Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h index 0d59c579d4..ac05f983f9 100644 --- a/tools/virsh-domain.h +++ b/tools/virsh-domain.h @@ -39,3 +39,21 @@ typedef enum { VIR_ENUM_DECL(virshDomainHostnameSource); extern const vshCmdDef domManagementCmds[]; + +#define VIRSH_DOMAIN_OPT_INTERFACE(_helpstr, oflags, cflags) \ + {.name = "interface", \ + .type = VSH_OT_STRING, \ + .flags = oflags, \ + .help = _helpstr, \ + .completer = virshDomainInterfaceCompleter, \ + .completer_flags = cflags, \ + } + +#define VIRSH_DOMAIN_OPT_MAC(_helpstr, oflags) \ + {.name = "mac", \ + .type = VSH_OT_STRING, \ + .flags = oflags, \ + .help = _helpstr, \ + .completer = virshDomainInterfaceCompleter, \ + .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, \ + } -- 2.26.0
On 11/2/20 9:26 AM, Lin Ma wrote:
The macro VIRSH_DOMAIN_OPT_INTERFACE for domain option '--interface', The macro VIRSH_DOMAIN_OPT_MAC for domain option '--mac'.
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h index 0d59c579d4..ac05f983f9 100644 --- a/tools/virsh-domain.h +++ b/tools/virsh-domain.h @@ -39,3 +39,21 @@ typedef enum { VIR_ENUM_DECL(virshDomainHostnameSource);
extern const vshCmdDef domManagementCmds[]; + +#define VIRSH_DOMAIN_OPT_INTERFACE(_helpstr, oflags, cflags) \ + {.name = "interface", \ + .type = VSH_OT_STRING, \ + .flags = oflags, \ + .help = _helpstr, \ + .completer = virshDomainInterfaceCompleter, \ + .completer_flags = cflags, \ + } + +#define VIRSH_DOMAIN_OPT_MAC(_helpstr, oflags) \ + {.name = "mac", \ + .type = VSH_OT_STRING, \ + .flags = oflags, \ + .help = _helpstr, \ + .completer = virshDomainInterfaceCompleter, \ + .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, \ + }
So, if we had two distinct completers, as I suggested, then this would look a bit different and both macros could accept cflags. We could then print MAC addresses only of running/inactive/all interfaces, which could be helpful. I'm stopping my review here. I think you get the gist of my review. Looking forward to v2. Michal
On 2020-11-02 19:40, Michal Privoznik wrote:
On 11/2/20 9:26 AM, Lin Ma wrote:
The macro VIRSH_DOMAIN_OPT_INTERFACE for domain option '--interface', The macro VIRSH_DOMAIN_OPT_MAC for domain option '--mac'.
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h index 0d59c579d4..ac05f983f9 100644 --- a/tools/virsh-domain.h +++ b/tools/virsh-domain.h @@ -39,3 +39,21 @@ typedef enum { VIR_ENUM_DECL(virshDomainHostnameSource); extern const vshCmdDef domManagementCmds[]; + +#define VIRSH_DOMAIN_OPT_INTERFACE(_helpstr, oflags, cflags) \ + {.name = "interface", \ + .type = VSH_OT_STRING, \ + .flags = oflags, \ + .help = _helpstr, \ + .completer = virshDomainInterfaceCompleter, \ + .completer_flags = cflags, \ + } + +#define VIRSH_DOMAIN_OPT_MAC(_helpstr, oflags) \ + {.name = "mac", \ + .type = VSH_OT_STRING, \ + .flags = oflags, \ + .help = _helpstr, \ + .completer = virshDomainInterfaceCompleter, \ + .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, \ + }
So, if we had two distinct completers, as I suggested, then this would look a bit different and both macros could accept cflags. We could then print MAC addresses only of running/inactive/all interfaces, which could be helpful.
The idea about two distinct completers, It's about virsh-interface, But in virsh-domain, Here is only one interface completer virshDomainInterfaceCompleter and it can handle name/mac well with a cflags. IMO, The question is that we have two form of parameters: "interface" and "mac" in virsh-domain. That's why I created two macros. If we replace '--mac' by '--interface' in virsh-domain, Then things go easy: With cflags's help, We don't need 2 macros any more, But obviously it changed the existing virsh cmd usage, Probably It's not acceptable. So I created macro VIRSH_DOMAIN_OPT_MAC only for the parameter '--mac', The '--mac' should accept mac address string only, Does a cflags really make sense to this macro? The discussion about macros perhaps impact the review of the patchset v2, so I removed them in patchset v2, If we need them, I'll post them later.
I'm stopping my review here. I think you get the gist of my review. Looking forward to v2.
Thank you so much for your nice review! I just posted v2 to libvirt ml. Thank again, LLin
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain-monitor.c | 20 +++----------------- tools/virsh-domain-monitor.h | 1 + tools/virsh-domain.c | 14 ++------------ 3 files changed, 6 insertions(+), 29 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index e0491d48ac..313fdaf559 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -802,12 +802,7 @@ static const vshCmdInfo info_domif_getlink[] = { static const vshCmdOptDef opts_domif_getlink[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .completer = virshDomainInterfaceCompleter, - .help = N_("interface device (MAC Address)") - }, + VIRSH_DOMAIN_OPT_INTERFACE(N_("interface device (MAC Address)"), VSH_OFLAG_REQ, 0), {.name = "persistent", .type = VSH_OT_ALIAS, .help = "config" @@ -1138,12 +1133,7 @@ static const vshCmdInfo info_domifstat[] = { static const vshCmdOptDef opts_domifstat[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .completer = virshDomainInterfaceCompleter, - .help = N_("interface device specified by name or MAC Address") - }, + VIRSH_DOMAIN_OPT_INTERFACE(N_("interface device specified by name or MAC Address"), VSH_OFLAG_REQ, 0), {.name = NULL} }; @@ -2328,11 +2318,7 @@ static const vshCmdInfo info_domifaddr[] = { static const vshCmdOptDef opts_domifaddr[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), - {.name = "interface", - .type = VSH_OT_STRING, - .flags = VSH_OFLAG_NONE, - .completer = virshDomainInterfaceCompleter, - .help = N_("network interface name")}, + VIRSH_DOMAIN_OPT_INTERFACE(N_("network interface name"), VSH_OFLAG_NONE, 0), {.name = "full", .type = VSH_OT_BOOL, .flags = VSH_OFLAG_NONE, diff --git a/tools/virsh-domain-monitor.h b/tools/virsh-domain-monitor.h index 0de47c50c4..540c266274 100644 --- a/tools/virsh-domain-monitor.h +++ b/tools/virsh-domain-monitor.h @@ -21,6 +21,7 @@ #pragma once #include "virsh.h" +#include "virsh-domain.h" char *virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool title, unsigned int flags) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5b9970fdf5..d50fdbad26 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3053,12 +3053,7 @@ static const vshCmdInfo info_domif_setlink[] = { static const vshCmdOptDef opts_domif_setlink[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .completer = virshDomainInterfaceCompleter, - .help = N_("interface device (MAC Address)") - }, + VIRSH_DOMAIN_OPT_INTERFACE(N_("interface device (MAC Address)"), VSH_OFLAG_REQ, 0), {.name = "state", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, @@ -3225,12 +3220,7 @@ static const vshCmdInfo info_domiftune[] = { static const vshCmdOptDef opts_domiftune[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), - {.name = "interface", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .completer = virshDomainInterfaceCompleter, - .help = N_("interface device (MAC Address)") - }, + VIRSH_DOMAIN_OPT_INTERFACE(N_("interface device (MAC Address)"), VSH_OFLAG_REQ, 0), {.name = "inbound", .type = VSH_OT_STRING, .help = N_("control domain's incoming traffics") -- 2.26.0
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d50fdbad26..ed7307cdb6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12291,12 +12291,7 @@ static const vshCmdOptDef opts_detach_interface[] = { .flags = VSH_OFLAG_REQ, .help = N_("network interface type") }, - {.name = "mac", - .type = VSH_OT_STRING, - .completer = virshDomainInterfaceCompleter, - .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, - .help = N_("MAC address") - }, + VIRSH_DOMAIN_OPT_MAC(N_("MAC address"), VSH_OFLAG_REQ_OPT), VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, -- 2.26.0
The upcoming patches introduce completers into virsh-completer-domain.c, They will invoke the functions which are defined in virsh-domain.c, So these functions need to be declared in virsh-domain.h. Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-domain.c | 1 - tools/virsh-domain.h | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ed7307cdb6..65bbb6c646 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8900,7 +8900,6 @@ static const vshCmdOptDef opts_send_process_signal[] = { {.name = NULL} }; -VIR_ENUM_DECL(virDomainProcessSignal); VIR_ENUM_IMPL(virDomainProcessSignal, VIR_DOMAIN_PROCESS_SIGNAL_LAST, "nop", "hup", "int", "quit", "ill", /* 0-4 */ diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h index ac05f983f9..55938e54c0 100644 --- a/tools/virsh-domain.h +++ b/tools/virsh-domain.h @@ -57,3 +57,7 @@ extern const vshCmdDef domManagementCmds[]; .completer = virshDomainInterfaceCompleter, \ .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, \ } + +VIR_ENUM_DECL(virDomainProcessSignal); +VIR_ENUM_DECL(virDomainLifecycle); +VIR_ENUM_DECL(virDomainLifecycleAction); -- 2.26.0
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-domain.c | 20 ++++++++++++++++++++ tools/virsh-completer-domain.h | 4 ++++ tools/virsh-domain.c | 1 + 3 files changed, 25 insertions(+) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 305711151f..c1151764ff 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -672,3 +672,23 @@ virshDomainConsoleCompleter(vshControl *ctl, return g_steal_pointer(&tmp); } + +char ** +virshDomainSignalCompleter(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + size_t i = 0; + VIR_AUTOSTRINGLIST tmp = NULL; + + virCheckFlags(0, NULL); + + tmp = g_new0(char *, VIR_DOMAIN_PROCESS_SIGNAL_LAST + 1); + + for (i = 0; i < VIR_DOMAIN_PROCESS_SIGNAL_LAST; i++) { + const char *name = virDomainProcessSignalTypeToString(i); + tmp[i] = g_strdup(name); + } + + return g_steal_pointer(&tmp); +} diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h index 02fea2fe94..cdec66f23e 100644 --- a/tools/virsh-completer-domain.h +++ b/tools/virsh-completer-domain.h @@ -98,3 +98,7 @@ char ** virshDomainVcpulistViaAgentCompleter(vshControl *ctl, char ** virshDomainConsoleCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** virshDomainSignalCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 65bbb6c646..f69fbe41a5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8895,6 +8895,7 @@ static const vshCmdOptDef opts_send_process_signal[] = { {.name = "signame", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, + .completer = virshDomainSignalCompleter, .help = N_("the signal number or name") }, {.name = NULL} -- 2.26.0
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-domain.c | 20 ++++++++++++++++++++ tools/virsh-completer-domain.h | 4 ++++ tools/virsh-domain.c | 1 + 3 files changed, 25 insertions(+) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index c1151764ff..d8ea3d62b3 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -692,3 +692,23 @@ virshDomainSignalCompleter(vshControl *ctl G_GNUC_UNUSED, return g_steal_pointer(&tmp); } + +char ** +virshDomainLifecycleCompleter(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + size_t i = 0; + VIR_AUTOSTRINGLIST tmp = NULL; + + virCheckFlags(0, NULL); + + tmp = g_new0(char *, VIR_DOMAIN_LIFECYCLE_LAST + 1); + + for (i = 0; i < VIR_DOMAIN_LIFECYCLE_LAST; i++) { + const char *name = virDomainLifecycleTypeToString(i); + tmp[i] = g_strdup(name); + } + + return g_steal_pointer(&tmp); +} diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h index cdec66f23e..70f6e30947 100644 --- a/tools/virsh-completer-domain.h +++ b/tools/virsh-completer-domain.h @@ -102,3 +102,7 @@ char ** virshDomainConsoleCompleter(vshControl *ctl, char ** virshDomainSignalCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** virshDomainLifecycleCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f69fbe41a5..516e55e564 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5707,6 +5707,7 @@ static const vshCmdOptDef opts_setLifecycleAction[] = { {.name = "type", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, + .completer = virshDomainLifecycleCompleter, .help = N_("lifecycle type to modify") }, {.name = "action", -- 2.26.0
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-completer-domain.c | 20 ++++++++++++++++++++ tools/virsh-completer-domain.h | 4 ++++ tools/virsh-domain.c | 1 + 3 files changed, 25 insertions(+) diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index d8ea3d62b3..9d2f41f0eb 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -712,3 +712,23 @@ virshDomainLifecycleCompleter(vshControl *ctl G_GNUC_UNUSED, return g_steal_pointer(&tmp); } + +char ** +virshDomainLifecycleActionCompleter(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) +{ + size_t i = 0; + VIR_AUTOSTRINGLIST tmp = NULL; + + virCheckFlags(0, NULL); + + tmp = g_new0(char *, VIR_DOMAIN_LIFECYCLE_ACTION_LAST + 1); + + for (i = 0; i < VIR_DOMAIN_LIFECYCLE_ACTION_LAST; i++) { + const char *action = virDomainLifecycleActionTypeToString(i); + tmp[i] = g_strdup(action); + } + + return g_steal_pointer(&tmp); +} diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h index 70f6e30947..92c57bce75 100644 --- a/tools/virsh-completer-domain.h +++ b/tools/virsh-completer-domain.h @@ -106,3 +106,7 @@ char ** virshDomainSignalCompleter(vshControl *ctl, char ** virshDomainLifecycleCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); + +char ** virshDomainLifecycleActionCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 516e55e564..132cfbf637 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5713,6 +5713,7 @@ static const vshCmdOptDef opts_setLifecycleAction[] = { {.name = "action", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, + .completer = virshDomainLifecycleActionCompleter, .help = N_("lifecycle action to set") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, -- 2.26.0
participants (4)
-
Lin Ma -
Lin Ma -
Michal Privoznik -
Peter Krempa