[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Add virsh commands for virInterface* functions.



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]