Re: [PATCH 1/3] bridge_driver: Return the live state info of a given virtual network

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

 



On 02.02.2015 15:08, Lin Ma wrote:
> It constructs a temporary static config of the network, Obtains all of
> attached interfaces information through netcf, Then removes the config.
> 
> Signed-off-by: Lin Ma <lma@xxxxxxxx>
> ---
>  include/libvirt/libvirt-network.h    |   1 +
>  src/Makefile.am                      |   3 +
>  src/network/bridge_driver.c          | 141 ++++++++++++++++++++++++++++++++++-
>  src/network/bridge_driver_platform.h |   7 ++
>  tests/Makefile.am                    |   4 +
>  5 files changed, 155 insertions(+), 1 deletion(-)

We don't use TABs. If you ran 'make syntax-check' it would catch the errors.

> 
> diff --git a/include/libvirt/libvirt-network.h b/include/libvirt/libvirt-network.h
> index 308f27f..9b09546 100644
> --- a/include/libvirt/libvirt-network.h
> +++ b/include/libvirt/libvirt-network.h
> @@ -30,6 +30,7 @@
>  
>  typedef enum {
>      VIR_NETWORK_XML_INACTIVE = (1 << 0), /* dump inactive network information */
> +    VIR_NETWORK_XML_IFACE_ATTACHED = (1 << 1), /* dump current live state */
>  } virNetworkXMLFlags;
>  
>  /**
> diff --git a/src/Makefile.am b/src/Makefile.am
> index b41c6d4..d22ae7e 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1412,6 +1412,9 @@ if WITH_NETWORK
>  noinst_LTLIBRARIES += libvirt_driver_network_impl.la
>  libvirt_driver_network_la_SOURCES =
>  libvirt_driver_network_la_LIBADD = libvirt_driver_network_impl.la
> +if WITH_NETCF
> +libvirt_driver_network_la_LIBADD += $(NETCF_LIBS)
> +endif WITH_NETCF
>  if WITH_DRIVER_MODULES
>  mod_LTLIBRARIES += libvirt_driver_network.la
>  libvirt_driver_network_la_LIBADD += ../gnulib/lib/libgnu.la \
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index c56e8f2..1e49e2e 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -3333,8 +3333,17 @@ static char *networkGetXMLDesc(virNetworkPtr net,
>      virNetworkObjPtr network;
>      virNetworkDefPtr def;
>      char *ret = NULL;
> +#ifdef WITH_NETCF
> +    struct netcf_if *iface = NULL;
> +    char *bridge = NULL;
> +    char *if_xml_tmp = NULL;
> +    xmlDocPtr xml = NULL;
> +    xmlXPathContextPtr ctxt = NULL;
> +    xmlXPathObjectPtr obj = NULL;
> +#endif
>  
> -    virCheckFlags(VIR_NETWORK_XML_INACTIVE, NULL);
> +    virCheckFlags(VIR_NETWORK_XML_INACTIVE |
> +                  VIR_NETWORK_XML_IFACE_ATTACHED, NULL);

So even if libvirt is build without netcf the flag is still supported? I
don't think so. I mean, it's okay that it's here. But I'm missing the
check in '#else' branch.

>  
>      if (!(network = networkObjFromNetwork(net)))
>          return ret;
> @@ -3342,6 +3351,135 @@ static char *networkGetXMLDesc(virNetworkPtr net,
>      if (virNetworkGetXMLDescEnsureACL(net->conn, network->def) < 0)
>          goto cleanup;
>  
> +#ifdef WITH_NETCF
> +    if ((flags & VIR_NETWORK_XML_INACTIVE) && network->newDef) {
> +        def = network->newDef;
> +        ret = virNetworkDefFormat(def, flags);
> +    }
> +    else if (flags & VIR_NETWORK_XML_IFACE_ATTACHED) {
> +        if (!(network->def->bridge)) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("network '%s' does not have a bridge name."),
> +                           network->def->name);
> +            goto cleanup;
> +        }
> +        ignore_value(VIR_STRDUP(bridge, network->def->bridge));

No. If strdup()-ing fails ...

> +
> +        if (virAsprintf(&if_xml_tmp,
> +                        "<interface type='bridge' name='%s'>"
> +                        "<start mode='none'/><bridge/></interface>",
> +                        bridge) < 0) {

.. this won't produce any meaningful XML.

> +         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("failed to generate temp xml for network"));

Indentation's off.

> +            goto cleanup;
> +        }
> +
> +        if (ncf_init(&driver->netcf, NULL) != 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("failed to init netcf"));
> +           goto cleanup;
> +        }
> +
> +        // create a temp bridge configuration file

We tend to put comments into /* */

> +        iface = ncf_define(driver->netcf, if_xml_tmp);
> +        if (!iface) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("failed to define the temp bridge %s"), bridge);
> +            ncf_close(driver->netcf);
> +            goto cleanup;
> +        }
> +
> +        ret = ncf_if_xml_state(iface);
> +        if (!ret) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("could not get bridge XML description"));
> +            ncf_if_free(iface);
> +            ncf_close(driver->netcf);
> +            goto cleanup;
> +        }
> +
> +        // remove the temp bridge configuration file
> +        if (ncf_if_undefine(iface) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("failed to undefine the temp bridge %s"), bridge);
> +            ncf_if_free(iface);
> +            ncf_close(driver->netcf);
> +            ret = NULL;

This is not the correct way to free @ret. You need to use VIR_FREE()
otherwise @ret is leaked.

