On Fri, Jun 12, 2009 at 10:25:19AM -0400, Laine Stump wrote:
I think this is complete (can't really try it out until the test driver is running), but have a few questions about it:
1) Does the command set look complete?
2) Do you agree with the naming (using "if-", and following the libvirt convention for if-create/if-destroy, rather than the fedora convention (if-up/if-down)?
Sigh. I wish we hadn't called our original commands 'net-XXX' as it is a rather misleading name. I think perhaps I'd go slightly longer and suggest 'iface-XXX' as the prefix.
3) I'm all for eliminating unnecessary copy/paste in code, but the way that cmdNetworkEdit() and cmdPoolEdit() are created (by awking/seding virsh.c to create a modified version of the cmdEdit function that lives in a separate .c which is #included by virsh.c) seems just a bit convoluted to me. I've made it one step worse for cmdInterfaceEdit because virInterfaceDefineXML has an extra argument (flags) that virDomainDefineXML doesn't. What are your opinions? Should I continue on in the tradition for consistency's sake (as I've done in this patch), or manually put cmdInterfaceEdit() directly into virsh.c?
Personally I think this auto-generation of network/pool edit commands is overkill, and has actually caused bugs in the past.
--- src/Makefile.am | 18 +++- src/virsh.c | 398 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 414 insertions(+), 2 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index d4f7ea1..7601611 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -585,7 +585,7 @@ virsh_LDADD = \ ../gnulib/lib/libgnu.la \ $(VIRSH_LIBS) virsh_CFLAGS = $(COVERAGE_CFLAGS) $(READLINE_CFLAGS) $(NUMACTL_CFLAGS) -BUILT_SOURCES = virsh-net-edit.c virsh-pool-edit.c libvirt.syms +BUILT_SOURCES = virsh-net-edit.c virsh-if-edit.c virsh-pool-edit.c libvirt.syms
virsh-net-edit.c: virsh.c Makefile.am rm -f $@-tmp @@ -602,6 +602,22 @@ virsh-net-edit.c: virsh.c Makefile.am rm -f $@ mv $@-tmp $@
+virsh-if-edit.c: virsh.c Makefile.am + rm -f $@-tmp + echo '/* Automatically generated from: $^ */' > $@-tmp + echo 'static int' >> $@-tmp + awk '/^cmdEdit/, /^}/' $< \ + | sed -e 's/domain/interface/g' \ + -e 's/Domain/Interface/g' \ + -e 's/cmdEdit/cmdInterfaceEdit/g' \ + -e 's/dom/iface/g' \ + -e 's/int flags.*/int flags = 0;/g' \ + -e 's/DefineXML\(.*\))/DefineXML\1, 0)/' \ + >> $@-tmp + chmod a-w $@-tmp + rm -f $@ + mv $@-tmp $@ + virsh-pool-edit.c: virsh.c Makefile.am rm -f $@-tmp echo '/* Automatically generated from: $^ */' > $@-tmp diff --git a/src/virsh.c b/src/virsh.c index f5248d9..6fd090c 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -228,6 +228,7 @@ static int vshCommandOptBool(const vshCmd *cmd, const char *name); #define VSH_BYID (1 << 1) #define VSH_BYUUID (1 << 2) #define VSH_BYNAME (1 << 3) +#define VSH_BYMAC (1 << 4)
static virDomainPtr vshCommandOptDomainBy(vshControl *ctl, const vshCmd *cmd, char **name, int flag); @@ -244,6 +245,14 @@ static virNetworkPtr vshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd, vshCommandOptNetworkBy(_ctl, _cmd, _name, \ VSH_BYUUID|VSH_BYNAME)
+static virInterfacePtr vshCommandOptInterfaceBy(vshControl *ctl, const vshCmd *cmd, + char **name, int flag); + +/* default is lookup by Name and MAC */ +#define vshCommandOptInterface(_ctl, _cmd, _name) \ + vshCommandOptInterfaceBy(_ctl, _cmd, _name, \ + VSH_BYMAC|VSH_BYNAME) + static virStoragePoolPtr vshCommandOptPoolBy(vshControl *ctl, const vshCmd *cmd, const char *optname, char **name, int flag);
@@ -2955,6 +2964,325 @@ cmdNetworkUuid(vshControl *ctl, const vshCmd *cmd) }
+/**************************************************************************/ +/* + * "if-list" command + */ +static const vshCmdInfo info_interface_list[] = { + {"help", gettext_noop("list physical host interfaces")}, + {"desc", gettext_noop("Returns list of physical host interfaces.")}, + {NULL, NULL} +}; + +static int +cmdInterfaceList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + int ifCount = 0, i; + char **ifNames = NULL; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + ifCount = virConnectNumOfInterfaces(ctl->conn); + if (ifCount < 0) { + vshError(ctl, FALSE, "%s", _("Failed to list interfaces")); + return FALSE; + } + if (ifCount) { + ifNames = vshMalloc(ctl, sizeof(char *) * ifCount); + + if ((ifCount = virConnectListInterfaces(ctl->conn, + ifNames, ifCount)) < 0) { + vshError(ctl, FALSE, "%s", _("Failed to list interfaces")); + free(ifNames); + return FALSE; + } + + qsort(&ifNames[0], ifCount, sizeof(char *), namesorter); + } + + vshPrintExtra(ctl, "%-20s %-30s\n", _("Name"), _("MAC Address")); + vshPrintExtra(ctl, "--------------------------------------------------\n"); + + for (i = 0; i < ifCount; i++) { + virInterfacePtr iface = virInterfaceLookupByName(ctl->conn, ifNames[i]); + + /* this kind of work with interfaces is not atomic operation */ + if (!iface) { + free(ifNames[i]); + continue; + } + + vshPrint(ctl, "%-20s %-30s\n", + virInterfaceGetName(iface), + virInterfaceGetMACString(iface)); + virInterfaceFree(iface); + free(ifNames[i]); + } + free(ifNames); + return TRUE; +}
This one should also take the flags --inactive / --all to allow selection of offline NICs vs online NICs. Which reminds me that the current public APIs are missing the equivalent for listing inactive interfaces. eg, virConnectNumOfDefinedInterfaces and virConnectListDefinedInterfaces
+ +/* + * "if-name" command + */ +static const vshCmdInfo info_interface_name[] = { + {"help", gettext_noop("convert an interface MAC address to interface name")}, + {"desc", ""}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_interface_name[] = { + {"interface", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("interface mac")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdInterfaceName(vshControl *ctl, const vshCmd *cmd) +{ + virInterfacePtr iface; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + if (!(iface = vshCommandOptInterfaceBy(ctl, cmd, NULL, + VSH_BYMAC))) + return FALSE; + + vshPrint(ctl, "%s\n", virInterfaceGetName(iface)); + virInterfaceFree(iface); + return TRUE; +} + +/* + * "if-mac" command + */ +static const vshCmdInfo info_interface_mac[] = { + {"help", gettext_noop("convert an interface name to interface MAC address")}, + {"desc", ""}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_interface_mac[] = { + {"interface", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("interface name")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdInterfaceMAC(vshControl *ctl, const vshCmd *cmd) +{ + virInterfacePtr iface; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + if (!(iface = vshCommandOptInterfaceBy(ctl, cmd, NULL, + VSH_BYNAME))) + return FALSE; + + vshPrint(ctl, "%s\n", virInterfaceGetMACString(iface)); + virInterfaceFree(iface); + return TRUE; +} + +/* + * "if-dumpxml" command + */ +static const vshCmdInfo info_interface_dumpxml[] = { + {"help", gettext_noop("interface information in XML")}, + {"desc", gettext_noop("Output the physical host interface information as an XML dump to stdout.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_interface_dumpxml[] = { + {"interface", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("interface name or MAC address")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdInterfaceDumpXML(vshControl *ctl, const vshCmd *cmd) +{ + virInterfacePtr iface; + int ret = TRUE; + char *dump; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + if (!(iface = vshCommandOptInterface(ctl, cmd, NULL))) + return FALSE; + + dump = virInterfaceGetXMLDesc(iface, 0); + if (dump != NULL) { + printf("%s", dump); + free(dump); + } else { + ret = FALSE; + } + + virInterfaceFree(iface); + return ret; +} + +/* + * "if-define" command + */ +static const vshCmdInfo info_interface_define[] = { + {"help", gettext_noop("define (but don't start) a physical host interface from an XML file")}, + {"desc", gettext_noop("Define a physical host interface.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_interface_define[] = { + {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("file containing an XML interface description")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdInterfaceDefine(vshControl *ctl, const vshCmd *cmd) +{ + virInterfacePtr interface; + char *from; + int found; + int ret = TRUE; + char *buffer; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + from = vshCommandOptString(cmd, "file", &found); + if (!found) + return FALSE; + + if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) + return FALSE; + + interface = virInterfaceDefineXML(ctl->conn, buffer, 0); + free (buffer); + + if (interface != NULL) { + vshPrint(ctl, _("Interface %s defined from %s\n"), + virInterfaceGetName(interface), from); + } else { + vshError(ctl, FALSE, _("Failed to define interface from %s"), from); + ret = FALSE; + } + return ret; +} + +/* + * "if-undefine" command + */ +static const vshCmdInfo info_interface_undefine[] = { + {"help", gettext_noop("undefine a physical host interface (remove it from configuration)")}, + {"desc", gettext_noop("undefine an interface.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_interface_undefine[] = { + {"interface", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("interface name or MAC address")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdInterfaceUndefine(vshControl *ctl, const vshCmd *cmd) +{ + virInterfacePtr iface; + int ret = TRUE; + char *name; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + if (!(iface = vshCommandOptInterface(ctl, cmd, &name))) + return FALSE; + + if (virInterfaceUndefine(iface) == 0) { + vshPrint(ctl, _("Interface %s undefined\n"), name); + } else { + vshError(ctl, FALSE, _("Failed to undefine interface %s"), name); + ret = FALSE; + } + + virInterfaceFree(iface); + return ret; +} + +/* + * "if-create" command + */ +static const vshCmdInfo info_interface_create[] = { + {"help", gettext_noop("create a physical host interface (enable it / \"if-up\")")}, + {"desc", gettext_noop("create a physical host interface.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_interface_create[] = { + {"interface", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("interface name or MAC address")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdInterfaceCreate(vshControl *ctl, const vshCmd *cmd) +{ + virInterfacePtr iface; + int ret = TRUE; + char *name; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + if (!(iface = vshCommandOptInterface(ctl, cmd, &name))) + return FALSE; + + if (virInterfaceCreate(iface, 0) == 0) { + vshPrint(ctl, _("Interface %s created\n"), name); + } else { + vshError(ctl, FALSE, _("Failed to create interface %s"), name); + ret = FALSE; + } + + virInterfaceFree(iface); + return ret; +}
This command should be called 'start' for consistency with other commands. 'create' is the command name we use for creating a transient object, rather than starting a persistent object. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|