[PATCH] Don't delete the physical interface config when defining a vlan.

[Laine Stump]

Prior to defining a new interface, drv_define() would previously call
rm_all_interfaces(), which would look for all nodes called "interface"
at every level, and remove the config for any existing interface with
the same name. This is necessary when defining a bridge or a bond
(since those interfaces will now be slaves of the toplevel
bridge/bond, and their config will be created anew along with the
toplevel), but is the wrong thing to do for vlans - not only does the
physical interface associated with a vlan have an existence of its
own, multiple vlans can be associated with the same physical
interface, and the act of defining a vlan *does not* re-create the
physical interface config. The result of this was dismal failure when
attempting to define a vlan interface - the associated physical
interface would disappear.

To solve this problem, rm_all_interfaces() has been replaced with
rm_interface_and_slaves(), which removes the config for the toplevel
interface, the checks to see if it is a bridge or a bond; if so, it
also removes the config for any interfaces slaved to this
bridge/bond.

Because a bridge may enslave a bond, which in turn may enslave
physical interfaces, this function was made recursive.
---
 src/drv_initscripts.c |  100 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 78 insertions(+), 22 deletions(-)

diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
index d5bbd44..389dbb6 100644
--- a/src/drv_initscripts.c
+++ b/src/drv_initscripts.c
@@ -936,34 +936,90 @@ static void rm_interface(struct netcf *ncf, const char *name) {
     FREE(path);
 }
 
-/* Remove all interfaces and their slaves mentioned in NCF_XML.  We need to
- * remove interfaces one by one when we define an interface, since what
- * will become a subinterface may not be related to the new toplevel
- * interface, and calling RM_INTERFACE on the toplevel interface is
- * therefore not enough.
+
+/* rm_interface_and_slaves_helper()
+ *
+ * Given a context pointing to an "interface" node, delete the config
+ * for the interface with the name of this node's "name" property.
+ * Then, if this node's type is "bridge" or "bond", cycle
through
+ * all "type"/interface subnodes and call this function to delete
+ * their config as well.
  */
