[libvirt] [PATCH 0/3] Prevent removing a in-used static bridge and destroying a in-used virtual network
* Get the live state info of a virtual network through netcf in networkGetXMLDesc. * Add --system flag for net-dumpxml to show the live state info. * Check the live state info in net-destroy. * Add --force flag for net-destroy to forcibly destroy the virtual network. * Check the transient interfaces info in iface-unbridge. --- Lin Ma (3): bridge_driver: Return the live state info of a given virtual network virsh: prevent destroying a in-used network for net-destroy virsh: prevent removing a in-used bridge for iface-unbridge 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 + tools/virsh-interface.c | 25 ++++++- tools/virsh-network.c | 62 ++++++++++++++- tools/virsh.pod | 8 +- 8 files changed, 241 insertions(+), 10 deletions(-) -- 1.8.4
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@suse.com> --- 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(-) 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); 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)); + + if (virAsprintf(&if_xml_tmp, + "<interface type='bridge' name='%s'>" + "<start mode='none'/><bridge/></interface>", + bridge) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to generate temp xml for network")); + 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 + 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; + 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; + } + 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; + } + } + } + else { + def = network->def; + ret = virNetworkDefFormat(def, flags); + } + + cleanup: + 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 + 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 \ -- 1.8.4
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@suse.com> --- 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
On 02/02/2015 09:08 AM, 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@suse.com> --- 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(-)
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);
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)); + + if (virAsprintf(&if_xml_tmp, + "<interface type='bridge' name='%s'>" + "<start mode='none'/><bridge/></interface>", + bridge) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to generate temp xml for network")); + 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 + iface = ncf_define(driver->netcf, if_xml_tmp);
NACK. This is very much against what netcf (and the virInterface API) was intended to do, and doing this is nearly *guaranteed* to completely mess up NetworkManager, which monitors the ifcfg files that are created by ncf_define. This is one of the things that we regularly have to tell people that they *shouldn't* do - the bridge devices created by libvirt must never be referenced in a system config file; those are by definition persistent (even though you're deleting it after a short time), and immediately come in the radar of whatever host system daemon is managing the persistent network device configs. Also, I'm not sure I like having netcf called by the bridge driver. Anyway, if all you want to do is check whether or not a network is in use, an even more reasonable thing would be to simply look at the connections attribute of the net (i.e. def->connections) and check that it is 0 - every connection from a libvirt-managed guest is counted there (and it's re-computed every time libvirtd is restarted), and anybody who connects to a libvirt-created bridge without going through libvirt to do it deserves what they get.
+ 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; + 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; + } + 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; + } + } + } + else { + def = network->def; + ret = virNetworkDefFormat(def, flags); + } + + cleanup: + 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 + 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 \
On Mon, Feb 02, 2015 at 10:08:21PM +0800, 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@suse.com> --- 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(-)
So, IIUC, the point of this change is to display a list of all the TAP devices attached to a network. I'm not entirely convinced this is the right approach to take. Also do we want to limit this to reporting of devices inside bridges. It could be equally useful to see a list of macvtap devices associated with a physical NIC. Modifying the XML to include the list of VIFs is certainly one options, but I'm not sure it is necceessarily the best. As Laine points out this is a output only attribute - not something you can feed in with the XML when defining the network. Perhaps we want to have an actual API for this - a virNetworkListPorts which returns a list of associated devices (of any type) from guests. Or perhaps we should be reporting a list of virDomainPtr's that are attached to the network, or perhaps even both. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
"Daniel P. Berrange" <berrange@redhat.com> 2015-2-4 下午 17:17 >>> On Mon, Feb 02, 2015 at 10:08:21PM +0800, 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 --- 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(-)
So, IIUC, the point of this change is to display a list of all the TAP devices attached to a network. I'm not entirely convinced this is the right approach to take. Also do we want to limit this to reporting of devices inside bridges. It could be equally useful to see a list of macvtap devices associated with a physical NIC. Obtaining a list of macvtap devices associated with a physical NIC, It's a good idea. I intend to implement this api, also implement a new virsh subcommand to show these information to user. If you agreed with my thought, Then what are the proper api name and virsh subcommand name you suggest? and what about the output format?
Modifying the XML to include the list of VIFs is certainly one options, but I'm not sure it is necceessarily the best. As Laine points out this is a output only attribute - not something you can feed in with the XML when defining the network.
Perhaps we want to have an actual API for this - a virNetworkListPorts which returns a list of associated devices (of any type) from guests. Or perhaps we should be reporting a list of virDomainPtr's that are attached to the network, or perhaps even both.
On 05.02.2015 16:36, Lin Ma wrote:
"Daniel P. Berrange" <berrange@redhat.com> 2015-2-4 下午 17:17 >>> On Mon, Feb 02, 2015 at 10:08:21PM +0800, 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 --- 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(-)
So, IIUC, the point of this change is to display a list of all the TAP devices attached to a network. I'm not entirely convinced this is the right approach to take. Also do we want to limit this to reporting of devices inside bridges. It could be equally useful to see a list of macvtap devices associated with a physical NIC. Obtaining a list of macvtap devices associated with a physical NIC, It's a good idea. I intend to implement this api, also implement a new virsh subcommand to show these information to user. If you agreed with my thought, Then what are the proper api name and virsh subcommand name you suggest? and what about the output format?
Well, let see. We already have a way of expressing node devices (even virtual net interfaces): virNode* set of API. For instance: virsh # nodedev-dumpxml net_macvtap0_52_54_00_62_35_0f <device> <name>net_macvtap0_52_54_00_62_35_0f</name> <path>/sys/devices/virtual/net/macvtap0</path> <parent>computer</parent> <capability type='net'> <interface>macvtap0</interface> <address>52:54:00:62:35:0f</address> <link state='unknown'/> <capability type='80203'/> </capability> </device> I think it would be possible to extend macvtap XMLs with <parent/> element to reflect NIC over which macvtap is created. Although we are aiming at the opposite mapping. But then again - we don't have any <child/> element in nodedev XML yet. When trying to build a nodedev tree, users are required to enumerate nodes and look for <parent/> (or use virNodeDeviceGetParent()). Michal
On Mon, Feb 09, 2015 at 10:39:35AM +0100, Michal Privoznik wrote:
On 05.02.2015 16:36, Lin Ma wrote:
"Daniel P. Berrange" <berrange@redhat.com> 2015-2-4 下午 17:17 >>> On Mon, Feb 02, 2015 at 10:08:21PM +0800, 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 --- 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(-)
So, IIUC, the point of this change is to display a list of all the TAP devices attached to a network. I'm not entirely convinced this is the right approach to take. Also do we want to limit this to reporting of devices inside bridges. It could be equally useful to see a list of macvtap devices associated with a physical NIC. Obtaining a list of macvtap devices associated with a physical NIC, It's a good idea. I intend to implement this api, also implement a new virsh subcommand to show these information to user. If you agreed with my thought, Then what are the proper api name and virsh subcommand name you suggest? and what about the output format?
Well, let see. We already have a way of expressing node devices (even virtual net interfaces): virNode* set of API. For instance:
virsh # nodedev-dumpxml net_macvtap0_52_54_00_62_35_0f <device> <name>net_macvtap0_52_54_00_62_35_0f</name> <path>/sys/devices/virtual/net/macvtap0</path> <parent>computer</parent> <capability type='net'> <interface>macvtap0</interface> <address>52:54:00:62:35:0f</address> <link state='unknown'/> <capability type='80203'/> </capability> </device>
I think it would be possible to extend macvtap XMLs with <parent/> element to reflect NIC over which macvtap is created. Although we are aiming at the opposite mapping. But then again - we don't have any <child/> element in nodedev XML yet. When trying to build a nodedev tree, users are required to enumerate nodes and look for <parent/> (or use virNodeDeviceGetParent()).
Based on discussions elsewhere in the thread, I'm not sure there is a compelling reason for any API additions in this respect anymore. We are going to put all the logic directly in the virNetworkDestroyFlags method impl now. This addition about enumerating macvtap devices was only needed when virsh was trying todo the logic Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
* add --system flag for net-dumpxml to show information about the attached interfaces of the virtual network. * call virNetworkGetXMLDesc in cmdNetworkDestroy to get the live state info to check whether the virtual network is in use. * add --force flag for net-destroy to forcibly destroy the virtual network even if it's in use. Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-network.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++---- tools/virsh.pod | 8 +++++-- 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 5f8743c..17a5710 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -253,6 +253,10 @@ static const vshCmdOptDef opts_network_destroy[] = { .flags = VSH_OFLAG_REQ, .help = N_("network name or uuid") }, + {.name = "force", + .type = VSH_OT_BOOL, + .help = N_("Forcefully stop a given network") + }, {.name = NULL} }; @@ -260,20 +264,55 @@ static bool cmdNetworkDestroy(vshControl *ctl, const vshCmd *cmd) { virNetworkPtr network; - bool ret = true; + bool ret = false; const char *name; + char *br_xml = NULL; + xmlDocPtr xml_doc = NULL; + xmlXPathContextPtr ctxt = NULL; if (!(network = vshCommandOptNetwork(ctl, cmd, &name))) - return false; + goto cleanup; + +#ifdef WITH_NETCF + if (!vshCommandOptBool(cmd, "force")) { + if (virNetworkIsActive(network) <= 0) { + vshError(ctl, "%s", _("network is not active")); + goto cleanup; + } + if (!(br_xml = virNetworkGetXMLDesc(network, + VIR_NETWORK_XML_IFACE_ATTACHED))) + goto cleanup; + + if (!(xml_doc = virXMLParseStringCtxt(br_xml, + _("(bridge interface " + "definition)"), &ctxt))) { + vshError(ctl, "%s", _("Failed to parse network configuration")); + goto cleanup; + } + + if (virXPathNode("./bridge/interface[1]", ctxt) != NULL) { + vshError(ctl, "%s", _("The network is in use by guests")); + vshPrint(ctl, "%s", _("Use --force flag if you do want to do so")); + goto cleanup; + } + } +#endif if (virNetworkDestroy(network) == 0) { vshPrint(ctl, _("Network %s destroyed\n"), name); } else { vshError(ctl, _("Failed to destroy network %s"), name); - ret = false; + goto cleanup; } - virNetworkFree(network); + ret = true; + cleanup: + if(network) + virNetworkFree(network); + VIR_FREE(br_xml); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml_doc); + return ret; } @@ -300,6 +339,10 @@ static const vshCmdOptDef opts_network_dumpxml[] = { .type = VSH_OT_BOOL, .help = N_("network information of an inactive domain") }, + {.name = "system", + .type = VSH_OT_BOOL, + .help = N_("current live state information including attached interfaces") + }, {.name = NULL} }; @@ -311,6 +354,7 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd) char *dump; unsigned int flags = 0; int inactive; + bool system = false; if (!(network = vshCommandOptNetwork(ctl, cmd, NULL))) return false; @@ -319,6 +363,16 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd) if (inactive) flags |= VIR_NETWORK_XML_INACTIVE; + system = vshCommandOptBool(cmd, "system"); + if (system) { + flags = VIR_NETWORK_XML_IFACE_ATTACHED; + if (virNetworkIsActive(network) <= 0) { + vshError(ctl, "%s", _("--system only works on active network")); + virNetworkFree(network); + return false; + } + } + dump = virNetworkGetXMLDesc(network, flags); if (dump != NULL) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 804458e..3373ada 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2672,16 +2672,20 @@ to get a description of the XML network format used by libvirt. Define a persistent virtual network from an XML I<file>, the network is just defined but not instantiated (started). -=item B<net-destroy> I<network> +=item B<net-destroy> I<network> [I<--force>] Destroy (stop) a given transient or persistent virtual network specified by its name or UUID. This takes effect immediately. +If I<--force> is specified, then forcefully destroy the virtual +network even if it's in use. -=item B<net-dumpxml> I<network> [I<--inactive>] +=item B<net-dumpxml> I<network> [I<--inactive>] [I<--system>] Output the virtual network information as an XML dump to stdout. If I<--inactive> is specified, then physical functions are not expanded into their associated virtual functions. +If I<--system> is specified, then directly output the current +live state corresponding to this network from system. =item B<net-edit> I<network> -- 1.8.4
On 02.02.2015 15:08, Lin Ma wrote:
* add --system flag for net-dumpxml to show information about the attached interfaces of the virtual network.
* call virNetworkGetXMLDesc in cmdNetworkDestroy to get the live state info to check whether the virtual network is in use.
* add --force flag for net-destroy to forcibly destroy the virtual network even if it's in use.
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-network.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++---- tools/virsh.pod | 8 +++++-- 2 files changed, 64 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 5f8743c..17a5710 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -253,6 +253,10 @@ static const vshCmdOptDef opts_network_destroy[] = { .flags = VSH_OFLAG_REQ, .help = N_("network name or uuid") }, + {.name = "force", + .type = VSH_OT_BOOL, + .help = N_("Forcefully stop a given network") + }, {.name = NULL} };
@@ -260,20 +264,55 @@ static bool cmdNetworkDestroy(vshControl *ctl, const vshCmd *cmd) { virNetworkPtr network; - bool ret = true; + bool ret = false; const char *name; + char *br_xml = NULL; + xmlDocPtr xml_doc = NULL; + xmlXPathContextPtr ctxt = NULL;
if (!(network = vshCommandOptNetwork(ctl, cmd, &name))) - return false; + goto cleanup; + +#ifdef WITH_NETCF
No. Even if we (the client) were built without netcf support, we may be talking to a daemon built with the support and therefore we may want --force.
+ if (!vshCommandOptBool(cmd, "force")) { + if (virNetworkIsActive(network) <= 0) { + vshError(ctl, "%s", _("network is not active")); + goto cleanup; + } + if (!(br_xml = virNetworkGetXMLDesc(network, + VIR_NETWORK_XML_IFACE_ATTACHED))) + goto cleanup; + + if (!(xml_doc = virXMLParseStringCtxt(br_xml, + _("(bridge interface " + "definition)"), &ctxt))) { + vshError(ctl, "%s", _("Failed to parse network configuration")); + goto cleanup; + } + + if (virXPathNode("./bridge/interface[1]", ctxt) != NULL) { + vshError(ctl, "%s", _("The network is in use by guests")); + vshPrint(ctl, "%s", _("Use --force flag if you do want to do so")); + goto cleanup;
I must say I'm not a big fan of this. It's a change in behaviour, while previously users were able to destroy a running network (and cut off domains), it won't be possible using the same command anymore. On the other hand, preventing them from shooting in their own foot is nice thing to do.
+ } + } +#endif
if (virNetworkDestroy(network) == 0) { vshPrint(ctl, _("Network %s destroyed\n"), name); } else { vshError(ctl, _("Failed to destroy network %s"), name); - ret = false; + goto cleanup; }
- virNetworkFree(network); + ret = true; + cleanup: + if(network) + virNetworkFree(network); + VIR_FREE(br_xml); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml_doc); + return ret; }
@@ -300,6 +339,10 @@ static const vshCmdOptDef opts_network_dumpxml[] = { .type = VSH_OT_BOOL, .help = N_("network information of an inactive domain") }, + {.name = "system", + .type = VSH_OT_BOOL, + .help = N_("current live state information including attached interfaces") + }, {.name = NULL} };
@@ -311,6 +354,7 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd) char *dump; unsigned int flags = 0; int inactive; + bool system = false;
if (!(network = vshCommandOptNetwork(ctl, cmd, NULL))) return false; @@ -319,6 +363,16 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd) if (inactive) flags |= VIR_NETWORK_XML_INACTIVE;
+ system = vshCommandOptBool(cmd, "system"); + if (system) { + flags = VIR_NETWORK_XML_IFACE_ATTACHED; + if (virNetworkIsActive(network) <= 0) { + vshError(ctl, "%s", _("--system only works on active network")); + virNetworkFree(network); + return false; + } + } + dump = virNetworkGetXMLDesc(network, flags);
if (dump != NULL) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 804458e..3373ada 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2672,16 +2672,20 @@ to get a description of the XML network format used by libvirt. Define a persistent virtual network from an XML I<file>, the network is just defined but not instantiated (started).
-=item B<net-destroy> I<network> +=item B<net-destroy> I<network> [I<--force>]
Destroy (stop) a given transient or persistent virtual network specified by its name or UUID. This takes effect immediately. +If I<--force> is specified, then forcefully destroy the virtual +network even if it's in use.
-=item B<net-dumpxml> I<network> [I<--inactive>] +=item B<net-dumpxml> I<network> [I<--inactive>] [I<--system>]
Output the virtual network information as an XML dump to stdout. If I<--inactive> is specified, then physical functions are not expanded into their associated virtual functions. +If I<--system> is specified, then directly output the current +live state corresponding to this network from system.
=item B<net-edit> I<network>
Documenting new command arguments, that's nice. Michal
On 02/02/2015 09:08 AM, Lin Ma wrote:
* add --system flag for net-dumpxml to show information about the attached interfaces of the virtual network.
I don't like this extra flag - I think it is unnecessary. If we're going to more info to the status, then we can just always add it to the status. As to how it can be provided in the status XML (getting back to patch 1/3 which was using the netcf API in a way that I didn't like), better data can be gathered into a list in the network object as guests are created - this would be done in networkAllocateActualDevice() (and networkNotifyActualDevice()) at the points where connections is incremented for the network, and removed from the list in networkReleaseActualDevice() where connections is decremented. You'll notice that those functions get both the virDomainDefPtr and the virDomainNetDefPtr, so they have enough information to store the name of the domain as well as the namne of the interface. I recall a long time ago considering adding this information to the network XML, but I was dissuaded for some reason; unfortunately I don't remember why...
* call virNetworkGetXMLDesc in cmdNetworkDestroy to get the live state info to check whether the virtual network is in use.
* add --force flag for net-destroy to forcibly destroy the virtual network even if it's in use.
As Michal points out (and I pointed out when you sent your previous patchset), this does change behavior that some scripts may be dependent on. I too like the increased safety, but am concerned about breaking functionality. In cases like this, I always like to hear the learned advice of Dan Berrange - Daniel, what do you say about changing the default behavior of virsh net-destroy? Safe enough?
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-network.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++---- tools/virsh.pod | 8 +++++-- 2 files changed, 64 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 5f8743c..17a5710 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -253,6 +253,10 @@ static const vshCmdOptDef opts_network_destroy[] = { .flags = VSH_OFLAG_REQ, .help = N_("network name or uuid") }, + {.name = "force", + .type = VSH_OT_BOOL, + .help = N_("Forcefully stop a given network") + }, {.name = NULL} };
@@ -260,20 +264,55 @@ static bool cmdNetworkDestroy(vshControl *ctl, const vshCmd *cmd) { virNetworkPtr network; - bool ret = true; + bool ret = false; const char *name; + char *br_xml = NULL; + xmlDocPtr xml_doc = NULL; + xmlXPathContextPtr ctxt = NULL;
if (!(network = vshCommandOptNetwork(ctl, cmd, &name))) - return false; + goto cleanup; + +#ifdef WITH_NETCF + if (!vshCommandOptBool(cmd, "force")) { + if (virNetworkIsActive(network) <= 0) { + vshError(ctl, "%s", _("network is not active")); + goto cleanup; + } + if (!(br_xml = virNetworkGetXMLDesc(network, + VIR_NETWORK_XML_IFACE_ATTACHED))) + goto cleanup; + + if (!(xml_doc = virXMLParseStringCtxt(br_xml, + _("(bridge interface " + "definition)"), &ctxt))) { + vshError(ctl, "%s", _("Failed to parse network configuration")); + goto cleanup; + } + + if (virXPathNode("./bridge/interface[1]", ctxt) != NULL) { + vshError(ctl, "%s", _("The network is in use by guests")); + vshPrint(ctl, "%s", _("Use --force flag if you do want to do so")); + goto cleanup; + } + } +#endif
if (virNetworkDestroy(network) == 0) { vshPrint(ctl, _("Network %s destroyed\n"), name); } else { vshError(ctl, _("Failed to destroy network %s"), name); - ret = false; + goto cleanup; }
- virNetworkFree(network); + ret = true; + cleanup: + if(network) + virNetworkFree(network); + VIR_FREE(br_xml); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml_doc); + return ret; }
@@ -300,6 +339,10 @@ static const vshCmdOptDef opts_network_dumpxml[] = { .type = VSH_OT_BOOL, .help = N_("network information of an inactive domain") }, + {.name = "system", + .type = VSH_OT_BOOL, + .help = N_("current live state information including attached interfaces") + }, {.name = NULL} };
@@ -311,6 +354,7 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd) char *dump; unsigned int flags = 0; int inactive; + bool system = false;
if (!(network = vshCommandOptNetwork(ctl, cmd, NULL))) return false; @@ -319,6 +363,16 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd) if (inactive) flags |= VIR_NETWORK_XML_INACTIVE;
+ system = vshCommandOptBool(cmd, "system"); + if (system) { + flags = VIR_NETWORK_XML_IFACE_ATTACHED; + if (virNetworkIsActive(network) <= 0) { + vshError(ctl, "%s", _("--system only works on active network")); + virNetworkFree(network); + return false; + } + } + dump = virNetworkGetXMLDesc(network, flags);
if (dump != NULL) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 804458e..3373ada 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2672,16 +2672,20 @@ to get a description of the XML network format used by libvirt. Define a persistent virtual network from an XML I<file>, the network is just defined but not instantiated (started).
-=item B<net-destroy> I<network> +=item B<net-destroy> I<network> [I<--force>]
Destroy (stop) a given transient or persistent virtual network specified by its name or UUID. This takes effect immediately. +If I<--force> is specified, then forcefully destroy the virtual +network even if it's in use.
-=item B<net-dumpxml> I<network> [I<--inactive>] +=item B<net-dumpxml> I<network> [I<--inactive>] [I<--system>]
Output the virtual network information as an XML dump to stdout. If I<--inactive> is specified, then physical functions are not expanded into their associated virtual functions. +If I<--system> is specified, then directly output the current +live state corresponding to this network from system.
=item B<net-edit> I<network>
On Tue, Feb 03, 2015 at 08:21:42PM -0500, Laine Stump wrote:
On 02/02/2015 09:08 AM, Lin Ma wrote:
* add --system flag for net-dumpxml to show information about the attached interfaces of the virtual network.
I don't like this extra flag - I think it is unnecessary. If we're going to more info to the status, then we can just always add it to the status.
As to how it can be provided in the status XML (getting back to patch 1/3 which was using the netcf API in a way that I didn't like), better data can be gathered into a list in the network object as guests are created - this would be done in networkAllocateActualDevice() (and networkNotifyActualDevice()) at the points where connections is incremented for the network, and removed from the list in networkReleaseActualDevice() where connections is decremented. You'll notice that those functions get both the virDomainDefPtr and the virDomainNetDefPtr, so they have enough information to store the name of the domain as well as the namne of the interface.
I recall a long time ago considering adding this information to the network XML, but I was dissuaded for some reason; unfortunately I don't remember why...
* call virNetworkGetXMLDesc in cmdNetworkDestroy to get the live state info to check whether the virtual network is in use.
* add --force flag for net-destroy to forcibly destroy the virtual network even if it's in use.
As Michal points out (and I pointed out when you sent your previous patchset), this does change behavior that some scripts may be dependent on. I too like the increased safety, but am concerned about breaking functionality. In cases like this, I always like to hear the learned advice of Dan Berrange - Daniel, what do you say about changing the default behavior of virsh net-destroy? Safe enough?
I don't think it is acceptable to change the behaviour. The 'destroy' commands are unambiguous about the fact that they are a forceful operation that will always succeed. Note that for the 'virsh destroy' command we added a 'graceful' option, thus preserving default behaviour
-=item B<net-dumpxml> I<network> [I<--inactive>] +=item B<net-dumpxml> I<network> [I<--inactive>] [I<--system>]
Output the virtual network information as an XML dump to stdout. If I<--inactive> is specified, then physical functions are not expanded into their associated virtual functions. +If I<--system> is specified, then directly output the current +live state corresponding to this network from system.
This description of the 'system' flag just makes it sound like it is the same as not specifying the --inactive flag. ie If the network is running, the XML we emit is always the live XML state Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
By checking transient interfaces, It obtains the live information of attached interfaces to see if the bridge is in use. Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-interface.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 5f848b6..ff40be0 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -1040,11 +1040,11 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) const char *br_name; char *if_type = NULL, *if_name = NULL; bool nostart = false; - char *br_xml = NULL; + char *br_xml = NULL, *br_xml_transient_if = NULL; xmlChar *if_xml = NULL; int if_xml_size; - xmlDocPtr xml_doc = NULL; - xmlXPathContextPtr ctxt = NULL; + xmlDocPtr xml_doc = NULL, xml_doc_transient_if = NULL; + xmlXPathContextPtr ctxt = NULL, ctxt_transient_if = NULL; xmlNodePtr top_node, if_node, cur; /* Get a handle to the original device */ @@ -1103,6 +1103,22 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) goto cleanup; } + /* verify whether there is any transient interface attached to bridge. */ + if (!(br_xml_transient_if = virInterfaceGetXMLDesc(br_handle, 0))) + goto cleanup; + + if (!(xml_doc_transient_if = virXMLParseStringCtxt(br_xml_transient_if, + _("(bridge interface definition)"), + &ctxt_transient_if))) { + vshError(ctl, _("Failed to parse configuration of %s"), br_name); + goto cleanup; + } + + if (virXPathNode("./bridge/interface[2]", ctxt_transient_if) != NULL) { + vshError(ctl, "%s", _("The bridge is in use by transient interfaces")); + goto cleanup; + } + /* Change the type and name of the outer/master interface to * the type/name of the attached slave interface. */ @@ -1198,10 +1214,13 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) virInterfaceFree(br_handle); VIR_FREE(if_xml); VIR_FREE(br_xml); + VIR_FREE(br_xml_transient_if); VIR_FREE(if_type); VIR_FREE(if_name); xmlXPathFreeContext(ctxt); + xmlXPathFreeContext(ctxt_transient_if); xmlFreeDoc(xml_doc); + xmlFreeDoc(xml_doc_transient_if); return ret; } -- 1.8.4
On 02.02.2015 15:08, Lin Ma wrote:
By checking transient interfaces, It obtains the live information of attached interfaces to see if the bridge is in use.
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-interface.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
Technically, this is a v2 to a previous patch (I mildly recall seeing something like this in the past).
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 5f848b6..ff40be0 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -1040,11 +1040,11 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) const char *br_name; char *if_type = NULL, *if_name = NULL; bool nostart = false; - char *br_xml = NULL; + char *br_xml = NULL, *br_xml_transient_if = NULL; xmlChar *if_xml = NULL; int if_xml_size; - xmlDocPtr xml_doc = NULL; - xmlXPathContextPtr ctxt = NULL; + xmlDocPtr xml_doc = NULL, xml_doc_transient_if = NULL; + xmlXPathContextPtr ctxt = NULL, ctxt_transient_if = NULL; xmlNodePtr top_node, if_node, cur;
/* Get a handle to the original device */ @@ -1103,6 +1103,22 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+ /* verify whether there is any transient interface attached to bridge. */ + if (!(br_xml_transient_if = virInterfaceGetXMLDesc(br_handle, 0))) + goto cleanup; + + if (!(xml_doc_transient_if = virXMLParseStringCtxt(br_xml_transient_if, + _("(bridge interface definition)"), + &ctxt_transient_if))) { + vshError(ctl, _("Failed to parse configuration of %s"), br_name); + goto cleanup; + } + + if (virXPathNode("./bridge/interface[2]", ctxt_transient_if) != NULL) { + vshError(ctl, "%s", _("The bridge is in use by transient interfaces")); + goto cleanup; + } + /* Change the type and name of the outer/master interface to * the type/name of the attached slave interface. */ @@ -1198,10 +1214,13 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) virInterfaceFree(br_handle); VIR_FREE(if_xml); VIR_FREE(br_xml); + VIR_FREE(br_xml_transient_if); VIR_FREE(if_type); VIR_FREE(if_name); xmlXPathFreeContext(ctxt); + xmlXPathFreeContext(ctxt_transient_if); xmlFreeDoc(xml_doc); + xmlFreeDoc(xml_doc_transient_if); return ret; }
ACK. I'll merge this tomorrow (unless somebody beats me). Michal
On 02/03/2015 11:39 AM, Michal Privoznik wrote:
On 02.02.2015 15:08, Lin Ma wrote:
By checking transient interfaces, It obtains the live information of attached interfaces to see if the bridge is in use.
Signed-off-by: Lin Ma <lma@suse.com> --- tools/virsh-interface.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) Technically, this is a v2 to a previous patch (I mildly recall seeing something like this in the past).
It looks to be the same patch, just with reference to a private bug report removed, and preceded with the check for net-destroy (since I had said in my response to the original patch that the behavior of iface-unbridge was made to be similar to net-destroy, and that my opinion was that either neither should be changed, or both).
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 5f848b6..ff40be0 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -1040,11 +1040,11 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) const char *br_name; char *if_type = NULL, *if_name = NULL; bool nostart = false; - char *br_xml = NULL; + char *br_xml = NULL, *br_xml_transient_if = NULL; xmlChar *if_xml = NULL; int if_xml_size; - xmlDocPtr xml_doc = NULL; - xmlXPathContextPtr ctxt = NULL; + xmlDocPtr xml_doc = NULL, xml_doc_transient_if = NULL; + xmlXPathContextPtr ctxt = NULL, ctxt_transient_if = NULL; xmlNodePtr top_node, if_node, cur;
/* Get a handle to the original device */ @@ -1103,6 +1103,22 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+ /* verify whether there is any transient interface attached to bridge. */ + if (!(br_xml_transient_if = virInterfaceGetXMLDesc(br_handle, 0))) + goto cleanup; + + if (!(xml_doc_transient_if = virXMLParseStringCtxt(br_xml_transient_if, + _("(bridge interface definition)"), + &ctxt_transient_if))) { + vshError(ctl, _("Failed to parse configuration of %s"), br_name); + goto cleanup; + } + + if (virXPathNode("./bridge/interface[2]", ctxt_transient_if) != NULL) { + vshError(ctl, "%s", _("The bridge is in use by transient interfaces")); + goto cleanup; + } + /* Change the type and name of the outer/master interface to * the type/name of the attached slave interface. */ @@ -1198,10 +1214,13 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) virInterfaceFree(br_handle); VIR_FREE(if_xml); VIR_FREE(br_xml); + VIR_FREE(br_xml_transient_if); VIR_FREE(if_type); VIR_FREE(if_name); xmlXPathFreeContext(ctxt); + xmlXPathFreeContext(ctxt_transient_if); xmlFreeDoc(xml_doc); + xmlFreeDoc(xml_doc_transient_if); return ret; }
ACK. I'll merge this tomorrow (unless somebody beats me).
Please don't push it as is. I think the behavior of iface-unbridge should match whatever is done to net-destroy (if anything). If the change is made, it should be made to both at the same time, and this one should also have a --force option to allow overriding the extra check, as patch 2/3 does for net-destroy. (BTW, I'm still curious about Suse's use of netcf, as there have been no updates to the Suse driver in netcf since the initial port was imported several years ago. Are there downstream changes that can be sent upstream?)
Laine Stump wrote:
(BTW, I'm still curious about Suse's use of netcf, as there have been no updates to the Suse driver in netcf since the initial port was imported several years ago. Are there downstream changes that can be sent upstream?)
SUSE doesn't use netcf. We were unable to find a volunteer from the networking team to maintain the initial port. Instead they provided a netcf interface on top of their technologies, which at least allows us to use the libvirt netcf backend. Regards, Jim
On 02/04/2015 10:57 AM, Jim Fehlig wrote:
Laine Stump wrote:
(BTW, I'm still curious about Suse's use of netcf, as there have been no updates to the Suse driver in netcf since the initial port was imported several years ago. Are there downstream changes that can be sent upstream?)
SUSE doesn't use netcf. We were unable to find a volunteer from the networking team to maintain the initial port. Instead they provided a netcf interface on top of their technologies, which at least allows us to use the libvirt netcf backend.
Ah yes, I vaguely remember this. They basically made a copy of the APIs in netcf.h, then provided their own library under it. This worries me, because it means that the two are in danger of getting out of sync in some subtle (or not so subtle) way. For example, netcf recently added support for reporting the link state (operstate from sysfs) and speed in its status output. If this other library for SUSE had been implemented as a backend in netcf, Suse would have gotten that addition more or less for free (since it is linux-specific, but not distro-specific), but instead people will complain that this feature (already used/expected in the output of libvirt virsh iface-dumpxml) doesn't work on SUSE until someone adds it to the SUSE work-alike library. (Even worse would be if something was implemented in both libraries, but behaved slightly differently in one). You know, there is no hard requirement that the current "SUSE" backend in netcf be the one that stays there - that backend was written by a person for (I think) some embedded system that was using a SUSE-based self-built distro, and they have long since abandoned it. If you have any interest in putting in a backend based on the code your network people have already written for their own library (either replacing the current SUSE backend, or in addition to it), I'd be happy to help with it. It may turn out to be more trouble than it's worth, but it's at least worth considering. Can you point me at the package that is implementing the netcf API, so I can poke around a bit?
Laine Stump wrote:
On 02/04/2015 10:57 AM, Jim Fehlig wrote:
Laine Stump wrote:
(BTW, I'm still curious about Suse's use of netcf, as there have been no updates to the Suse driver in netcf since the initial port was imported several years ago. Are there downstream changes that can be sent upstream?)
SUSE doesn't use netcf. We were unable to find a volunteer from the networking team to maintain the initial port. Instead they provided a netcf interface on top of their technologies, which at least allows us to use the libvirt netcf backend.
Ah yes, I vaguely remember this. They basically made a copy of the APIs in netcf.h, then provided their own library under it. This worries me, because it means that the two are in danger of getting out of sync in some subtle (or not so subtle) way.
I suspect they already have -:(. If I've found the correct package devel area, there haven't been any updates in six months.
For example, netcf recently added support for reporting the link state (operstate from sysfs) and speed in its status output. If this other library for SUSE had been implemented as a backend in netcf, Suse would have gotten that addition more or less for free (since it is linux-specific, but not distro-specific), but instead people will complain that this feature (already used/expected in the output of libvirt virsh iface-dumpxml) doesn't work on SUSE until someone adds it to the SUSE work-alike library. (Even worse would be if something was implemented in both libraries, but behaved slightly differently in one).
You know, there is no hard requirement that the current "SUSE" backend in netcf be the one that stays there - that backend was written by a person for (I think) some embedded system that was using a SUSE-based self-built distro, and they have long since abandoned it. If you have any interest in putting in a backend based on the code your network people have already written for their own library (either replacing the current SUSE backend, or in addition to it), I'd be happy to help with it. It may turn out to be more trouble than it's worth, but it's at least worth considering.
Can you point me at the package that is implementing the netcf API, so I can poke around a bit?
I'm pretty sure netcontrol is maintained here https://build.opensuse.org/package/show/network:utilities/netcontrol I'll let the networking folks know about this thread too. Regards, Jim
Laine Stump <laine@laine.org> 2015-2-4 下午 17:12 >>> On 02/03/2015 11:39 AM, Michal Privoznik wrote: On 02.02.2015 15:08, Lin Ma wrote: By checking transient interfaces, It obtains the live information of attached interfaces to see if the bridge is in use.
Signed-off-by: Lin Ma --- tools/virsh-interface.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) Technically, this is a v2 to a previous patch (I mildly recall seeing something like this in the past).
It looks to be the same patch, just with reference to a private bug report removed, and preceded with the check for net-destroy (since I had said in my response to the original patch that the behavior of iface-unbridge was made to be similar to net-destroy, and that my opinion was that either neither should be changed, or both). It seems like that we decided to keep the original net-destroy behaviour, Then let's keep iface-unbridge's too.
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 5f848b6..ff40be0 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -1040,11 +1040,11 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) const char *br_name; char *if_type = NULL, *if_name = NULL; bool nostart = false; - char *br_xml = NULL; + char *br_xml = NULL, *br_xml_transient_if = NULL; xmlChar *if_xml = NULL; int if_xml_size; - xmlDocPtr xml_doc = NULL; - xmlXPathContextPtr ctxt = NULL; + xmlDocPtr xml_doc = NULL, xml_doc_transient_if = NULL; + xmlXPathContextPtr ctxt = NULL, ctxt_transient_if = NULL; xmlNodePtr top_node, if_node, cur;
/* Get a handle to the original device */ @@ -1103,6 +1103,22 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+ /* verify whether there is any transient interface attached to bridge. */ + if (!(br_xml_transient_if = virInterfaceGetXMLDesc(br_handle, 0))) + goto cleanup; + + if (!(xml_doc_transient_if = virXMLParseStringCtxt(br_xml_transient_if, + _("(bridge interface definition)"), + &ctxt_transient_if))) { + vshError(ctl, _("Failed to parse configuration of %s"), br_name); + goto cleanup; + } + + if (virXPathNode("./bridge/interface[2]", ctxt_transient_if) != NULL) { + vshError(ctl, "%s", _("The bridge is in use by transient interfaces")); + goto cleanup; + } + /* Change the type and name of the outer/master interface to * the type/name of the attached slave interface. */ @@ -1198,10 +1214,13 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) virInterfaceFree(br_handle); VIR_FREE(if_xml); VIR_FREE(br_xml); + VIR_FREE(br_xml_transient_if); VIR_FREE(if_type); VIR_FREE(if_name); xmlXPathFreeContext(ctxt); + xmlXPathFreeContext(ctxt_transient_if); xmlFreeDoc(xml_doc); + xmlFreeDoc(xml_doc_transient_if); return ret; }
ACK. I'll merge this tomorrow (unless somebody beats me).
Please don't push it as is. I think the behavior of iface-unbridge should match whatever is done to net-destroy (if anything). If the change is made, it should be made to both at the same time, and this one should also have a --force option to allow overriding the extra check, as patch 2/3 does for net-destroy. As Daniel points out, destroy is The net-destroy already shows to user that it's a forceful operation, So we don't need a --force option, then iface-unbridge either.
On 02.02.2015 15:08, Lin Ma wrote:
* Get the live state info of a virtual network through netcf in networkGetXMLDesc. * Add --system flag for net-dumpxml to show the live state info. * Check the live state info in net-destroy. * Add --force flag for net-destroy to forcibly destroy the virtual network. * Check the transient interfaces info in iface-unbridge.
--- Lin Ma (3): bridge_driver: Return the live state info of a given virtual network virsh: prevent destroying a in-used network for net-destroy virsh: prevent removing a in-used bridge for iface-unbridge
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 + tools/virsh-interface.c | 25 ++++++- tools/virsh-network.c | 62 ++++++++++++++- tools/virsh.pod | 8 +- 8 files changed, 241 insertions(+), 10 deletions(-)
So I've spent some time thinking about this. I don't really like the idea of producing completely different XML (it doesn't even have <network/> as its root element!). But I see what you're trying to achieve. How about putting the bridged interfaces into the network definition (on request signalized by a flag, of course). I think we've come to a point where we need to introduce live and config XML (like we have for domains). We already have that to some extent, but not truly. On the other hand, looking into virNetworkDefFormat(), there's this '@connections' attribute to the <network/> element which is a nonzero integer if the network is in use. Won't that suffice for what you're trying to achieve? Michal
On 02/03/2015 11:47 AM, Michal Privoznik wrote:
On 02.02.2015 15:08, Lin Ma wrote:
* Get the live state info of a virtual network through netcf in networkGetXMLDesc. * Add --system flag for net-dumpxml to show the live state info. * Check the live state info in net-destroy. * Add --force flag for net-destroy to forcibly destroy the virtual network. * Check the transient interfaces info in iface-unbridge.
--- Lin Ma (3): bridge_driver: Return the live state info of a given virtual network virsh: prevent destroying a in-used network for net-destroy virsh: prevent removing a in-used bridge for iface-unbridge
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 + tools/virsh-interface.c | 25 ++++++- tools/virsh-network.c | 62 ++++++++++++++- tools/virsh.pod | 8 +- 8 files changed, 241 insertions(+), 10 deletions(-)
So I've spent some time thinking about this. I don't really like the idea of producing completely different XML (it doesn't even have <network/> as its root element!). But I see what you're trying to achieve. How about putting the bridged interfaces into the network definition (on request signalized by a flag, of course).
I think that if we're going to add the list of connected guest devices to a network's status, that it should just always be there - adding a new flag for every little bit of different status sets a bad precedent and sets us up to have an infinitely increasing number of flags. Like I mentioned in my response to one of the patches, I think this information is better gathered and maintained by the bridge driver as guests connect to / disconnect from a network; this avoids the necessity of dragging netcf into the picture and allows much better information - the name of the domain using each interface can be included.
I think we've come to a point where we need to introduce live and config XML (like we have for domains). We already have that to some extent, but not truly.
I think the situation is more that we have live and config XML, but there may be a small problem here and there. As a matter of fact, a long time ago virsh wasn't using the live XML to edit a network, and this was causing problems (e.g. if you tried to edit a network twice without restarting it, the second time you would lose all the changes from the first edit).
On the other hand, looking into virNetworkDefFormat(), there's this '@connections' attribute to the <network/> element which is a nonzero integer if the network is in use. Won't that suffice for what you're trying to achieve?
In my opinion, yes - that's exactly the type of thing it was intended for :-)
On Wed, Feb 04, 2015 at 02:21:18AM -0500, Laine Stump wrote:
On 02/03/2015 11:47 AM, Michal Privoznik wrote:
On 02.02.2015 15:08, Lin Ma wrote:
* Get the live state info of a virtual network through netcf in networkGetXMLDesc. * Add --system flag for net-dumpxml to show the live state info. * Check the live state info in net-destroy. * Add --force flag for net-destroy to forcibly destroy the virtual network. * Check the transient interfaces info in iface-unbridge.
--- Lin Ma (3): bridge_driver: Return the live state info of a given virtual network virsh: prevent destroying a in-used network for net-destroy virsh: prevent removing a in-used bridge for iface-unbridge
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 + tools/virsh-interface.c | 25 ++++++- tools/virsh-network.c | 62 ++++++++++++++- tools/virsh.pod | 8 +- 8 files changed, 241 insertions(+), 10 deletions(-)
So I've spent some time thinking about this. I don't really like the idea of producing completely different XML (it doesn't even have <network/> as its root element!). But I see what you're trying to achieve. How about putting the bridged interfaces into the network definition (on request signalized by a flag, of course).
I think that if we're going to add the list of connected guest devices to a network's status, that it should just always be there - adding a new flag for every little bit of different status sets a bad precedent and sets us up to have an infinitely increasing number of flags.
Like I mentioned in my response to one of the patches, I think this information is better gathered and maintained by the bridge driver as guests connect to / disconnect from a network; this avoids the necessity of dragging netcf into the picture and allows much better information - the name of the domain using each interface can be included.
I think we should consider whether we should expose it as an explicit API too, rather than just stuffing it into the XML as a way to avoid the work of defining an API. The XML is generally describing the configuration of the virtual network. This isn't really configuration information, but rather a reporting about usage of the network, so it isn't a clearly compelling thing to put in the XML.
I think we've come to a point where we need to introduce live and config XML (like we have for domains). We already have that to some extent, but not truly.
I think the situation is more that we have live and config XML, but there may be a small problem here and there. As a matter of fact, a long time ago virsh wasn't using the live XML to edit a network, and this was causing problems (e.g. if you tried to edit a network twice without restarting it, the second time you would lose all the changes from the first edit).
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Feb 04, 2015 at 02:21:18AM -0500, Laine Stump wrote:
On 02/03/2015 11:47 AM, Michal Privoznik wrote:
On 02.02.2015 15:08, Lin Ma wrote:
* Get the live state info of a virtual network through netcf in networkGetXMLDesc. * Add --system flag for net-dumpxml to show the live state info. * Check the live state info in net-destroy. * Add --force flag for net-destroy to forcibly destroy the virtual network. * Check the transient interfaces info in iface-unbridge.
--- Lin Ma (3): bridge_driver: Return the live state info of a given virtual network virsh: prevent destroying a in-used network for net-destroy virsh: prevent removing a in-used bridge for iface-unbridge
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 + tools/virsh-interface.c | 25 ++++++- tools/virsh-network.c | 62 ++++++++++++++- tools/virsh.pod | 8 +- 8 files changed, 241 insertions(+), 10 deletions(-)
So I've spent some time thinking about this. I don't really like the idea of producing completely different XML (it doesn't even have <network/> as its root element!). But I see what you're trying to achieve. How about putting the bridged interfaces into the network definition (on request signalized by a flag, of course).
I think that if we're going to add the list of connected guest devices to a network's status, that it should just always be there - adding a new flag for every little bit of different status sets a bad precedent and sets us up to have an infinitely increasing number of flags.
Like I mentioned in my response to one of the patches, I think this information is better gathered and maintained by the bridge driver as guests connect to / disconnect from a network; this avoids the necessity of dragging netcf into the picture and allows much better information - the name of the domain using each interface can be included.
You know if this checking for guests were done in the virNetworkDestroy API implementation in src/network/bridge_driver.c, instead of in virsh, then we would not need any of this code for extending the XML nor parsing it in virsh. We could simply check the number of connections which is something we have directly available internally. So 95% of this patch series would go away Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 02/04/2015 09:58 AM, Daniel P. Berrange wrote:
On 02/03/2015 11:47 AM, Michal Privoznik wrote:
On 02.02.2015 15:08, Lin Ma wrote:
* Get the live state info of a virtual network through netcf in networkGetXMLDesc. * Add --system flag for net-dumpxml to show the live state info. * Check the live state info in net-destroy. * Add --force flag for net-destroy to forcibly destroy the virtual network. * Check the transient interfaces info in iface-unbridge.
--- Lin Ma (3): bridge_driver: Return the live state info of a given virtual network virsh: prevent destroying a in-used network for net-destroy virsh: prevent removing a in-used bridge for iface-unbridge
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 + tools/virsh-interface.c | 25 ++++++- tools/virsh-network.c | 62 ++++++++++++++- tools/virsh.pod | 8 +- 8 files changed, 241 insertions(+), 10 deletions(-)
So I've spent some time thinking about this. I don't really like the idea of producing completely different XML (it doesn't even have <network/> as its root element!). But I see what you're trying to achieve. How about putting the bridged interfaces into the network definition (on request signalized by a flag, of course). I think that if we're going to add the list of connected guest devices to a network's status, that it should just always be there - adding a new flag for every little bit of different status sets a bad precedent and sets us up to have an infinitely increasing number of flags.
Like I mentioned in my response to one of the patches, I think this information is better gathered and maintained by the bridge driver as guests connect to / disconnect from a network; this avoids the necessity of dragging netcf into the picture and allows much better information - the name of the domain using each interface can be included. You know if this checking for guests were done in the virNetworkDestroy API implementation in src/network/bridge_driver.c, instead of in virsh, then we would not need any of this code for extending the XML nor parsing it in virsh. We could simply check the number of connections which is something we have directly available internally. So 95% of this
On Wed, Feb 04, 2015 at 02:21:18AM -0500, Laine Stump wrote: patch series would go away
Except that the virNetworkDestroy() API was created before we realized it was a good idea to have a flags argument to every new API. So that would require creating a new virNetworkDestroyFlags() API (and the first flag to be defined would be VIR_NETWORK_DESTROY_GRACEFUL). Note that a network's connections count *is* returned in the status XML, so virsh could check that and it would work even when connecting to older servers. I think I prefer your idea, though, because it is more consistent with what is done for the virDomain API (it won't work when talking to older servers though, and virsh will have to be written to fall back to virNetworkDestroy() when the server is too old to have virNetworkDestroyFlags().
On Wed, Feb 04, 2015 at 12:39:56PM -0500, Laine Stump wrote:
On 02/04/2015 09:58 AM, Daniel P. Berrange wrote:
On 02/03/2015 11:47 AM, Michal Privoznik wrote:
On 02.02.2015 15:08, Lin Ma wrote:
* Get the live state info of a virtual network through netcf in networkGetXMLDesc. * Add --system flag for net-dumpxml to show the live state info. * Check the live state info in net-destroy. * Add --force flag for net-destroy to forcibly destroy the virtual network. * Check the transient interfaces info in iface-unbridge.
--- Lin Ma (3): bridge_driver: Return the live state info of a given virtual network virsh: prevent destroying a in-used network for net-destroy virsh: prevent removing a in-used bridge for iface-unbridge
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 + tools/virsh-interface.c | 25 ++++++- tools/virsh-network.c | 62 ++++++++++++++- tools/virsh.pod | 8 +- 8 files changed, 241 insertions(+), 10 deletions(-)
So I've spent some time thinking about this. I don't really like the idea of producing completely different XML (it doesn't even have <network/> as its root element!). But I see what you're trying to achieve. How about putting the bridged interfaces into the network definition (on request signalized by a flag, of course). I think that if we're going to add the list of connected guest devices to a network's status, that it should just always be there - adding a new flag for every little bit of different status sets a bad precedent and sets us up to have an infinitely increasing number of flags.
Like I mentioned in my response to one of the patches, I think this information is better gathered and maintained by the bridge driver as guests connect to / disconnect from a network; this avoids the necessity of dragging netcf into the picture and allows much better information - the name of the domain using each interface can be included. You know if this checking for guests were done in the virNetworkDestroy API implementation in src/network/bridge_driver.c, instead of in virsh, then we would not need any of this code for extending the XML nor parsing it in virsh. We could simply check the number of connections which is something we have directly available internally. So 95% of this
On Wed, Feb 04, 2015 at 02:21:18AM -0500, Laine Stump wrote: patch series would go away
Except that the virNetworkDestroy() API was created before we realized it was a good idea to have a flags argument to every new API. So that would require creating a new virNetworkDestroyFlags() API (and the first flag to be defined would be VIR_NETWORK_DESTROY_GRACEFUL).
Note that a network's connections count *is* returned in the status XML, so virsh could check that and it would work even when connecting to older servers. I think I prefer your idea, though, because it is more consistent with what is done for the virDomain API (it won't work when talking to older servers though, and virsh will have to be written to fall back to virNetworkDestroy() when the server is too old to have virNetworkDestroyFlags().
Yep, adding a virNetworkDestroyFlags() is no big deal - we'll inevitably need it at some point. There's probably an argument for identifying all our remaining APIs which lack a flags parameter and fixing it once and for all :-) Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
"Daniel P. Berrange" <berrange@redhat.com> 2015-2-4 下午 23:10 >>> On Wed, Feb 04, 2015 at 02:21:18AM -0500, Laine Stump wrote: On 02/03/2015 11:47 AM, Michal Privoznik wrote: On 02.02.2015 15:08, Lin Ma wrote: * Get the live state info of a virtual network through netcf in networkGetXMLDesc. * Add --system flag for net-dumpxml to show the live state info. * Check the live state info in net-destroy. * Add --force flag for net-destroy to forcibly destroy the virtual network. * Check the transient interfaces info in iface-unbridge.
--- Lin Ma (3): bridge_driver: Return the live state info of a given virtual network virsh: prevent destroying a in-used network for net-destroy virsh: prevent removing a in-used bridge for iface-unbridge
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 + tools/virsh-interface.c | 25 ++++++- tools/virsh-network.c | 62 ++++++++++++++- tools/virsh.pod | 8 +- 8 files changed, 241 insertions(+), 10 deletions(-)
So I've spent some time thinking about this. I don't really like the idea of producing completely different XML (it doesn't even have as its root element!). But I see what you're trying to achieve. How about putting the bridged interfaces into the network definition (on request signalized by a flag, of course).
I think that if we're going to add the list of connected guest devices to a network's status, that it should just always be there - adding a new flag for every little bit of different status sets a bad precedent and sets us up to have an infinitely increasing number of flags.
Like I mentioned in my response to one of the patches, I think this information is better gathered and maintained by the bridge driver as guests connect to / disconnect from a network; this avoids the necessity of dragging netcf into the picture and allows much better information - the name of the domain using each interface can be included.
You know if this checking for guests were done in the virNetworkDestroy API implementation in src/network/bridge_driver.c, instead of in virsh, then we would not need any of this code for extending the XML nor parsing it in virsh. We could simply check the number of connections which is something we have directly available internally. So 95% of this patch series would go away :-C I'm sad...... :-), Anyway, Because we keep the original behaviour of net-destroy, So we keep iface-unbridge no changes as well, Then droping
this patch series makes sense.
participants (5)
-
Daniel P. Berrange -
Jim Fehlig -
Laine Stump -
Lin Ma -
Michal Privoznik