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