> +            goto cleanup;
> +        }
> +        ncf_if_free(iface);
> +        ncf_close(driver->netcf);
> +
> +        // remove the dummp tap interface section from the result
> +        if (network->def->mac_specified) {
> +            char *macTapIfName = networkBridgeDummyNicName(network->def->bridge);
> +            if (macTapIfName) {
> +                char mac[VIR_MAC_STRING_BUFLEN];
> +                xmlNodePtr cur = NULL, matchNode = NULL;
> +                xmlChar *br_xml = NULL;
> +                int br_xml_size;
> +                char buf[64];
> +	            size_t i;
> +                int diff_mac;
> +                virMacAddrFormat(&network->def->mac, mac);
> +                snprintf(buf, sizeof(buf), "./bridge/interface[@name='%s']",
> +                         macTapIfName);
> +                if (!(xml = virXMLParseStringCtxt(ret,
> +                                                  _("(bridge interface "
> +                                                    "definition)"), &ctxt))) {
> +                    virReportError(VIR_ERR_INTERNAL_ERROR,
> +                                   ("Failed to parse network configuration"));
> +                    VIR_FREE(macTapIfName);
> +                    ret = NULL;
> +                    goto cleanup;
> +                }
> +                obj = xmlXPathEval(BAD_CAST buf, ctxt);
> +                if (obj == NULL || obj->type != XPATH_NODESET ||
> +                    obj->nodesetval == NULL || obj->nodesetval->nodeNr == 0) {
> +	                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                                   _("No interface found whose name is %s"),
> +                                   macTapIfName);
> +                    VIR_FREE(macTapIfName);
> +	                ret = NULL;
> +                    goto cleanup;
> +                }


You've meant virXPathNodeSet(), right?

> +                VIR_FREE(macTapIfName);
> +                for (i = 0; i < obj->nodesetval->nodeNr; i++) {
> +                    cur = obj->nodesetval->nodeTab[i]->children;
> +                    while (cur != NULL) {
> +                        if (cur->type == XML_ELEMENT_NODE &&
> +                            xmlStrEqual(cur->name, BAD_CAST "mac")) {
> +                            char *tmp_mac = virXMLPropString(cur, "address");
> +                            diff_mac = virMacAddrCompare(tmp_mac, mac);
> +                            VIR_FREE(tmp_mac);
> +                            if (!diff_mac) {
> +                                matchNode = obj->nodesetval->nodeTab[i];
> +                                xmlUnlinkNode(matchNode);
> +                                break;
> +                            }
> +                        }
> +                        cur = cur->next;
> +                    }
> +                }
> +                xmlDocDumpMemory(xml, &br_xml, &br_xml_size);
> +                ret = (char *)br_xml;

Are you aware, that this will produce XML that is not acceptable by
libvirt back, right? E.g. for my 'default' network I get this:

# net-dumpxml --system default
<?xml version="1.0"?>
<interface name="virbr0" type="bridge">
  <bridge>

  </bridge>
  <protocol family="ipv4">
    <ip address="192.168.122.1" prefix="24"/>
  </protocol>
</interface>

This is not a libvirt network XML.

> +            }
> +        }
> +    }
> +    else {
> +        def = network->def;
> +        ret = virNetworkDefFormat(def, flags);
> +    }
> +
> + cleanup:

This introduces yet another label of the very same name we already have.
I think these labels can be merged into one.

> +    xmlXPathFreeObject(obj);
> +    xmlXPathFreeContext(ctxt);
> +    xmlFreeDoc(xml);
> +    VIR_FREE(if_xml_tmp);
> +    VIR_FREE(bridge);
> +    if (network)
> +        virNetworkObjUnlock(network);
> +#else
>      if ((flags & VIR_NETWORK_XML_INACTIVE) && network->newDef)
>          def = network->newDef;
>      else
> @@ -3352,6 +3490,7 @@ static char *networkGetXMLDesc(virNetworkPtr net,
>   cleanup:
>      if (network)
>          virNetworkObjUnlock(network);
> +#endif
>      return ret;
>  }
>  
> diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
> index 1e8264a..43ea1c3 100644
> --- a/src/network/bridge_driver_platform.h
> +++ b/src/network/bridge_driver_platform.h
> @@ -24,6 +24,9 @@
>  #ifndef __VIR_BRIDGE_DRIVER_PLATFORM_H__
>  # define __VIR_BRIDGE_DRIVER_PLATFORM_H__
>  
> +#ifdef WITH_NETCF
> +# include <netcf.h>
> +#endif
>  # include "internal.h"
>  # include "virthread.h"
>  # include "virdnsmasq.h"
> @@ -34,6 +37,10 @@
>  struct _virNetworkDriverState {
>      virMutex lock;
>  
> +#ifdef WITH_NETCF
> +    struct netcf *netcf;
> +#endif
> +

So what's the good having netcf here, in the driver, if it's used
nowhere but dumpXML()? Moreover, if we init and close the netcf handle
on each function call?

>      virNetworkObjList networks;
>  
>      char *networkConfigDir;
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 938270c..0662337 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -67,6 +67,10 @@ LDADDS = \
>  	$(GNULIB_LIBS) \
>  	../src/libvirt.la
>  
> +if WITH_NETCF
> +LDADDS += $(NETCF_LIBS)
> +endif WITH_NETCF
> +
>  EXTRA_DIST =		\
>  	bhyvexml2argvdata \
>  	bhyvexml2xmloutdata \
> 

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]