[PATCH 0/3] Add ncf_if_xml_state to report live interface config

[Laine Stump]

The backend of this uses ioctl(SIOCGIFADDR), instead of the NETLINK
socket (which is the proper way to do it), which limits it to
IPv4-only, and just a single IP address per interface. This is just
temporary until I understand netlink better (and it does happen to
meet the requirements of most users), and allows us to give the new
API a workout in the meantime.

Note that I just added the function name into the public symbol list
in the NETCF_1.0.0 section, since the actual version is still at
0.1.1, so I'm not sure what to do with it...

[PATCH 1/3] remove unused variable.

[Laine Stump]

---
 src/drv_initscripts.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
index e17d834..98cebcb 100644
--- a/src/drv_initscripts.c
+++ b/src/drv_initscripts.c
@@ -707,7 +707,7 @@ char *drv_xml_desc(struct netcf_if *nif) {
     struct augeas *aug;
     struct netcf *ncf;
     char **devs = NULL, **intf = NULL;
-    xmlDocPtr aug_xml = NULL, ncf_xml = NULL;
+    xmlDocPtr aug_xml = NULL;
     int ndevs = 0, nint = 0;
 
     ncf = nif->ncf;
@@ -731,7 +731,6 @@ char *drv_xml_desc(struct netcf_if *nif) {
     free_matches(ndevs, &devs);
     free_matches(nint, &intf);
     xmlFreeDoc(aug_xml);
-    xmlFreeDoc(ncf_xml);
     return result;
  error:
     FREE(result);
-- 
1.6.2.5

[PATCH 1/3] remove unused variable.

[David Lutterkort]

On Tue, 2009-09-22 at 16:49 -0400, Laine Stump wrote:
>  ---
>   src/drv_initscripts.c |    3 +--
>   1 files changed, 1 insertions(+), 2 deletions(-) 
ACK. Nice catch.

David

[PATCH 2/3] Change name of is_active to if_is_active to make it consistent with upcoming if_ipv4_address, if_ipv4_prefix, etc.

[Laine Stump]

---
 src/drv_initscripts.c |    6 +++---
 src/dutil.c           |    2 +-
 src/dutil.h           |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
index 98cebcb..58794aa 100644
--- a/src/drv_initscripts.c
+++ b/src/drv_initscripts.c
@@ -539,9 +539,9 @@ static int list_interface_ids(struct netcf *ncf,
             ERR_COND_BAIL(r < 0, ncf, EOTHER);
 
             if (!is_qualified) {
-                int if_is_active = is_active(ncf, name);
-                if ((if_is_active && (flags & NETCF_IFACE_ACTIVE))
-                    || ((!if_is_active) && (flags & NETCF_IFACE_INACTIVE)))
{
+                int is_active = if_is_active(ncf, name);
+                if ((is_active && (flags & NETCF_IFACE_ACTIVE))
+                    || ((!is_active) && (flags & NETCF_IFACE_INACTIVE))) {
 
                     is_qualified = 1;
                 }
diff --git a/src/dutil.c b/src/dutil.c
index c044766..d51994f 100644
--- a/src/dutil.c
+++ b/src/dutil.c
@@ -428,7 +428,7 @@ error:
     return -1;
 }
 
-int is_active(struct netcf *ncf, const char *intf) {
+int if_is_active(struct netcf *ncf, const char *intf) {
     struct ifreq ifr;
 
     MEMZERO(&ifr, 1);
diff --git a/src/dutil.h b/src/dutil.h
index fc86e00..509f308 100644
--- a/src/dutil.h
+++ b/src/dutil.h
@@ -108,8 +108,8 @@ char *xml_prop(xmlNodePtr node, const char *name);
 /* Get a file descriptor to a ioctl socket */
 int init_ioctl_fd(struct netcf *ncf);
 
-/* Check if the interface INTF is up using a ioctl call */
-int is_active(struct netcf *ncf, const char *intf);
+/* Check if the interface INTF is up using an ioctl call */
+int if_is_active(struct netcf *ncf, const char *intf);
 
 /* Create a new netcf if instance for interface NAME */
 struct netcf_if *make_netcf_if(struct netcf *ncf, char *name);
-- 
1.6.2.5

[PATCH 2/3] Change name of is_active to if_is_active to make it consistent with upcoming if_ipv4_address, if_ipv4_prefix, etc.

[David Lutterkort]

On Tue, 2009-09-22 at 16:49 -0400, Laine Stump wrote:
>  ---
>   src/drv_initscripts.c |    6 +++---
>   src/dutil.c           |    2 +-
>   src/dutil.h           |    4 ++--
>   3 files changed, 6 insertions(+), 6 deletions(-) 
ACK. One minor nit: please keep the summary line fairly short ( < 76
chars or so). I fixed that up and committed.

David

[PATCH 3/3] Add a new API function ncf_if_xml_state().

[Laine Stump]

ncf_if_xml_state() returns the current live state of the interface,
which is just the same document returned by ncf_if_xml_desc(), with
the addition/modification of IP address and netmask (prefix) to the
values retrieved directly from the interface. This is useful to learn
the IP address assigned to an interface configured to used DHCP.

Currently only the 1st IPv4 address on an interface is added. The next
step will be to use the netlink socket (rather than
ioctl(SOICGIFxxxx)) to get all IPv4 and IPv6 addresses for the
interface.
---
 src/drv_initscripts.c |   60 +++++++++++++++++++++++++++-
 src/dutil.c           |  108 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/dutil.h           |   14 ++++++
 src/internal.h        |    1 +
 src/ncftool.c         |    8 +++-
 src/netcf.c           |   10 +++++
 src/netcf.h           |   11 ++++-
 src/netcf_public.syms |    1 +
 8 files changed, 209 insertions(+), 4 deletions(-)

diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
index 58794aa..c9031b3 100644
--- a/src/drv_initscripts.c
+++ b/src/drv_initscripts.c
@@ -702,6 +702,7 @@ static int aug_put_xml(struct netcf *ncf, xmlDocPtr xml) {
     return result;
 }
 
+/* return the current static configuration (as saved on disk) */
 char *drv_xml_desc(struct netcf_if *nif) {
     char *result = NULL;
     struct augeas *aug;
@@ -723,7 +724,6 @@ char *drv_xml_desc(struct netcf_if *nif) {
     ERR_BAIL(ncf);
 
     aug_xml = aug_get_xml(ncf, nint, intf);
-
     result = apply_stylesheet_to_string(ncf, ncf->driver->put, aug_xml);
     ERR_BAIL(ncf);
 
@@ -737,6 +737,64 @@ char *drv_xml_desc(struct netcf_if *nif) {
     goto done;
 }
 
+/* return the current live configuration state - a combination of
+ * drv_xml_desc + results of querying the interface directly */
+
+char *drv_xml_state(struct netcf_if *nif) {
+    char *result = NULL;
+    int r, result_len;
+    struct augeas *aug;
+    struct netcf *ncf;
+    char **devs = NULL, **intf = NULL;
+    xmlDocPtr aug_xml = NULL, ncf_xml = NULL;
+    int ndevs = 0, nint = 0;
+    unsigned int ipv4;
+    int prefix;
+
+    ncf = nif->ncf;
+    aug = get_augeas(ncf);
+    ERR_BAIL(ncf);
+
+    ndevs = aug_fmt_match(ncf, &devs,
+              "%s[ DEVICE = '%s' or BRIDGE = '%s' or MASTER =
'%s']/DEVICE",
+              ifcfg_path, nif->name, nif->name, nif->name);
+    ERR_BAIL(ncf);
+
+    nint = uniq_ifcfg_paths(ncf, ndevs, devs, &intf);
+    ERR_BAIL(ncf);
+
+    aug_xml = aug_get_xml(ncf, nint, intf);
+
+
+    /* same as drv_xml_desc() up to this point, a bit different from now on... */
+
+    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 */
+    ipv4 = if_ipv4_address(ncf, nif->name);
+    if (ipv4 != 0) {
+        prefix = if_ipv4_prefix(ncf, nif->name);
+        add_state_to_xml_doc(ncf_xml, ncf, ipv4, prefix);
+        ERR_BAIL(ncf);
+    }
+
+    r = xsltSaveResultToString((xmlChar **)&result, &result_len,
+                               ncf_xml, ncf->driver->put);
+    ERR_NOMEM(r < 0, ncf);
+
+ done:
+    free_matches(ndevs, &devs);
+    free_matches(nint, &intf);
+    xmlFreeDoc(ncf_xml);
+    xmlFreeDoc(aug_xml);
+    return result;
+ error:
+    FREE(result);
+    goto done;
+}
+
 /* Get the content of /interface/@name. Result must be freed with xmlFree() */
 static char *device_name_from_xml(xmlDocPtr xml) {
     xmlNodePtr iface;
diff --git a/src/dutil.c b/src/dutil.c
index d51994f..5ff1ccb 100644
--- a/src/dutil.c
+++ b/src/dutil.c
@@ -35,6 +35,9 @@
 #include <fcntl.h>
 #include <sys/ioctl.h>
 #include <net/if.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+
 
 #include "safe-alloc.h"
 #include "ref.h"
@@ -440,6 +443,45 @@ int if_is_active(struct netcf *ncf, const char *intf) {
     return ((ifr.ifr_flags & IFF_UP) == IFF_UP);
 }
 
+unsigned int if_ipv4_address(struct netcf *ncf, const char *intf) {
+    struct ifreq ifr;
+
+    MEMZERO(&ifr, 1);
+    strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
+    ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
+    if (ioctl(ncf->driver->ioctl_fd, SIOCGIFADDR, &ifr))  {
+        return 0;
+    }
+
+    return (((struct sockaddr_in *)&ifr.ifr_addr)->sin_addr.s_addr);
+}
+
+unsigned int if_ipv4_netmask(struct netcf *ncf, const char *intf) {
+    struct ifreq ifr;
+
+    MEMZERO(&ifr, 1);
+    strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
+    ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
+    if (ioctl(ncf->driver->ioctl_fd, SIOCGIFNETMASK, &ifr))  {
+        return 0;
+    }
+
+    return (((struct sockaddr_in *)&ifr.ifr_netmask)->sin_addr.s_addr);
+}
+
+
+int if_ipv4_prefix(struct netcf *ncf, const char *intf) {
+    unsigned int nmv4 = if_ipv4_netmask(ncf, intf);
+    int prefix = 0;
+
+    nmv4 = ntohl(nmv4);
+    while ((nmv4 & 0xFFFFFFFF) != 0) {
+        nmv4 <<= 1;
+        prefix++;
+    }
+    return prefix;
+}
+
 /* Create a new netcf if instance for interface NAME */
 struct netcf_if *make_netcf_if(struct netcf *ncf, char *name) {
     int r;
@@ -499,6 +541,72 @@ int dutil_put_aug(struct netcf *ncf, const char *aug_xml, char
**ncf_xml) {
     return result;
 }
 
+/* Given an xml document that follows interface.rng, add the IP
+ * address and prefix under protocol/ip
+ */
+void add_state_to_xml_doc(xmlDocPtr doc, struct netcf *ncf ATTRIBUTE_UNUSED,
+                          unsigned int ipv4, int prefix) {
+
+    xmlNodePtr root = NULL, proto = NULL, ip = NULL, cur;
+
+    root = xmlDocGetRootElement(doc);
+    ERR_THROW((root == NULL), ncf, EINTERNAL, "failed to get document root
element");
+
+    ERR_THROW(!xmlStrEqual(root->name, BAD_CAST "interface"),
+              ncf, EINTERNAL, "root document is not an interface");
+
+    cur = root->children;
+    while (cur != NULL) {
+        if ((cur->type == XML_ELEMENT_NODE) &&
+            (xmlStrEqual(cur->name, BAD_CAST "protocol"))) {
+            xmlChar *family = xmlGetProp(cur, BAD_CAST "family");
+            if (family != NULL) {
+                if (xmlStrEqual(family, BAD_CAST "ipv4"))
+                    proto = cur;
+                xmlFree(family);
+                if (proto != NULL) {
+                    goto proto_found;
+                }
+            }
+        }
+        cur = cur->next;
+    }
+    proto = xmlNewDocNode(doc, NULL, BAD_CAST "protocol", NULL);
+    ERR_THROW((proto == NULL), ncf, EINTERNAL, "Failed to add protocol node to
document");
+
+    xmlAddChild(root, proto);
+    xmlSetProp(proto, BAD_CAST "family", BAD_CAST "ipv4");
+
+proto_found:
+
+    cur = proto->children;
+    while (cur != NULL) {
+        if ((cur->type == XML_ELEMENT_NODE) &&
+            (xmlStrEqual(cur->name, BAD_CAST "ip"))) {
+            goto ip_found;
+        }
+        cur = cur->next;
+    }
+  
+    ip = xmlNewDocNode(doc, NULL, BAD_CAST "ip", NULL);
+    ERR_THROW((ip == NULL), ncf, EINTERNAL, "Failed to add ip node to
document");
+
+    xmlAddChild(proto, ip);
+
+ip_found:
+    {
+        char ip_str[48], prefix_str[16];
+
+        inet_ntop(AF_INET, (struct in_addr *)&ipv4, ip_str, sizeof(ip_str));
+        xmlSetProp(ip, BAD_CAST "address", BAD_CAST ip_str);
+        sprintf(prefix_str, "%d", prefix);
+        xmlSetProp(ip, BAD_CAST "prefix", BAD_CAST prefix_str);
+    }
+
+error:
+    return;
+}
+
 /*
  * Bringing interfaces up/down
  */
diff --git a/src/dutil.h b/src/dutil.h
index 509f308..20e40e6 100644
--- a/src/dutil.h
+++ b/src/dutil.h
@@ -111,6 +111,17 @@ int init_ioctl_fd(struct netcf *ncf);
 /* Check if the interface INTF is up using an ioctl call */
 int if_is_active(struct netcf *ncf, const char *intf);
 
+/* Get the current IPv4 address of INTF as an unsigned int using an ioctl call */
+unsigned int if_ipv4_address(struct netcf *ncf, const char *intf);
+
+/* get the current IPv4 netmask of INTF as an unsigned int using an ioctl call */
+unsigned int if_ipv4_netmask(struct netcf *ncf, const char *intf);
+
+/* get the current IPv4 prefix (#bits of netmask) of INTF as an
+ * int using an ioctl call */
+
+int if_ipv4_prefix(struct netcf *ncf, const char *intf);
+
 /* Create a new netcf if instance for interface NAME */
 struct netcf_if *make_netcf_if(struct netcf *ncf, char *name);
 
@@ -120,6 +131,9 @@ int dutil_get_aug(struct netcf *ncf, const char *ncf_xml, char
**aug_xml);
 /* Transform the Augeas XML AUG_XML into interface XML NCF_XML */
 int dutil_put_aug(struct netcf *ncf, const char *aug_xml, char **ncf_xml);
 
+/* add the given state (currently IP + netmask) to the interface's xml document */
+void add_state_to_xml_doc(xmlDocPtr doc, struct netcf *ncf, unsigned int ipv4, int
prefix);
+
 /* Run the program PROG with the single argument ARG */
 void run1(struct netcf *ncf, const char *prog, const char *arg);
 
diff --git a/src/internal.h b/src/internal.h
index 7205e72..ca9a0ef 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -179,6 +179,7 @@ struct netcf_if *drv_lookup_by_name(struct netcf *ncf, const char
*name);
 int drv_lookup_by_mac_string(struct netcf *, const char *mac,
                              int maxifaces, struct netcf_if **ifaces);
 char *drv_xml_desc(struct netcf_if *);
+char *drv_xml_state(struct netcf_if *);
 const char *drv_mac_string(struct netcf_if *nif);
 struct netcf_if *drv_define(struct netcf *ncf, const char *xml);
 int drv_undefine(struct netcf_if *nif);
diff --git a/src/ncftool.c b/src/ncftool.c
index a1362c2..3639dd7 100644
--- a/src/ncftool.c
+++ b/src/ncftool.c
@@ -228,7 +228,11 @@ static int cmd_dump_xml(const struct command *cmd) {
         goto done;
     }
 
-    xml = ncf_if_xml_desc(nif);
+    if (opt_present(cmd, "live")) {
+        xml = ncf_if_xml_state(nif);
+    } else {
+        xml = ncf_if_xml_desc(nif);
+    }
     if (xml == NULL)
         goto done;
 
@@ -246,6 +250,8 @@ static const struct command_opt_def cmd_dump_xml_opts[] = {
       .help = "interpret the name as a MAC address" },
     { .tag = CMD_OPT_ARG, .name = "name",
       .help = "the name of the interface" },
+    { .tag = CMD_OPT_BOOL, .name = "live",
+      .help = "show show live state of the interface, rather than just config file
settings" },
     CMD_OPT_DEF_LAST
 };
 
diff --git a/src/netcf.c b/src/netcf.c
index 2907349..7eff436 100644
--- a/src/netcf.c
+++ b/src/netcf.c
@@ -200,6 +200,16 @@ char *ncf_if_xml_desc(struct netcf_if *nif) {
     return drv_xml_desc(nif);
 }
 
+/* Produce an XML description of the current live state of the
+ * interface, in the same format that NCF_DEFINE expects, but
+ * potentially with extra info not contained in the static config (ie
+ * the current IP address of an interface that uses DHCP)
+ */
+char *ncf_if_xml_state(struct netcf_if *nif) {
+    API_ENTRY(nif->ncf);
+    return drv_xml_state(nif);
+}
+
 /* Release any resources used by this NETCF_IF; the pointer is invalid
  * after this call
  */
diff --git a/src/netcf.h b/src/netcf.h
index 482b6d0..210dd4f 100644
--- a/src/netcf.h
+++ b/src/netcf.h
@@ -149,11 +149,18 @@ int ncf_if_down(struct netcf_if *);
 /* Delete the definition */
 int ncf_if_undefine(struct netcf_if *);
 
-/* Produce an XML description for the interface, in the same format that
- * NCF_DEFINE expects
+/* Produce an XML description for the static (stored) interface
+ * config, in the same format that NCF_DEFINE expects
  */
 char *ncf_if_xml_desc(struct netcf_if *);
 
+/* Produce an XML description of the current live state of the
+ * interface, in the same format that NCF_DEFINE expects, but
+ * potentially with extra info not contained in the static config (ie
+ * the current IP address of an interface that uses DHCP)
+ */
+char *ncf_if_xml_state(struct netcf_if *);
+
 /* Release any resources used by this NETCF_IF; the pointer is invalid
  * after this call
  */
diff --git a/src/netcf_public.syms b/src/netcf_public.syms
index 230b6bf..4abad74 100644
--- a/src/netcf_public.syms
+++ b/src/netcf_public.syms
@@ -13,6 +13,7 @@ NETCF_1.0.0 {
       ncf_if_down;
       ncf_if_undefine;
       ncf_if_xml_desc;
+      ncf_if_xml_state;
       ncf_if_free;
       ncf_error;
     local:
-- 
1.6.2.5

[PATCH 3/3] Add a new API function ncf_if_xml_state().

[David Lutterkort]

On Tue, 2009-09-22 at 16:49 -0400, Laine Stump wrote:

I get this error when I try to apply the patch:

        /tmp/patches/laine-netcf-3.patch:298: trailing whitespace.
          
        error: patch failed: src/drv_initscripts.c:737
        error: src/drv_initscripts.c: patch does not apply
        
It seems you need to rebase the patch against the latest HEAD in git.

>  diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
>  index 58794aa..c9031b3 100644
>  --- a/src/drv_initscripts.c
>  +++ b/src/drv_initscripts.c
>  @@ -702,6 +702,7 @@ static int aug_put_xml(struct netcf *ncf, xmlDocPtr xml) {
>       return result;
>   }
>   
>  +/* return the current static configuration (as saved on disk) */
>   char *drv_xml_desc(struct netcf_if *nif) {
>       char *result = NULL;
>       struct augeas *aug;
>  @@ -723,7 +724,6 @@ char *drv_xml_desc(struct netcf_if *nif) {
>       ERR_BAIL(ncf);
>   
>       aug_xml = aug_get_xml(ncf, nint, intf);
>  -
>       result = apply_stylesheet_to_string(ncf, ncf->driver->put, aug_xml);
>       ERR_BAIL(ncf); 
Try not to delete empty lines etc. without need. Just makes the patch
noisier.
 
>  @@ -737,6 +737,64 @@ char *drv_xml_desc(struct netcf_if *nif) {
>       goto done;
>   }
>   
>  +/* return the current live configuration state - a combination of
>  + * drv_xml_desc + results of querying the interface directly */
>  +
>  +char *drv_xml_state(struct netcf_if *nif) {
>  +    char *result = NULL;
>  +    int r, result_len;
>  +    struct augeas *aug;
>  +    struct netcf *ncf;
>  +    char **devs = NULL, **intf = NULL;
>  +    xmlDocPtr aug_xml = NULL, ncf_xml = NULL;
>  +    int ndevs = 0, nint = 0;
>  +    unsigned int ipv4;
>  +    int prefix;
>  +
>  +    ncf = nif->ncf;
>  +    aug = get_augeas(ncf);
>  +    ERR_BAIL(ncf);
>  +
>  +    ndevs = aug_fmt_match(ncf, &devs,
>  +              "%s[ DEVICE = '%s' or BRIDGE = '%s' or MASTER =
> '%s']/DEVICE",
>  +              ifcfg_path, nif->name, nif->name, nif->name);
>  +    ERR_BAIL(ncf);
>  +
>  +    nint = uniq_ifcfg_paths(ncf, ndevs, devs, &intf);
>  +    ERR_BAIL(ncf);
>  +
>  +    aug_xml = aug_get_xml(ncf, nint, intf);
>  +
>  +
>  +    /* same as drv_xml_desc() up to this point, a bit different from now on... */
Can that be factored into some helper function that drv_xml_desc and
drv_xml_state both use ?

>  +    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 */
>  +    ipv4 = if_ipv4_address(ncf, nif->name); 
if_ipv4_address should set ncf->errcode (with one of the ERR_ macros),
and you should use ERR_BAIL(ncf) after the invocation to make sure
errors ar propagated. That also means you don't have the 'if (ipv4 !=
0)' check.

>  +    if (ipv4 != 0) {
>  +        prefix = if_ipv4_prefix(ncf, nif->name);
>  +        add_state_to_xml_doc(ncf_xml, ncf, ipv4, prefix);
>  +        ERR_BAIL(ncf);
>  +    }
>  +
>  +    r = xsltSaveResultToString((xmlChar **)&result, &result_len,
>  +                               ncf_xml, ncf->driver->put);
>  +    ERR_NOMEM(r < 0, ncf);
>  +
>  + done:
>  +    free_matches(ndevs, &devs);
>  +    free_matches(nint, &intf);
>  +    xmlFreeDoc(ncf_xml);
>  +    xmlFreeDoc(aug_xml);
>  +    return result;
>  + error:
>  +    FREE(result);
>  +    goto done;
>  +}
>  +
>   /* Get the content of /interface/@name. Result must be freed with xmlFree() */
>   static char *device_name_from_xml(xmlDocPtr xml) {
>       xmlNodePtr iface; 
This is where applying the patch fails (the context is wrong) - you need
to rebase.

>  diff --git a/src/dutil.c b/src/dutil.c
>  index d51994f..5ff1ccb 100644
>  --- a/src/dutil.c
>  +++ b/src/dutil.c
>  @@ -35,6 +35,9 @@
>   #include <fcntl.h>
>   #include <sys/ioctl.h>
>   #include <net/if.h>
>  +#include <netinet/in.h>
>  +#include <arpa/inet.h>
>  +
>   
>   #include "safe-alloc.h"
>   #include "ref.h"
>  @@ -440,6 +443,45 @@ int if_is_active(struct netcf *ncf, const char *intf) {
>       return ((ifr.ifr_flags & IFF_UP) == IFF_UP);
>   }
>   
>  +unsigned int if_ipv4_address(struct netcf *ncf, const char *intf) {
>  +    struct ifreq ifr;
>  +
>  +    MEMZERO(&ifr, 1);
>  +    strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
>  +    ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
>  +    if (ioctl(ncf->driver->ioctl_fd, SIOCGIFADDR, &ifr))  {
>  +        return 0; 
Use one of the ERR_ macros to set the ncf->errcode. Feel free to add a
new error code if there isn't an appropriate one yet.

>  +    }
>  +
>  +    return (((struct sockaddr_in *)&ifr.ifr_addr)->sin_addr.s_addr);
>  +}
>  +
>  +unsigned int if_ipv4_netmask(struct netcf *ncf, const char *intf) {
>  +    struct ifreq ifr;
>  +
>  +    MEMZERO(&ifr, 1);
>  +    strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
>  +    ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
>  +    if (ioctl(ncf->driver->ioctl_fd, SIOCGIFNETMASK, &ifr))  {
>  +        return 0;
>  +    } 
You need to set ncf->errcode here, too, if the ioctl fails.

>  +    return (((struct sockaddr_in
> *)&ifr.ifr_netmask)->sin_addr.s_addr);
>  +}
>  +
>  +
>  +int if_ipv4_prefix(struct netcf *ncf, const char *intf) {
>  +    unsigned int nmv4 = if_ipv4_netmask(ncf, intf);
>  +    int prefix = 0;
>  +
>  +    nmv4 = ntohl(nmv4);
>  +    while ((nmv4 & 0xFFFFFFFF) != 0) {
>  +        nmv4 <<= 1;
>  +        prefix++;
>  +    }
>  +    return prefix;
>  +}
>  +
>   /* Create a new netcf if instance for interface NAME */
>   struct netcf_if *make_netcf_if(struct netcf *ncf, char *name) {
>       int r;
>  @@ -499,6 +541,72 @@ int dutil_put_aug(struct netcf *ncf, const char *aug_xml, char
> **ncf_xml) {
>       return result;
>   }
>   
>  +/* Given an xml document that follows interface.rng, add the IP
>  + * address and prefix under protocol/ip
>  + */
>  +void add_state_to_xml_doc(xmlDocPtr doc, struct netcf *ncf ATTRIBUTE_UNUSED,
>  +                          unsigned int ipv4, int prefix) {
>  +
>  +    xmlNodePtr root = NULL, proto = NULL, ip = NULL, cur;
>  +
>  +    root = xmlDocGetRootElement(doc);
>  +    ERR_THROW((root == NULL), ncf, EINTERNAL, "failed to get document root
> element");
>  +
>  +    ERR_THROW(!xmlStrEqual(root->name, BAD_CAST "interface"),
>  +              ncf, EINTERNAL, "root document is not an interface");
>  +
>  +    cur = root->children;
>  +    while (cur != NULL) { 
Clearer as 'for (cur = root->children; cur != NULL; cur = cur->next) {'

>  +        if ((cur->type == XML_ELEMENT_NODE) &&
>  +            (xmlStrEqual(cur->name, BAD_CAST "protocol"))) {
>  +            xmlChar *family = xmlGetProp(cur, BAD_CAST "family");
>  +            if (family != NULL) {
>  +                if (xmlStrEqual(family, BAD_CAST "ipv4"))
>  +                    proto = cur;
>  +                xmlFree(family);
>  +                if (proto != NULL) {
>  +                    goto proto_found; 
I'd personally prefer a 'break' here, and then something like
        if (proto == NULL) {
          .. create proto node ..
        }
right after the for loop.

>  +                }
>  +            }
>  +        }
>  +        cur = cur->next;
>  +    }
>  +    proto = xmlNewDocNode(doc, NULL, BAD_CAST "protocol", NULL);
>  +    ERR_THROW((proto == NULL), ncf, EINTERNAL, "Failed to add protocol node to
> document"); 
Generally, I only use EINTERNAL for things that are bugs - I haven't
looked why xmlNewDocNode can fail, but I assume NETCF_ENOMEM is more
appropriate here.

>  +
>  +    xmlAddChild(root, proto);
>  +    xmlSetProp(proto, BAD_CAST "family", BAD_CAST "ipv4");
>  +
>  +proto_found:
>  +
>  +    cur = proto->children;
>  +    while (cur != NULL) { 
Similarly, better as a for loop.

>  +        if ((cur->type == XML_ELEMENT_NODE) &&
>  +            (xmlStrEqual(cur->name, BAD_CAST "ip"))) {
>  +            goto ip_found;
>  +        }
>  +        cur = cur->next;
>  +    }
>  +  
>  +    ip = xmlNewDocNode(doc, NULL, BAD_CAST "ip", NULL);
>  +    ERR_THROW((ip == NULL), ncf, EINTERNAL, "Failed to add ip node to
> document"); 
Same comment to use ERR_NOMEM instead of reporting an internal error.

>  +    xmlAddChild(proto, ip);
>  +
>  +ip_found:
>  +    {
>  +        char ip_str[48], prefix_str[16];
>  +
>  +        inet_ntop(AF_INET, (struct in_addr *)&ipv4, ip_str, sizeof(ip_str));
>  +        xmlSetProp(ip, BAD_CAST "address", BAD_CAST ip_str);
>  +        sprintf(prefix_str, "%d", prefix); 
To be defensive, use snprintf, not sprintf.

>  +        xmlSetProp(ip, BAD_CAST "prefix", BAD_CAST
> prefix_str); 
Can xmlSetProp fail ? I assume it could run out of memory.

>  +/* Produce an XML description of the current live state of the
>  + * interface, in the same format that NCF_DEFINE expects, but
>  + * potentially with extra info not contained in the static config (ie
>  + * the current IP address of an interface that uses DHCP)
>  + */
>  +char *ncf_if_xml_state(struct netcf_if *); 
That comment brings up the question what the intent for, e.g., DHCP is:
to consistently report 'live' settings, you'd also need to look if the
file /var/run/dhclient-$IFACE.pid exists and that there is a process
with that PID around. Right now, ncf_if_xml_state will report whether
the interface is _configured_ to use DHCP, not whether it is getting its
address with DHCP.

The interface.rng currently also doesn't allow both a <dhcp/> element
and an <ip/> entry - in the future it will, but with a different
meaning. So, I wonder if there is a need to add something to the <ip/>
element to indicate that the address is the live address.

David

[PATCH 3/3] Add a new API function ncf_if_xml_state().

[Laine Stump]

(I've got all the suggested changes noted below in my tree, but will 
wait for replies to this before posting again...)

On 09/23/2009 03:27 PM, David Lutterkort wrote:
>  On Tue, 2009-09-22 at 16:49 -0400, Laine Stump wrote:
>
>  I get this error when I try to apply the patch:
>
>           /tmp/patches/laine-netcf-3.patch:298: trailing whitespace.
>
>           error: patch failed: src/drv_initscripts.c:737
>           error: src/drv_initscripts.c: patch does not apply
>
>  It seems you need to rebase the patch against the latest HEAD in git.
>      

Yeah, sorry - I forgot to rebase before I sent it.


>     
> > diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
> > index 58794aa..c9031b3 100644
> > --- a/src/drv_initscripts.c
> > +++ b/src/drv_initscripts.c
> > @@ -702,6 +702,7 @@ static int aug_put_xml(struct netcf *ncf, xmlDocPtr xml) {
> >       return result;
> >   }
> >
> > +/* return the current static configuration (as saved on disk) */
> >   char *drv_xml_desc(struct netcf_if *nif) {
> >       char *result = NULL;
> >       struct augeas *aug;
> > @@ -723,7 +724,6 @@ char *drv_xml_desc(struct netcf_if *nif) {
> >       ERR_BAIL(ncf);
> >
> >       aug_xml = aug_get_xml(ncf, nint, intf);
> > -
> >       result = apply_stylesheet_to_string(ncf, ncf->driver->put, aug_xml);
> >       ERR_BAIL(ncf);
> >      
>  Try not to delete empty lines etc. without need. Just makes the patch
>  noisier.
>      

Again, caused by my haste.


>     
> > @@ -737,6 +737,64 @@ char *drv_xml_desc(struct netcf_if *nif) {
> >       goto done;
> >   }
> >
> > +/* return the current live configuration state - a combination of
> > + * drv_xml_desc + results of querying the interface directly */
> > +
> > +char *drv_xml_state(struct netcf_if *nif) {
> > +    char *result = NULL;
> > +    int r, result_len;
> > +    struct augeas *aug;
> > +    struct netcf *ncf;
> > +    char **devs = NULL, **intf = NULL;
> > +    xmlDocPtr aug_xml = NULL, ncf_xml = NULL;
> > +    int ndevs = 0, nint = 0;
> > +    unsigned int ipv4;
> > +    int prefix;
> > +
> > +    ncf = nif->ncf;
> > +    aug = get_augeas(ncf);
> > +    ERR_BAIL(ncf);
> > +
> > +    ndevs = aug_fmt_match(ncf,&devs,
> > +              "%s[ DEVICE = '%s' or BRIDGE = '%s' or MASTER =
> '%s']/DEVICE",
> > +              ifcfg_path, nif->name, nif->name, nif->name);
> > +    ERR_BAIL(ncf);
> > +
> > +    nint = uniq_ifcfg_paths(ncf, ndevs, devs,&intf);
> > +    ERR_BAIL(ncf);
> > +
> > +    aug_xml = aug_get_xml(ncf, nint, intf);
> > +
> > +
> > +    /* same as drv_xml_desc() up to this point, a bit different from now on... */
> >      
>  Can that be factored into some helper function that drv_xml_desc and
>  drv_xml_state both use ?
>      

Does "aug_get_xml" is already taken by the function that gets the 
aug_xml for all interfaces. How about I name this function 
aug_get_one_xml? Or do you have a better idea?


>     
> > +    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 */
> > +    ipv4 = if_ipv4_address(ncf, nif->name);
> >      
>  if_ipv4_address should set ncf->errcode (with one of the ERR_ macros),
>  and you should use ERR_BAIL(ncf) after the invocation to make sure
>  errors ar propagated. That also means you don't have the 'if (ipv4 !=
>  0)' check.
>      

Actually I was assuming that, in the case of an interface that didn't 
yet have an IP address ifconfiged, the ioctl would return a 0 result, 
but no error. In this case you wouldn't want to add 'ip 
address="0.0.0.0"' to the document. So I think I should have the if_* 
functions setting errcode, but I still need to check for != 0.


>     
> > +    if (ipv4 != 0) {
> > +        prefix = if_ipv4_prefix(ncf, nif->name);
> > +        add_state_to_xml_doc(ncf_xml, ncf, ipv4, prefix);
> > +        ERR_BAIL(ncf);
> > +    }
> > +
> > +    r = xsltSaveResultToString((xmlChar **)&result,&result_len,
> > +                               ncf_xml, ncf->driver->put);
> > +    ERR_NOMEM(r<  0, ncf);
> > +
> > + done:
> > +    free_matches(ndevs,&devs);
> > +    free_matches(nint,&intf);
> > +    xmlFreeDoc(ncf_xml);
> > +    xmlFreeDoc(aug_xml);
> > +    return result;
> > + error:
> > +    FREE(result);
> > +    goto done;
> > +}
> > +
> >   /* Get the content of /interface/@name. Result must be freed with xmlFree() */
> >   static char *device_name_from_xml(xmlDocPtr xml) {
> >       xmlNodePtr iface;
> >      
>  This is where applying the patch fails (the context is wrong) - you need
>  to rebase.
>      

Yup.


>     
> > diff --git a/src/dutil.c b/src/dutil.c
> > index d51994f..5ff1ccb 100644
> > --- a/src/dutil.c
> > +++ b/src/dutil.c
> > @@ -35,6 +35,9 @@
> >   #include<fcntl.h>
> >   #include<sys/ioctl.h>
> >   #include<net/if.h>
> > +#include<netinet/in.h>
> > +#include<arpa/inet.h>
> > +
> >
> >   #include "safe-alloc.h"
> >   #include "ref.h"
> > @@ -440,6 +443,45 @@ int if_is_active(struct netcf *ncf, const char *intf) {
> >       return ((ifr.ifr_flags&  IFF_UP) == IFF_UP);
> >   }
> >
> > +unsigned int if_ipv4_address(struct netcf *ncf, const char *intf) {
> > +    struct ifreq ifr;
> > +
> > +    MEMZERO(&ifr, 1);
> > +    strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
> > +    ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
> > +    if (ioctl(ncf->driver->ioctl_fd, SIOCGIFADDR,&ifr))  {
> > +        return 0;
> >      
>  Use one of the ERR_ macros to set the ncf->errcode. Feel free to add a
>  new error code if there isn't an appropriate one yet.
>      

Done now for all 3 of the if_ functions.


>     
> > +    }
> > +
> > +    return (((struct sockaddr_in *)&ifr.ifr_addr)->sin_addr.s_addr);
> > +}
> > +
> > +unsigned int if_ipv4_netmask(struct netcf *ncf, const char *intf) {
> > +    struct ifreq ifr;
> > +
> > +    MEMZERO(&ifr, 1);
> > +    strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
> > +    ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
> > +    if (ioctl(ncf->driver->ioctl_fd, SIOCGIFNETMASK,&ifr))  {
> > +        return 0;
> > +    }
> >      
>  You need to set ncf->errcode here, too, if the ioctl fails.
>
>     
> > +    return (((struct sockaddr_in *)&ifr.ifr_netmask)->sin_addr.s_addr);
> > +}
> > +
> > +
> > +int if_ipv4_prefix(struct netcf *ncf, const char *intf) {
> > +    unsigned int nmv4 = if_ipv4_netmask(ncf, intf);
> > +    int prefix = 0;
> > +
> > +    nmv4 = ntohl(nmv4);
> > +    while ((nmv4&  0xFFFFFFFF) != 0) {
> > +        nmv4<<= 1;
> > +        prefix++;
> > +    }
> > +    return prefix;
> > +}
> > +
> >   /* Create a new netcf if instance for interface NAME */
> >   struct netcf_if *make_netcf_if(struct netcf *ncf, char *name) {
> >       int r;
> > @@ -499,6 +541,72 @@ int dutil_put_aug(struct netcf *ncf, const char *aug_xml, char
> **ncf_xml) {
> >       return result;
> >   }
> >
> > +/* Given an xml document that follows interface.rng, add the IP
> > + * address and prefix under protocol/ip
> > + */
> > +void add_state_to_xml_doc(xmlDocPtr doc, struct netcf *ncf ATTRIBUTE_UNUSED,
> > +                          unsigned int ipv4, int prefix) {
> > +
> > +    xmlNodePtr root = NULL, proto = NULL, ip = NULL, cur;
> > +
> > +    root = xmlDocGetRootElement(doc);
> > +    ERR_THROW((root == NULL), ncf, EINTERNAL, "failed to get document root
> element");
> > +
> > +    ERR_THROW(!xmlStrEqual(root->name, BAD_CAST "interface"),
> > +              ncf, EINTERNAL, "root document is not an interface");
> > +
> > +    cur = root->children;
> > +    while (cur != NULL) {
> >      
>  Clearer as 'for (cur = root->children; cur != NULL; cur = cur->next) {'
>      

That's what I get for taking someone else's (working!) code ;-) (I 
figured "if it ain't broke, don't fix it!" - Ture story - while talking

to a coworker once about refactoring a piece of working code, I opened 
the fortune cookie that came with my lunch and found exactly that quote.)


>     
> > +        if ((cur->type == XML_ELEMENT_NODE)&&
> > +            (xmlStrEqual(cur->name, BAD_CAST "protocol"))) {
> > +            xmlChar *family = xmlGetProp(cur, BAD_CAST "family");
> > +            if (family != NULL) {
> > +                if (xmlStrEqual(family, BAD_CAST "ipv4"))
> > +                    proto = cur;
> > +                xmlFree(family);
> > +                if (proto != NULL) {
> > +                    goto proto_found;
> >      
>  I'd personally prefer a 'break' here, and then something like
>           if (proto == NULL) {
>             .. create proto node ..
>           }
>  right after the for loop.
>      

Yeah, I agree. I actually dislike goto's and labels (knee-jerk reflex 
from from my first quarter freshman year data structures class (so long 
ago it was taught in Pascal)), but I've seen so many lately it's become 
background noise.


>     
> > +                }
> > +            }
> > +        }
> > +        cur = cur->next;
> > +    }
> > +    proto = xmlNewDocNode(doc, NULL, BAD_CAST "protocol", NULL);
> > +    ERR_THROW((proto == NULL), ncf, EINTERNAL, "Failed to add protocol node to
> document");
> >      
>  Generally, I only use EINTERNAL for things that are bugs - I haven't
>  looked why xmlNewDocNode can fail, but I assume NETCF_ENOMEM is more
>  appropriate here.
>      

Makes sense to me. I'll make it ENOMEM.


>     
> > +
> > +    xmlAddChild(root, proto);
> > +    xmlSetProp(proto, BAD_CAST "family", BAD_CAST "ipv4");
> > +
> > +proto_found:
> > +
> > +    cur = proto->children;
> > +    while (cur != NULL) {
> >      
>  Similarly, better as a for loop.
>      
Changed.

>     
> > +        if ((cur->type == XML_ELEMENT_NODE)&&
> > +            (xmlStrEqual(cur->name, BAD_CAST "ip"))) {
> > +            goto ip_found;
> > +        }
> > +        cur = cur->next;
> > +    }
> > +
> > +    ip = xmlNewDocNode(doc, NULL, BAD_CAST "ip", NULL);
> > +    ERR_THROW((ip == NULL), ncf, EINTERNAL, "Failed to add ip node to
> document");
> >      
>  Same comment to use ERR_NOMEM instead of reporting an internal error.
>      
Changed.

>     
> > +    xmlAddChild(proto, ip);
> > +
> > +ip_found:
> > +    {
> > +        char ip_str[48], prefix_str[16];
> > +
> > +        inet_ntop(AF_INET, (struct in_addr *)&ipv4, ip_str, sizeof(ip_str));
> > +        xmlSetProp(ip, BAD_CAST "address", BAD_CAST ip_str);
> > +        sprintf(prefix_str, "%d", prefix);
> >      
>  To be defensive, use snprintf, not sprintf.
>      

Agreed. I was asleep at the wheel.


>     
> > +        xmlSetProp(ip, BAD_CAST "prefix", BAD_CAST prefix_str);
> >      
>  Can xmlSetProp fail ? I assume it could run out of memory.
>      

 From the source, it looks like it can fail (and return NULL) due to 
internal inconsistencies in the data structures, or from memory 
allocation failure (my guess is that any internal inconsistencies will 
likely have been caused by a prior memory allocation failure).

For some reason, I can't find a single instance of someone calling it 
and checking the return value (including the one other invocation 
already in netcf ;-). I'll make mine the first.

The same comments apply to xmlAddChild.



>     
> > +/* Produce an XML description of the current live state of the
> > + * interface, in the same format that NCF_DEFINE expects, but
> > + * potentially with extra info not contained in the static config (ie
> > + * the current IP address of an interface that uses DHCP)
> > + */
> > +char *ncf_if_xml_state(struct netcf_if *);
> >      
>  That comment brings up the question what the intent for, e.g., DHCP is:
>  to consistently report 'live' settings, you'd also need to look if the
>  file /var/run/dhclient-$IFACE.pid exists and that there is a process
>  with that PID around. Right now, ncf_if_xml_state will report whether
>  the interface is _configured_ to use DHCP, not whether it is getting its
>  address with DHCP.
>      

Hmm. Just how far do we want to go down that road? For example, maybe 
dhclient is running, but someone logged in and ifconfiged the interface, 
leaving it with a different address until the next time dhclient thinks 
it needs to renew. *then* what/how do we report?

My opinion is that, while it's something anyone using the returned info 
should consider, taking it into account goes beyond the call of duty.


>  The interface.rng currently also doesn't allow both
> a<dhcp/>  element
>  and an<ip/>  entry - in the future it will, but with a different
>  meaning. 

Ah, right. So since I'm modifying the tree just before spitting it out 
as ASCII, it never gets validated, and never sees that I've broken the 
rules!


>  So, I wonder if there is a need to add something to the<ip/>
>  element to indicate that the address is the live address.
>      

I think it's implied in which function is called that it's the live 
address. What I wonder is if anyone will ever want to know that, for 
example, the config file has one particular static IP, but the interface 
is currently set to something different. If that's important to get in a 
single call, then we would need something added to <ip/> to 
differentiate between static and live addresses.

Yo'ure just suggesting adding something like 'live="true"' (or
whatever, 
maybe 'source="device"' vs. 'source="config"'?) to an
otherwise 
identically formatted node, right?

[PATCH 3/3] Add a new API function ncf_if_xml_state().

[David Lutterkort]

On Thu, 2009-09-24 at 17:11 -0400, Laine Stump wrote:
    
>  >> @@ -737,6 +737,64 @@ char *drv_xml_desc(struct netcf_if
> *nif) {
>  >>       goto done;
>  >>   }
>  >>
>  >> +/* return the current live configuration state - a combination of
>  >> + * drv_xml_desc + results of querying the interface directly */
>  >> +
>  >> +char *drv_xml_state(struct netcf_if *nif) {
>  >> +    char *result = NULL;
>  >> +    int r, result_len;
>  >> +    struct augeas *aug;
>  >> +    struct netcf *ncf;
>  >> +    char **devs = NULL, **intf = NULL;
>  >> +    xmlDocPtr aug_xml = NULL, ncf_xml = NULL;
>  >> +    int ndevs = 0, nint = 0;
>  >> +    unsigned int ipv4;
>  >> +    int prefix;
>  >> +
>  >> +    ncf = nif->ncf;
>  >> +    aug = get_augeas(ncf);
>  >> +    ERR_BAIL(ncf);
>  >> +
>  >> +    ndevs = aug_fmt_match(ncf,&devs,
>  >> +              "%s[ DEVICE = '%s' or BRIDGE = '%s' or
> MASTER = '%s']/DEVICE",
>  >> +              ifcfg_path, nif->name, nif->name, nif->name);
>  >> +    ERR_BAIL(ncf);
>  >> +
>  >> +    nint = uniq_ifcfg_paths(ncf, ndevs, devs,&intf);
>  >> +    ERR_BAIL(ncf);
>  >> +
>  >> +    aug_xml = aug_get_xml(ncf, nint, intf);
>  >> +
>  >> +
>  >> +    /* same as drv_xml_desc() up to this point, a bit different from now on...
> */
>  >>      
>  > Can that be factored into some helper function that drv_xml_desc and
>  > drv_xml_state both use ?
>  >    
>  
>  
>  Does "aug_get_xml" is already taken by the function that gets the 
>  aug_xml for all interfaces. How about I name this function 
>  aug_get_one_xml? Or do you have a better idea? 
How about aug_get_xml_for_nif, since it takes a netcf_if and returns the
parts of the Augeas tree that are important for that interface.

>  
>  >    
>  >> +    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 */
>  >> +    ipv4 = if_ipv4_address(ncf, nif->name);
>  >>      
>  > if_ipv4_address should set ncf->errcode (with one of the ERR_ macros),
>  > and you should use ERR_BAIL(ncf) after the invocation to make sure
>  > errors ar propagated. That also means you don't have the 'if (ipv4 !=
>  > 0)' check.
>  >    
>  
>  
>  Actually I was assuming that, in the case of an interface that didn't 
>  yet have an IP address ifconfiged, the ioctl would return a 0 result, 
>  but no error. In this case you wouldn't want to add 'ip 
>  address="0.0.0.0"' to the document. So I think I should have the if_* 
>  functions setting errcode, but I still need to check for != 0. 
Agreed - you need to handle both, some sort of error (OOM etc.) and that
the interface has no address assigned.

>  Yeah, I agree. I actually dislike goto's and labels (knee-jerk
> reflex 
>  from from my first quarter freshman year data structures class (so long 
>  ago it was taught in Pascal)), but I've seen so many lately it's become 
>  background noise. 
Dijkstra lives ! I try to only use goto's where you'd throw an xception
in other languages, and have only at most two labels in a function, one
for successful exit, and one for error exit + cleanup.

>  > The interface.rng currently also doesn't allow both
> a<dhcp/>  element
>  > and an<ip/>  entry - in the future it will, but with a different
>  > meaning.
>  
>  
>  Ah, right. So since I'm modifying the tree just before spitting it out 
>  as ASCII, it never gets validated, and never sees that I've broken the 
>  rules! 
I am not particularly worried about that - just something to keep in
mind.

>  > So, I wonder if there is a need to add something to
> the<ip/>
>  > element to indicate that the address is the live address.
>  >    
>  
>  
>  I think it's implied in which function is called that it's the live 
>  address. What I wonder is if anyone will ever want to know that, for 
>  example, the config file has one particular static IP, but the interface 
>  is currently set to something different. If that's important to get in a 
>  single call, then we would need something added to <ip/> to 
>  differentiate between static and live addresses. 
For that, they can always call the two different XML-generating
functions and hunt through the docs themselves.

>  Yo'ure just suggesting adding something like
> 'live="true"' (or whatever, 
>  maybe 'source="device"' vs. 'source="config"'?) to an
> otherwise 
>  identically formatted node, right? 
Yeah, something like that - might be overkill, but I like the
'source="device"' annotation.

David

[PATCH 3/3] (take two) Add a new API function ncf_if_xml_state().

[Laine Stump]

(I *think* I got all your suggestions in here, including rebase. I
will be out for a few hours starting at 2PM EST (Dr. appt), so if you
want any changes, try to mail back early. Also, it wasn't clear to me
if the rng needs to be changed right away to allow for dhcp and an ip
address to be set at the same time (was that added in with your ipv6
commit?); likewise - I guess we need to add something to allow for
'source="device"', but I'm not familiar enough with RNG to know
what
to put where.)

ncf_if_xml_state() returns the current live state of the interface,
which is just the same document returned by ncf_if_xml_desc(), with
the addition/modification of IP address and netmask (prefix) to the
values retrieved directly from the interface. This is useful to learn
the IP address assigned to an interface configured to used DHCP.

Any ip address retrieved in this manner will have a property "source"
set to "device", as a clue for the consumer.

Currently only the 1st IPv4 address on an interface is added. The next
step will be to use the netlink socket (rather than
ioctl(SOICGIFxxxx)) to get all IPv4 and IPv6 addresses for the
interface.
---
 src/drv_initscripts.c |   68 +++++++++++++++++++++++++----
 src/dutil.c           |  115 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/dutil.h           |   14 ++++++
 src/internal.h        |    1 +
 src/ncftool.c         |    8 +++-
 src/netcf.c           |   10 ++++
 src/netcf.h           |   14 +++++-
 src/netcf_public.syms |    5 ++
 8 files changed, 222 insertions(+), 13 deletions(-)

diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
index 02b7e6e..e8c4698 100644
--- a/src/drv_initscripts.c
+++ b/src/drv_initscripts.c
@@ -702,18 +702,16 @@ static int aug_put_xml(struct netcf *ncf, xmlDocPtr xml) {
     return result;
 }
 
-char *drv_xml_desc(struct netcf_if *nif) {
-    char *result = NULL;
-    struct augeas *aug;
+/* given a netcf_if, get the config for an interface in the simple
+ * Augeas format
+ */
+static xmlDocPtr aug_get_xml_for_nif(struct netcf_if *nif) {
     struct netcf *ncf;
     char **devs = NULL, **intf = NULL;
     xmlDocPtr aug_xml = NULL;
     int ndevs = 0, nint = 0;
 
     ncf = nif->ncf;
-    aug = get_augeas(ncf);
-    ERR_BAIL(ncf);
-
     ndevs = aug_fmt_match(ncf, &devs,
               "%s[ DEVICE = '%s' or BRIDGE = '%s' or MASTER =
'%s'"
               "    or MASTER = ../*[BRIDGE = '%s']/DEVICE ]/DEVICE",
@@ -725,12 +723,64 @@ char *drv_xml_desc(struct netcf_if *nif) {
 
     aug_xml = aug_get_xml(ncf, nint, intf);
 
+ error:
+    free_matches(ndevs, &devs);
+    free_matches(nint, &intf);
+    return aug_xml;
+}
+
+/* return the current static configuration (as saved on disk) */
+char *drv_xml_desc(struct netcf_if *nif) {
+    char *result = NULL;
+    struct netcf *ncf;
+    xmlDocPtr aug_xml = NULL;
+
+    ncf = nif->ncf;
+    aug_xml = aug_get_xml_for_nif(nif);
+    ERR_BAIL(ncf);
+
     result = apply_stylesheet_to_string(ncf, ncf->driver->put, aug_xml);
+
+ error:
+    xmlFreeDoc(aug_xml);
+    return result;
+}
+
+/* return the current live configuration state - a combination of
+ * drv_xml_desc + results of querying the interface directly */
+
+char *drv_xml_state(struct netcf_if *nif) {
+    char *result = NULL;
+    int r, result_len;
+    struct netcf *ncf;
+    xmlDocPtr aug_xml = NULL, ncf_xml = NULL;
+    unsigned int ipv4;
+    int prefix;
+
+    ncf = nif->ncf;
+    aug_xml = aug_get_xml_for_nif(nif);
     ERR_BAIL(ncf);
 
- done:
-    free_matches(ndevs, &devs);
-    free_matches(nint, &intf);
+    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 */
+    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);
+    }
+
+    r = xsltSaveResultToString((xmlChar **)&result, &result_len,
+                               ncf_xml, ncf->driver->put);
+    ERR_NOMEM(r < 0, ncf);
+
+done:
+    xmlFreeDoc(ncf_xml);
     xmlFreeDoc(aug_xml);
     return result;
  error:
diff --git a/src/dutil.c b/src/dutil.c
index d51994f..e2d05b8 100644
--- a/src/dutil.c
+++ b/src/dutil.c
@@ -35,6 +35,9 @@
 #include <fcntl.h>
 #include <sys/ioctl.h>
 #include <net/if.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+
 
 #include "safe-alloc.h"
 #include "ref.h"
@@ -435,11 +438,53 @@ int if_is_active(struct netcf *ncf, const char *intf) {
     strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
     ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
     if (ioctl(ncf->driver->ioctl_fd, SIOCGIFFLAGS, &ifr))  {
+        report_error(ncf, NETCF_EIOCTL, "Failed to get interface flags for %s",
intf);
         return 0;
     }
     return ((ifr.ifr_flags & IFF_UP) == IFF_UP);
 }
 
+unsigned int if_ipv4_address(struct netcf *ncf, const char *intf) {
+    struct ifreq ifr;
+
+    MEMZERO(&ifr, 1);
+    strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
+    ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
+    if (ioctl(ncf->driver->ioctl_fd, SIOCGIFADDR, &ifr))  {
+        report_error(ncf, NETCF_EIOCTL, "Failed to get ipv4 address for %s",
intf);
+        return 0;
+    }
+
+    return (((struct sockaddr_in *)&ifr.ifr_addr)->sin_addr.s_addr);
+}
+
+unsigned int if_ipv4_netmask(struct netcf *ncf, const char *intf) {
+    struct ifreq ifr;
+
+    MEMZERO(&ifr, 1);
+    strncpy(ifr.ifr_name, intf, sizeof(ifr.ifr_name));
+    ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
+    if (ioctl(ncf->driver->ioctl_fd, SIOCGIFNETMASK, &ifr))  {
+        report_error(ncf, NETCF_EIOCTL, "Failed to get ipv4 netmask for %s",
intf);
+        return 0;
+    }
+
+    return (((struct sockaddr_in *)&ifr.ifr_netmask)->sin_addr.s_addr);
+}
+
+
+int if_ipv4_prefix(struct netcf *ncf, const char *intf) {
+    unsigned int nmv4 = if_ipv4_netmask(ncf, intf);
+    int prefix = 0;
+
+    nmv4 = ntohl(nmv4);
+    while ((nmv4 & 0xFFFFFFFF) != 0) {
+        nmv4 <<= 1;
+        prefix++;
+    }
+    return prefix;
+}
+
 /* Create a new netcf if instance for interface NAME */
 struct netcf_if *make_netcf_if(struct netcf *ncf, char *name) {
     int r;
@@ -499,6 +544,76 @@ int dutil_put_aug(struct netcf *ncf, const char *aug_xml, char
**ncf_xml) {
     return result;
 }
 
+/* Given an xml document that follows interface.rng, add the IP
+ * address and prefix under protocol/ip
+ */
+void add_state_to_xml_doc(xmlDocPtr doc, struct netcf *ncf ATTRIBUTE_UNUSED,
+                          unsigned int ipv4, int prefix) {
+
+    xmlNodePtr root = NULL, proto = NULL, ip = NULL, cur;
+    xmlAttrPtr prop = NULL;
+    char ip_str[48], prefix_str[16];
+
+
+    root = xmlDocGetRootElement(doc);
+    ERR_THROW((root == NULL), ncf, EINTERNAL, "failed to get document root
element");
+
+    ERR_THROW(!xmlStrEqual(root->name, BAD_CAST "interface"),
+              ncf, EINTERNAL, "root document is not an interface");
+
+    for (cur = root->children; cur != NULL; cur = cur->next) {
+        if ((cur->type == XML_ELEMENT_NODE) &&
+            (xmlStrEqual(cur->name, BAD_CAST "protocol"))) {
+            xmlChar *family = xmlGetProp(cur, BAD_CAST "family");
+            if (family != NULL) {
+                if (xmlStrEqual(family, BAD_CAST "ipv4"))
+                    proto = cur;
+                xmlFree(family);
+                if (proto != NULL) {
+                    break;
+                }
+            }
+        }
+    }
+    if (proto == NULL) {
+        proto = xmlNewDocNode(doc, NULL, BAD_CAST "protocol", NULL);
+        ERR_NOMEM((proto == NULL), ncf);
+
+        cur = xmlAddChild(root, proto);
+        ERR_NOMEM((cur == NULL), ncf);
+        prop = xmlSetProp(proto, BAD_CAST "family", BAD_CAST
"ipv4");
+        ERR_NOMEM((prop == NULL), ncf);
+
+    }
+
+    for (cur = proto->children; cur != NULL; cur = cur->next) {
+        if ((cur->type == XML_ELEMENT_NODE) &&
+            (xmlStrEqual(cur->name, BAD_CAST "ip"))) {
+            break;
+        }
+    }
+
+    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);
+    }
+
+    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;
+}
+
 /*
  * Bringing interfaces up/down
  */
diff --git a/src/dutil.h b/src/dutil.h
index 094324f..0a9715f 100644
--- a/src/dutil.h
+++ b/src/dutil.h
@@ -112,6 +112,17 @@ int init_ioctl_fd(struct netcf *ncf);
 /* Check if the interface INTF is up using an ioctl call */
 int if_is_active(struct netcf *ncf, const char *intf);
 
+/* Get the current IPv4 address of INTF as an unsigned int using an ioctl call */
+unsigned int if_ipv4_address(struct netcf *ncf, const char *intf);
+
+/* get the current IPv4 netmask of INTF as an unsigned int using an ioctl call */
+unsigned int if_ipv4_netmask(struct netcf *ncf, const char *intf);
+
+/* get the current IPv4 prefix (#bits of netmask) of INTF as an
+ * int using an ioctl call */
+
+int if_ipv4_prefix(struct netcf *ncf, const char *intf);
+
 /* Create a new netcf if instance for interface NAME */
 struct netcf_if *make_netcf_if(struct netcf *ncf, char *name);
 
@@ -121,6 +132,9 @@ int dutil_get_aug(struct netcf *ncf, const char *ncf_xml, char
**aug_xml);
 /* Transform the Augeas XML AUG_XML into interface XML NCF_XML */
 int dutil_put_aug(struct netcf *ncf, const char *aug_xml, char **ncf_xml);
 
+/* add the given state (currently IP + netmask) to the interface's xml document */
+void add_state_to_xml_doc(xmlDocPtr doc, struct netcf *ncf, unsigned int ipv4, int
prefix);
+
 /* Run the program PROG with the single argument ARG */
 void run1(struct netcf *ncf, const char *prog, const char *arg);
 
diff --git a/src/internal.h b/src/internal.h
index 7205e72..ca9a0ef 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -179,6 +179,7 @@ struct netcf_if *drv_lookup_by_name(struct netcf *ncf, const char
*name);
 int drv_lookup_by_mac_string(struct netcf *, const char *mac,
                              int maxifaces, struct netcf_if **ifaces);
 char *drv_xml_desc(struct netcf_if *);
+char *drv_xml_state(struct netcf_if *);
 const char *drv_mac_string(struct netcf_if *nif);
 struct netcf_if *drv_define(struct netcf *ncf, const char *xml);
 int drv_undefine(struct netcf_if *nif);
diff --git a/src/ncftool.c b/src/ncftool.c
index a1362c2..3639dd7 100644
--- a/src/ncftool.c
+++ b/src/ncftool.c
@@ -228,7 +228,11 @@ static int cmd_dump_xml(const struct command *cmd) {
         goto done;
     }
 
-    xml = ncf_if_xml_desc(nif);
+    if (opt_present(cmd, "live")) {
+        xml = ncf_if_xml_state(nif);
+    } else {
+        xml = ncf_if_xml_desc(nif);
+    }
     if (xml == NULL)
         goto done;
 
@@ -246,6 +250,8 @@ static const struct command_opt_def cmd_dump_xml_opts[] = {
       .help = "interpret the name as a MAC address" },
     { .tag = CMD_OPT_ARG, .name = "name",
       .help = "the name of the interface" },
+    { .tag = CMD_OPT_BOOL, .name = "live",
+      .help = "show show live state of the interface, rather than just config file
settings" },
     CMD_OPT_DEF_LAST
 };
 
diff --git a/src/netcf.c b/src/netcf.c
index 7ca4fd9..97d3b17 100644
--- a/src/netcf.c
+++ b/src/netcf.c
@@ -200,6 +200,16 @@ char *ncf_if_xml_desc(struct netcf_if *nif) {
     return drv_xml_desc(nif);
 }
 
+/* Produce an XML description of the current live state of the
+ * interface, in the same format that NCF_DEFINE expects, but
+ * potentially with extra info not contained in the static config (ie
+ * the current IP address of an interface that uses DHCP)
+ */
+char *ncf_if_xml_state(struct netcf_if *nif) {
+    API_ENTRY(nif->ncf);
+    return drv_xml_state(nif);
+}
+
 /* Release any resources used by this NETCF_IF; the pointer is invalid
  * after this call
  */
diff --git a/src/netcf.h b/src/netcf.h
index 482b6d0..4219cc1 100644
--- a/src/netcf.h
+++ b/src/netcf.h
@@ -51,7 +51,8 @@ typedef enum {
     NETCF_EINUSE,        /* attempt to close a netcf instance that is still
                           * used by other data structures */
     NETCF_EXSLTFAILED,   /* XSLT transformation failed */
-    NETCF_EFILE          /* Some file access failed */
+    NETCF_EFILE,         /* Some file access failed */
+    NETCF_EIOCTL         /* An ioctl call failed */
 } netcf_errcode_t;
 
 
@@ -149,11 +150,18 @@ int ncf_if_down(struct netcf_if *);
 /* Delete the definition */
 int ncf_if_undefine(struct netcf_if *);
 
-/* Produce an XML description for the interface, in the same format that
- * NCF_DEFINE expects
+/* Produce an XML description for the static (stored) interface
+ * config, in the same format that NCF_DEFINE expects
  */
 char *ncf_if_xml_desc(struct netcf_if *);
 
+/* Produce an XML description of the current live state of the
+ * interface, in the same format that NCF_DEFINE expects, but
+ * potentially with extra info not contained in the static config (ie
+ * the current IP address of an interface that uses DHCP)
+ */
+char *ncf_if_xml_state(struct netcf_if *);
+
 /* Release any resources used by this NETCF_IF; the pointer is invalid
  * after this call
  */
diff --git a/src/netcf_public.syms b/src/netcf_public.syms
index 230b6bf..d2d1832 100644
--- a/src/netcf_public.syms
+++ b/src/netcf_public.syms
@@ -18,3 +18,8 @@ NETCF_1.0.0 {
     local:
       *;
 };
+
+NETCF_1.1.0 {
+    global:
+      ncf_if_xml_state;
+} NETCF_1.0.0;
-- 
1.6.2.5

[PATCH 3/3] (take two) Add a new API function ncf_if_xml_state().

[David Lutterkort]

On Fri, 2009-09-25 at 09:36 -0400, Laine Stump wrote:
>  (I *think* I got all your suggestions in here, including rebase.
ACK. This looks good; I made some minor formatting changes and changed
the commit message + pushed.

One minor nit: for interfaces with static IP's, you get something like
this:

        <?xml version="1.0"?>
        <interface type="bridge" name="br0">
          <start mode="onboot"/>
          <protocol family="ipv4">
            <ip address="172.31.0.9" prefix="24"/>
            <route gateway="172.31.0.1"/>
            <ip address="172.31.0.9" prefix="24"
source="device"/>
          </protocol>
          <bridge delay="0">
            <interface type="ethernet" name="eth0">
              <mac address="00:1c:c0:89:dd:01"/>
            </interface>
          </bridge>
        </interface>
        
The duplication doesn't bother me, since I would look for the live
address with "ip[@source='device']" so that the other <ip/>
element
doesn't matter.

>  Also, it wasn't clear to me
>  if the rng needs to be changed right away to allow for dhcp and an ip
>  address to be set at the same time (was that added in with your ipv6
>  commit?); likewise - I guess we need to add something to allow for
>  'source="device"', but I'm not familiar enough with RNG to know
> what
>  to put where.) 
Dunno ... since the interface.rng defines what you can use with
ncf_define, it shouldn't go into interface.rng. And forking the
interface.rng will be a lot of work (I mean: making that fork
maintainable)

For the time being, a decent textual description (look for "ip[@device =
'source']" in the returned XML) should be good enough.

David

[PATCH 3/3] (take two) Add a new API function ncf_if_xml_state().

[Laine Stump]

On 09/26/2009 08:28 PM, David Lutterkort wrote:
>  On Fri, 2009-09-25 at 09:36 -0400, Laine Stump wrote:
>     
> > (I *think* I got all your suggestions in here, including rebase.
> >      
>  ACK. This looks good; I made some minor formatting changes and changed
>  the commit message + pushed.
>
>  One minor nit: for interfaces with static IP's, you get something like
>  this:
>
>           <?xml version="1.0"?>
>           <interface type="bridge" name="br0">
>             <start mode="onboot"/>
>             <protocol family="ipv4">
>               <ip address="172.31.0.9" prefix="24"/>
>               <route gateway="172.31.0.1"/>
>               <ip address="172.31.0.9" prefix="24"
> source="device"/>
>      

Shoot! That wasn't intended, and it either wasn't working that way at 
some point, or I didn't notice the extra node when looking at the output 
(but it's there plain as day now). Do you want a patch to eliminate the 
duplicate? (It almost sounds like you prefer it this way.)

[PATCH 3/3] (take two) Add a new API function ncf_if_xml_state().

[David Lutterkort]

On Sat, 2009-09-26 at 22:48 -0400, Laine Stump wrote:
>  On 09/26/2009 08:28 PM, David Lutterkort wrote:
>  > On Fri, 2009-09-25 at 09:36 -0400, Laine Stump wrote:
>  >    
>  >> (I *think* I got all your suggestions in here, including rebase.
>  >>      
>  > ACK. This looks good; I made some minor formatting changes and changed
>  > the commit message + pushed.
>  >
>  > One minor nit: for interfaces with static IP's, you get something like
>  > this:
>  >
>  >          <?xml version="1.0"?>
>  >          <interface type="bridge" name="br0">
>  >            <start mode="onboot"/>
>  >            <protocol family="ipv4">
>  >              <ip address="172.31.0.9" prefix="24"/>
>  >              <route gateway="172.31.0.1"/>
>  >              <ip address="172.31.0.9" prefix="24"
> source="device"/>
>  >    
>  
>  
>  Shoot! That wasn't intended, and it either wasn't working that way at 
>  some point, or I didn't notice the extra node when looking at the output 
>  (but it's there plain as day now). Do you want a patch to eliminate the 
>  duplicate? (It almost sounds like you prefer it this way.) 
Yeah, a cleanup patch would be good.

David

[PATCH 3/3] (take two) Add a new API function ncf_if_xml_state().

[Laine Stump]

On 09/27/2009 12:25 AM, David Lutterkort wrote:
>  On Sat, 2009-09-26 at 22:48 -0400, Laine Stump wrote:
>     
> > On 09/26/2009 08:28 PM, David Lutterkort wrote:
> >      
> >> On Fri, 2009-09-25 at 09:36 -0400, Laine Stump wrote:
> >>
> >>        
> >>> (I *think* I got all your suggestions in here, including rebase.
> >>>
> >>>          
> >> ACK. This looks good; I made some minor formatting changes and changed
> >> the commit message + pushed.
> >>
> >> One minor nit: for interfaces with static IP's, you get something like
> >> this:
> >>
> >>           <?xml version="1.0"?>
> >>           <interface type="bridge" name="br0">
> >>             <start mode="onboot"/>
> >>             <protocol family="ipv4">
> >>               <ip address="172.31.0.9" prefix="24"/>
> >>               <route gateway="172.31.0.1"/>
> >>               <ip address="172.31.0.9" prefix="24"
> source="device"/>
> >>
> >>        
> >
> > Shoot! That wasn't intended, and it either wasn't working that way at
> > some point, or I didn't notice the extra node when looking at the output
> > (but it's there plain as day now). Do you want a patch to eliminate the
> > duplicate? (It almost sounds like you prefer it this way.)
> >      
>  Yeah, a cleanup patch would be good.
>      
In the case of an interface that has a static IP in the config, but is 
currently down, it's not possible to get any addresses from the 
interface. In that case, should there be no IP addresses, or should the 
originals from the config file be returned? (I'm leaning toward no 
addresses, but could possibly be convinced otherwise).

[PATCH 3/3] (take two) Add a new API function ncf_if_xml_state().

[David Lutterkort]

On Mon, 2009-09-28 at 21:46 -0400, Laine Stump wrote:
>  On 09/27/2009 12:25 AM, David Lutterkort wrote:
>  > On Sat, 2009-09-26 at 22:48 -0400, Laine Stump wrote:
>  >    
>  >> On 09/26/2009 08:28 PM, David Lutterkort wrote:
>  >>      
>  >>> One minor nit: for interfaces with static IP's, you get something like
>  >>> this:
>  >>>
>  >>>           <?xml version="1.0"?>
>  >>>           <interface type="bridge" name="br0">
>  >>>             <start mode="onboot"/>
>  >>>             <protocol family="ipv4">
>  >>>               <ip address="172.31.0.9"
> prefix="24"/>
>  >>>               <route gateway="172.31.0.1"/>
>  >>>               <ip address="172.31.0.9" prefix="24"
> source="device"/>
>  >>>
>  >>>        
>  >>
>  >> Shoot! That wasn't intended, and it either wasn't working that way at
>  >> some point, or I didn't notice the extra node when looking at the output
>  >> (but it's there plain as day now). Do you want a patch to eliminate the
>  >> duplicate? (It almost sounds like you prefer it this way.)
>  >>      
>  > Yeah, a cleanup patch would be good.
>  >    
>  
>  In the case of an interface that has a static IP in the config, but is 
>  currently down, it's not possible to get any addresses from the 
>  interface. In that case, should there be no IP addresses, or should the 
>  originals from the config file be returned? (I'm leaning toward no 
>  addresses, but could possibly be convinced otherwise). 
That brings up the big question: what should ncf_if_cml_state return ?
Just information about the live configuration or a combination of live
config + and ondisk config ? I am leaning towards the latter, especially
since we mark the live config with 'source="device"' .. in that case,
the XML above is correct in having two <ip/> elements.

David

[David Lutterkort]

On Tue, 2009-09-22 at 16:49 -0400, Laine Stump wrote:
>  Note that I just added the function name into the public symbol list
>  in the NETCF_1.0.0 section, since the actual version is still at
>  0.1.1, so I'm not sure what to do with it... 
The version of the DSO is _not_ coupled to the version of netcf - I
follow the libtool recommendations for versioning the DSO[1] It leads to
version numbers M.N.R for DSO's where M is only ever incremented if the
interface changes in a backwards incompatible way, N is incremented when
the interface is expanded, and R is incremented when the implementation
changes. (Note that that is _not_ the way it is written down in
configure.ac)

That means that your additions should go into a section

  NETCF_1.1.0 {
        global:
          ncf_if_xml_state;
  } NETCF_1.0.0;

in src/netcf_public.syms.

David

[1] http://www.gnu.org/software/libtool/manual/html_node/Versioning.html