-static void rm_all_interfaces(struct netcf *ncf, xmlDocPtr ncf_xml) {
-    xmlXPathContextPtr context = NULL;
-	xmlXPathObjectPtr obj = NULL;
+static void rm_interface_and_slaves_helper(struct netcf *ncf,
+                                           xmlDocPtr ncf_xml,
+                                           xmlXPathContextPtr ctxt) {
+    xmlNodePtr save_node = ctxt->node;
+    xmlXPathObjectPtr obj = NULL;
+    xmlChar *name = NULL, *type = NULL;
+    char *if_node_exp = NULL;
+
+    xmlNodeSetPtr ns;
+
+    /* remove the top-level interface */
+    name = xmlGetProp(ctxt->node, BAD_CAST "name");
+    ERR_NOMEM(name == NULL, ncf);
+    rm_interface(ncf, (char *) name);
+    ERR_BAIL(ncf);
 
-	context = xmlXPathNewContext(ncf_xml);
-    ERR_NOMEM(context == NULL, ncf);
+    type = xmlGetProp(ctxt->node, BAD_CAST "type");
+    ERR_NOMEM(name == NULL, ncf);
 
-	obj = xmlXPathEvalExpression(BAD_CAST "//interface", context);
-    ERR_NOMEM(obj == NULL, ncf);
+    if (xmlStrEqual(type, BAD_CAST "bridge")
+        || xmlStrEqual(type, BAD_CAST "bond")) {
 
+        /* get a list of all interface subnodes under
+         * this interface's "type" node.
+         * eg "./bond/interface"
+         */
+
+        int r = xasprintf(&if_node_exp, "./%s/interface", (char *)type);
+        ERR_NOMEM(r < 0, ncf);
+
+        obj = xmlXPathEvalExpression(BAD_CAST if_node_exp, ctxt);
+        ERR_NOMEM(obj == NULL, ncf);
+        ns = obj->nodesetval;
+        ERR_NOMEM(ns == NULL, ncf);
+
+        /* iterate through them all, removing them as we go */
+        for (int ii=0; ii < ns->nodeNr; ii++) {
+
+            ERR_NOMEM(ns->nodeTab[ii] == NULL, ncf);
+
+            /* make this interface the toplevel node & recurse */
+            ctxt->node = ns->nodeTab[ii];
+            rm_interface_and_slaves_helper(ncf, ncf_xml, ctxt);
+            ERR_BAIL(ncf);
+        }
+    }
 
-    xmlNodeSetPtr ns = obj->nodesetval;
-    for (int i=0; i < ns->nodeNr; i++) {
-        xmlChar *name = xmlGetProp(ns->nodeTab[i], BAD_CAST "name");
-        ERR_NOMEM(name == NULL, ncf);
-        rm_interface(ncf, (char *) name);
-        xmlFree(name);
-        ERR_BAIL(ncf);
-	}
  error:
     xmlXPathFreeObject(obj);
-    xmlXPathFreeContext(context);
+    FREE(if_node_exp);
+    xmlFree(name);
+    xmlFree(type);
+    ctxt->node = save_node;
+}
+
+/* Remove the toplevel interface, and all slaves recursively.  We need to
+ * remove interfaces one by one when we define an interface, since what
+ * will become a subinterface may not be related to the new toplevel
+ * interface, and calling RM_INTERFACE on the toplevel interface is
+ * therefore not enough.
+ *
+ * This function just sets up the initial context and calls the helper
+ * function above, which does the real work.
+ */
+static void rm_interface_and_slaves(struct netcf *ncf, xmlDocPtr ncf_xml) {
+    xmlXPathContextPtr ctxt;
+
+    ctxt = xmlXPathNewContext(ncf_xml);
+    ERR_NOMEM(ctxt == NULL, ncf);
+
+    ctxt->node = xmlDocGetRootElement(ncf_xml);
+    if (xmlStrEqual(ctxt->node->name, BAD_CAST "interface"))
+        rm_interface_and_slaves_helper(ncf, ncf_xml, ctxt);
+error:
+    xmlXPathFreeContext(ctxt);
 }
 
 /* Dig through interface NAME and all its subinterfaces for bonds
@@ -1017,7 +1073,7 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str)
{
     name = device_name_from_xml(ncf, ncf_xml);
     ERR_COND_BAIL(name == NULL, ncf, EINTERNAL);
 
-    rm_all_interfaces(ncf, ncf_xml);
+    rm_interface_and_slaves(ncf, ncf_xml);
     ERR_BAIL(ncf);
 
     aug_xml = apply_stylesheet(ncf, ncf->driver->get, ncf_xml);
-- 
1.7.1.1

[PATCHv2] Don't delete the physical interface config when defining a vlan.

[Laine Stump]

This is a much simpler way of solving the problem, suggested by David
Lutterkort.

Prior to defining a new interface, drv_define() would previously call
rm_all_interfaces(), which would look for all nodes called "interface"
at every level, and remove the config for any existing interface with
the same name. This is necessary when defining a bridge or a bond
(since those interfaces will now be slaves of the toplevel
bridge/bond, and their config will be created anew along with the
toplevel), but is the wrong thing to do for vlans - not only does the
physical interface associated with a vlan have an existence of its
own, multiple vlans can be associated with the same physical
interface, and the act of defining a vlan *does not* re-create the
physical interface config. The result of this was dismal failure when
attempting to define a vlan interface - the associated physical
interface would disappear.

The solution is to beef up the expression used when finding all
interfaces inside the XML to exclude any whose parent is a "vlan"
node.
---
 src/drv_initscripts.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
index d5bbd44..c0bcfd5 100644
--- a/src/drv_initscripts.c
+++ b/src/drv_initscripts.c
@@ -949,7 +949,9 @@ static void rm_all_interfaces(struct netcf *ncf, xmlDocPtr ncf_xml) {
 	context = xmlXPathNewContext(ncf_xml);
     ERR_NOMEM(context == NULL, ncf);
 
-	obj = xmlXPathEvalExpression(BAD_CAST "//interface", context);
+    obj = xmlXPathEvalExpression(BAD_CAST
+                                 "//interface[count(parent::vlan) = 0]",
+                                 context);
     ERR_NOMEM(obj == NULL, ncf);
 
 
-- 
1.7.1.1

[PATCHv2] Don't delete the physical interface config when defining a vlan.

[David Lutterkort]

On Tue, 2010-07-27 at 14:43 -0400, Laine Stump wrote:
>  This is a much simpler way of solving the problem, suggested by
> David
>  Lutterkort.
>  
>  Prior to defining a new interface, drv_define() would previously call
>  rm_all_interfaces(), which would look for all nodes called "interface"
>  at every level, and remove the config for any existing interface with
>  the same name. This is necessary when defining a bridge or a bond
>  (since those interfaces will now be slaves of the toplevel
>  bridge/bond, and their config will be created anew along with the
>  toplevel), but is the wrong thing to do for vlans - not only does the
>  physical interface associated with a vlan have an existence of its
>  own, multiple vlans can be associated with the same physical
>  interface, and the act of defining a vlan *does not* re-create the
>  physical interface config. The result of this was dismal failure when
>  attempting to define a vlan interface - the associated physical
>  interface would disappear.
>  
>  The solution is to beef up the expression used when finding all
>  interfaces inside the XML to exclude any whose parent is a "vlan"
>  node. 
ACK

David