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

David Lutterkort lutter at redhat.com
Wed Sep 23 19:27:52 UTC 2009


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




More information about the netcf-devel mailing list