[PATCH 0/3] Eliminate errors and config-based ips from ncf_if_xml_state

[Laine Stump]

The originalpatch for ncf_if_xml_state would search for the first ip
node within the ipv4 protocol node, and replace it with the results of
SIOCGIFADDR (or sometimes just add a new one on.)

Now we remove all ip addresses sourced from the config, even if we
can't get any replacements from the device.

[PATCH 1/3] Remove config-origined ip addrs from ncf_if_xml_state output.

[Laine Stump]

The output of ncf_if_xml_state will now contain only IP addresses
retrieved directly from the device.
---
 src/dutil.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/dutil.c b/src/dutil.c
index e2d05b8..dd75768 100644
--- a/src/dutil.c
+++ b/src/dutil.c
@@ -586,20 +586,24 @@ void add_state_to_xml_doc(xmlDocPtr doc, struct netcf *ncf
ATTRIBUTE_UNUSED,
 
     }
 
-    for (cur = proto->children; cur != NULL; cur = cur->next) {
+    /* remove all existing ip nodes from the protocol node - we only
+     * want addresses retrieved from the device in the output.
+     */
+    cur = proto->children;
+    while(cur != NULL) {
+        xmlNodePtr next = cur->next;
         if ((cur->type == XML_ELEMENT_NODE) &&
             (xmlStrEqual(cur->name, BAD_CAST "ip"))) {
-            break;
+            xmlUnlinkNode(cur);
+            xmlFreeNode(cur);
         }
+        cur = next;
     }
 
-    if (ip == NULL) {
-        ip = xmlNewDocNode(doc, NULL, BAD_CAST "ip", NULL);
-        ERR_NOMEM((ip == NULL), ncf);
-
-        cur = xmlAddChild(proto, ip);
-        ERR_NOMEM((cur == NULL), ncf);
-    }
+    ip = xmlNewDocNode(doc, NULL, BAD_CAST "ip", NULL);
+    ERR_NOMEM((ip == NULL), ncf);
+    cur = xmlAddChild(proto, ip);
+    ERR_NOMEM((cur == NULL), ncf);
 
     inet_ntop(AF_INET, (struct in_addr *)&ipv4, ip_str, sizeof(ip_str));
     prop = xmlSetProp(ip, BAD_CAST "address", BAD_CAST ip_str);
-- 
1.6.2.5

[PATCH 2/3] Always remove all configured ip addresses.

[Laine Stump]

even if the interface is down, and we can't get any addresses from the
device itself.
---
 src/drv_initscripts.c |   15 +++++++--------
 src/dutil.c           |   28 +++++++++++++++-------------
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
index e8c4698..6034698 100644
--- a/src/drv_initscripts.c
+++ b/src/drv_initscripts.c
@@ -764,16 +764,15 @@ char *drv_xml_state(struct netcf_if *nif) {
     ncf_xml = apply_stylesheet(ncf, ncf->driver->put, aug_xml);
     ERR_BAIL(ncf);
 
-    /* get the current IP address. If it's non-zero, also get the
-     * current prefix, and add both to the document */
+    /* get the current IP address and prefix, and add both to the
+     * document.
+     */
     ipv4 = if_ipv4_address(ncf, nif->name);
     ERR_BAIL(ncf);
-    if (ipv4 != 0) {
-        prefix = if_ipv4_prefix(ncf, nif->name);
-        ERR_BAIL(ncf);
-        add_state_to_xml_doc(ncf_xml, ncf, ipv4, prefix);
-        ERR_BAIL(ncf);
-    }
+    prefix = if_ipv4_prefix(ncf, nif->name);
+    ERR_BAIL(ncf);
+    add_state_to_xml_doc(ncf_xml, ncf, ipv4, prefix);
+    ERR_BAIL(ncf);
 
     r = xsltSaveResultToString((xmlChar **)&result, &result_len,
                                ncf_xml, ncf->driver->put);
diff --git a/src/dutil.c b/src/dutil.c
index dd75768..d2090a7 100644
--- a/src/dutil.c
+++ b/src/dutil.c
@@ -600,19 +600,21 @@ void add_state_to_xml_doc(xmlDocPtr doc, struct netcf *ncf
ATTRIBUTE_UNUSED,
         cur = next;
     }
 
-    ip = xmlNewDocNode(doc, NULL, BAD_CAST "ip", NULL);
-    ERR_NOMEM((ip == NULL), ncf);
-    cur = xmlAddChild(proto, ip);
-    ERR_NOMEM((cur == NULL), ncf);
-
-    inet_ntop(AF_INET, (struct in_addr *)&ipv4, ip_str, sizeof(ip_str));
-    prop = xmlSetProp(ip, BAD_CAST "address", BAD_CAST ip_str);
-    ERR_NOMEM((prop == NULL), ncf);
-    snprintf(prefix_str, sizeof(prefix_str), "%d", prefix);
-    prop = xmlSetProp(ip, BAD_CAST "prefix", BAD_CAST prefix_str);
-    ERR_NOMEM((prop == NULL), ncf);
-    prop = xmlSetProp(ip, BAD_CAST "source", BAD_CAST "device");
-    ERR_NOMEM((prop == NULL), ncf);
+    if (ipv4 != 0) {
+        ip = xmlNewDocNode(doc, NULL, BAD_CAST "ip", NULL);
+        ERR_NOMEM((ip == NULL), ncf);
+        cur = xmlAddChild(proto, ip);
+        ERR_NOMEM((cur == NULL), ncf);
+
+        inet_ntop(AF_INET, (struct in_addr *)&ipv4, ip_str, sizeof(ip_str));
+        prop = xmlSetProp(ip, BAD_CAST "address", BAD_CAST ip_str);
+        ERR_NOMEM((prop == NULL), ncf);
+        snprintf(prefix_str, sizeof(prefix_str), "%d", prefix);
+        prop = xmlSetProp(ip, BAD_CAST "prefix", BAD_CAST prefix_str);
+        ERR_NOMEM((prop == NULL), ncf);
+        prop = xmlSetProp(ip, BAD_CAST "source", BAD_CAST "device");
+        ERR_NOMEM((prop == NULL), ncf);
+    }
 
 error:
     return;
-- 
1.6.2.5

[PATCH 3/3] Don't attempt SIOCGIFADDR/NETMASK on downed interfaces.

[Laine Stump]

Both of these ioctls return an error if the interface is down.
---
 src/dutil.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/dutil.c b/src/dutil.c
index d2090a7..f3f89a5 100644
--- a/src/dutil.c
+++ b/src/dutil.c
@@ -447,6 +447,10 @@ int if_is_active(struct netcf *ncf, const char *intf) {
 unsigned int if_ipv4_address(struct netcf *ncf, const char *intf) {
     struct ifreq ifr;
 
+    if (!if_is_active(ncf, intf)) {
+        /* SIOCGIFADDR fails on a device that is down */
+        return 0;
+        }
     MEMZERO(&ifr, 1);
     strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
     ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
@@ -461,6 +465,10 @@ unsigned int if_ipv4_address(struct netcf *ncf, const char *intf) {
 unsigned int if_ipv4_netmask(struct netcf *ncf, const char *intf) {
     struct ifreq ifr;
 
+    if (!if_is_active(ncf, intf)) {
+        /* SIOCGIFNETMASK fails on a device that is down */
+        return 0;
+        }
     MEMZERO(&ifr, 1);
     strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
     ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
-- 
1.6.2.5

[David Lutterkort]

On Tue, 2009-09-29 at 14:39 -0400, Laine Stump wrote:
>  The originalpatch for ncf_if_xml_state would search for the first ip
>  node within the ipv4 protocol node, and replace it with the results of
>  SIOCGIFADDR (or sometimes just add a new one on.)
>  
>  Now we remove all ip addresses sourced from the config, even if we
>  can't get any replacements from the device. 
The big question (as I mentioned in the other mail) is: what does
ncf_if_xml_state report ? Only live information or a combination of live
+ configured info ? I could be persuaded either way - it mostly depends
on how people will use the results of ncf_if_xml_state: is ist mostly
for information/reporting, or should it be convenient to use the result
of ncf_if_xml_state and pass it to ncf_define ?

David

[Laine Stump]

On 09/30/2009 02:15 PM, David Lutterkort wrote:
>  On Tue, 2009-09-29 at 14:39 -0400, Laine Stump wrote:
>     
> > The originalpatch for ncf_if_xml_state would search for the first ip
> > node within the ipv4 protocol node, and replace it with the results of
> > SIOCGIFADDR (or sometimes just add a new one on.)
> >
> > Now we remove all ip addresses sourced from the config, even if we
> > can't get any replacements from the device.
> >      
>  The big question (as I mentioned in the other mail) is: what does
>  ncf_if_xml_state report ? Only live information or a combination of live
>  + configured info ? I could be persuaded either way - it mostly depends
>  on how people will use the results of ncf_if_xml_state: is ist mostly
>  for information/reporting, or should it be convenient to use the result
>  of ncf_if_xml_state and pass it to ncf_define ?
>      

I must confess that I went with the former (live info only) at least 
partly because it was easiest to make that work with libvirt in the 
short term (libvirt parses the xml, and currently throws away all but 
the first ip node; that will need to be changed anyway, of course).

 From a complete vaccum of use cases, I'm assuming that ncf_if_xml_state 
will be used mostly for information/reporting; I may be completely 
wrong, though. At any rate, I still don't have a strong enough opinion 
to try and convince you to cross over though. Hopefully someone with a 
strong opinion (and a good reason for it) can step in.