Preparing for aditional drivers
by Jonas Eriksson
Hello,
This is the last set of patches before the amount of shared code
between the SuSE-driver (in its current state, at least) and the
current initscripts driver is almost nil.
The last two patches were only discussed briefly, so I kept them
at the end. Feel free to react, and note that it means that all
"variables" in an ifcfg-file will get single quotes. The lens is
however able to read unquoted variables from these files.
For the SuSE-driver, I currently lack VLAN-support (which on SuSE
is a mess in some aspects), and some tests. Hopefully, these
things won't take long to fix.
/Jonas
14 years, 7 months
mac-tag in XML
by Jonas Eriksson
Hi,
Is the <mac> tag in XML to be used to search for interfaces or to
set the mac address of that interface that contains the mac tag?
As this is converted to HWADDR and, according to the info that I
have found, I have asumed that this is for searching for the
correct interface.
This proved a bit difficult on SuSE, as the connection device
name <-> mac address is handled strictly by persistent udev-rules
written when an interface is first found. I solved this by
implementing the following prio logic:
1. xmlif.name == hostif.name && xmlif.macaddr == hostif.macaddr
2. xmlif.macaddr == hostif.macaddr
3. xmlif.name == hostif.name
This may lead to interface name changing when doing a
define/dumpxml-cycle given that an interface with the wrong name
but correct mac address is found. Is this the intended way of
netcf's mac address handling?
If this assumption is correct, why cannot the interface-tag
inside the vlan-tag in netcf-xml contain a mac-tag? If this is
wrong, i hope that we can mitigate this even though the xml
schema is considered stable.
/Jonas
--
Jonas Eriksson
Consultant at AS/EAB/FLJ/IL
Phone: +46 8 58086281
Combitech AB
Älvsjö, Sweden
14 years, 8 months
Suggestions (renaming drv_initscripts, changing the internal xml format, displaying augeas errors)
by Jonas Eriksson
Hi,
I thought that I should sum up some suggestions I would like to
discuss and if possible implement or wake up one day and see them
implemented (which happened today :) ).
How about renaming the initscripts driver to redhat or something
similar? The SuSE driver will also target the initscript network
configuration layer, and hopefully more drivers in the future as
well.
I suggest that we change the xml format that data/xml/augeas.rng
reflect, by changing the path attribute to an interface name
attribute. The reason for this is that the path is quite
irrelevant inside the XSLT, and only leads to duplication of data
(i.e. the ifcfg_path as it is called in drv_initscripts.c).
(I raised this one in one of my commit mails. This was probaly
not the best place, so I'll lift it out here instead.)
When I was debugging a problem with aug_save, I noticed that it
was quite hard to get exact error messages from augeas. I of
added temporary debuging code that gave me this information,
although augeas information ought to be more easily accessible
than this. I therefore suggest adding the error state EAUGEAS (or
similar) that would trigger a dump of /augeas//error and
/augeas//error/message from the ERR_* macros. If this would be to
much information, it could be enabled through the planned debug
mode.
Regards,
Jonas
--
Jonas Eriksson
Consultant at AS/EAB/FLJ/IL
Combitech AB
Älvsjö, Sweden
14 years, 8 months
[PATCH] Make lookup-by-MAC case-insensitive
by David Lutterkort
The /sys file system stores MAC's in all-lowercase; we therefore have to
translate MAC's to lowercase before looking up.
Fixes BZ 512955
* bootstrap: use c-ctype module from gnulib
* src/dutil-linux.c (aug_match_mac): use tolower'd MAC for lookup
* tests/test-initscripts.c: test correct lookup of mixed-case MACs
---
bootstrap | 1 +
src/dutil_linux.c | 14 +++++++++++---
tests/test-initscripts.c | 10 ++++++++++
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/bootstrap b/bootstrap
index 91225fb..f0653ef 100755
--- a/bootstrap
+++ b/bootstrap
@@ -58,6 +58,7 @@ gnulib_tool=$GNULIB_SRCDIR/gnulib-tool
<$gnulib_tool || exit
modules='
+c-ctype
read-file
safe-alloc
warnings
diff --git a/src/dutil_linux.c b/src/dutil_linux.c
index cd87d3b..1878719 100644
--- a/src/dutil_linux.c
+++ b/src/dutil_linux.c
@@ -31,6 +31,7 @@
#include <string.h>
#include <unistd.h>
#include <ctype.h>
+#include <c-ctype.h>
#include "safe-alloc.h"
#include "ref.h"
@@ -42,11 +43,17 @@
/* Returns a list of all interfaces with MAC address INTF */
int aug_match_mac(struct netcf *ncf, const char *mac, char ***matches) {
int r, nmatches;
- char *path;
+ char *path, *mac_lower;
struct augeas *aug = get_augeas(ncf);
+ mac_lower = strdup(mac);
+ ERR_COND_BAIL(mac_lower == NULL, ncf, ENOMEM);
+ for (char *s = mac_lower; *s != '\0'; s++)
+ *s = c_tolower(*s);
+
r = xasprintf(&path,
- "/files/sys/class/net/*[address/content = '%s']", mac);
+ "/files/sys/class/net/*[address/content = '%s']", mac_lower);
+ FREE(mac_lower);
ERR_COND_BAIL(r < 0, ncf, ENOMEM);
r = aug_match(aug, path, matches);
@@ -72,8 +79,9 @@ int aug_match_mac(struct netcf *ncf, const char *mac, char ***matches) {
return nmatches;
error:
+ FREE(mac_lower);
FREE(path);
- return r;
+ return -1;
}
/* Get the MAC address of the interface INTF */
diff --git a/tests/test-initscripts.c b/tests/test-initscripts.c
index 7f62c0b..253aef4 100644
--- a/tests/test-initscripts.c
+++ b/tests/test-initscripts.c
@@ -251,12 +251,14 @@ static void testLookupByName(CuTest *tc) {
static void testLookupByMAC(CuTest *tc) {
static const char *const good_mac = "aa:bb:cc:dd:ee:ff";
+ static const char *const good_mac_caps = "AA:bb:cc:DD:Ee:ff";
struct netcf_if *nif;
int r;
r = ncf_lookup_by_mac_string(ncf, "00:00:00:00:00:00", 1, &nif);
CuAssertIntEquals(tc, 0, r);
CuAssertPtrEquals(tc, NULL, nif);
+
r = ncf_lookup_by_mac_string(ncf, good_mac, 1, &nif);
CuAssertIntEquals(tc, 1, r);
CuAssertPtrNotNull(tc, nif);
@@ -264,6 +266,14 @@ static void testLookupByMAC(CuTest *tc) {
CuAssertStrEquals(tc, good_mac, ncf_if_mac_string(nif));
ncf_if_free(nif);
CuAssertIntEquals(tc, 1, ncf->ref);
+
+ r = ncf_lookup_by_mac_string(ncf, good_mac_caps, 1, &nif);
+ CuAssertIntEquals(tc, 1, r);
+ CuAssertPtrNotNull(tc, nif);
+ CuAssertStrEquals(tc, "br0", nif->name);
+ CuAssertStrEquals(tc, good_mac, ncf_if_mac_string(nif));
+ ncf_if_free(nif);
+ CuAssertIntEquals(tc, 1, ncf->ref);
}
static void testDefineUndefine(CuTest *tc) {
--
1.6.0.6
14 years, 8 months
Break out driver utility functions
by Jonas Eriksson
Hi,
First of, sorry for my absence on this list. When the others leave for
vacation, the work load apparently increases for the ones that do not :)
Also, thanks for all your input earlier, David.
This is my first effort to divide utility functions into
OS-independent (dutil.c) and OS-dependent (dutil_linux.c) and move
them out of the drivers.
It is noteworthy that the 4th test (testDefineUndefine) started
failing after I added the 2nd patch. After some debugging, this seems
to be due to augeas trying to save /sys/class/net/br0/address, which I
thought was kind of strange. I attached gdb and set breakpoints on
aug_set, aug_rm, aug_mv and aug_insert but was unable to find any call
that changed /files/sys/class/net/br0/address. Does someone else
experience the same problem, or is there something wrong with my
libaugeas build?
For my future projects, besides adapting the suse-driver according to
Davids suggestions and this commit, I am thinking about adding debug
macros (as mentioned) and improve error reporting. I would like to get
some opinions on this suggestion:
When I was debugging the problem above with aug_save, it was quite
hard to get exact error messages from augeas. I of course added
temporary debuging code that gave me this information, although augeas
information ought to be more easily accessible than this. I therefore
suggest adding a EAUGEAS or similar that would trigger a dump of
/augeas//error and /augeas//error/message from ERR_*.
Regards,
Jonas
14 years, 9 months
Replace aug_submatch?
by Jonas Eriksson
Hi,
while bringing the suse-driver up to speed, I noted this very
common pattern with the path variable only used once:
r = xasprintf(&pathx, {format}, ..);
ERR_COND_BAIL(r < 0, ncf, ENOMEM);
nint = aug_get(aug, pathx, {&var or NULL});
ERR_COND_BAIL(nint < 0, ncf, EOTHER);
..or with other names on the variables, and/or aug_match instead
of aug_get. This has probably been noticed by David earlier, and
has lead to the creation of the aug_submatch function to simplify
the case of path=path1/path2.
I would however suggest a more general approach, in the lines of:
r = aug_printf{get,match}(ncf, {&var or NULL}, {format}, ..);
(or with a less bloated name, like for example aug_pf{get,match})
This means that this:
nmatches = aug_submatch(ncf, intf[i], "*", &matches);
ERR_COND_BAIL(nmatches < 0, ncf, EOTHER);
Will be replaced by this:
nmatches = aug_printfmatch(ncf, &matches, "%s/%s", intf[i], "*");
ERR_BAIL(ncf);
..which is slightly more code, but easier to follow without
looking up what the function realy does. The option still exist
to keep aug_submatch, the simplest way would be through a macro
like:
aug_submatch(ncf, p1, p2, m) aug_printfmatch(ncf, m, "%s/%s", p1, p2);
Although I would recommend to move to the aug_printf-approach
even for the current use of aug_submatch, as aug_submatch is used
4 times currently, and aug_printfmatch could be used in about 5
cases (excluding the uses of aug_submatch today).
The only negative thing I could think about in this approach is
the signature of aug_printfmatch, which does not adhere to the
existing aug_*-functions as these place the path variable first
in all cases.
Other opinions? I am open to an addition of aug_printfset (or
aug_pfset), should the need be deemed large enough.
Regards,
Jonas
--
Jonas Eriksson
Consultant at AS/EAB/FLJ/IL
Combitech AB
Älvsjö, Sweden
14 years, 9 months
[PATCH 1/1] schema: add delay attribute to bridge element
by David Lutterkort
This makes it possible to set the forward delay for a bridge
---
data/xml/initscripts-get.xsl | 3 +++
data/xml/initscripts-put.xsl | 3 +++
data/xml/interface.rng | 6 +++++-
tests/initscripts/bridge.xml | 1 +
tests/interface/bridge.xml | 2 +-
5 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/data/xml/initscripts-get.xsl b/data/xml/initscripts-get.xsl
index 8f7aca7..b2a9020 100644
--- a/data/xml/initscripts-get.xsl
+++ b/data/xml/initscripts-get.xsl
@@ -70,6 +70,9 @@
<xsl:if test="bridge/@stp">
<node label="STP" value="{bridge/@stp}"/>
</xsl:if>
+ <xsl:if test="bridge/@delay">
+ <node label="DELAY" value="{bridge/@delay}"/>
+ </xsl:if>
</tree>
<xsl:for-each select='bridge/interface'>
<tree>
diff --git a/data/xml/initscripts-put.xsl b/data/xml/initscripts-put.xsl
index aac047a..8df15c7 100644
--- a/data/xml/initscripts-put.xsl
+++ b/data/xml/initscripts-put.xsl
@@ -76,6 +76,9 @@
<xsl:if test="node[@label = 'STP']">
<xsl:attribute name="stp"><xsl:value-of select="node[@label = 'STP']/@value"/></xsl:attribute>
</xsl:if>
+ <xsl:if test="node[@label = 'DELAY']">
+ <xsl:attribute name="delay"><xsl:value-of select="node[@label = 'DELAY']/@value"/></xsl:attribute>
+ </xsl:if>
<xsl:for-each select="/descendant-or-self::*[node[@label = 'BRIDGE' and @value = $iface]]">
<xsl:if test="count(node[@label = 'VLAN']) = 0">
<xsl:call-template name="bare-ethernet-interface"/>
diff --git a/data/xml/interface.rng b/data/xml/interface.rng
index 93ec9f8..6388d44 100644
--- a/data/xml/interface.rng
+++ b/data/xml/interface.rng
@@ -7,7 +7,7 @@
is released. The current version is indicated with the v:serial
attribute on the start element.
-->
- <start v:serial="1">
+ <start v:serial="2">
<choice>
<ref name="ethernet-interface"/>
<ref name="bridge-interface"/>
@@ -115,6 +115,10 @@
<ref name="on-or-off"/>
</attribute>
</optional>
+ <!-- Bridge forward delay (see 'brctl setfd') -->
+ <optional v:since="2">
+ <attribute name="delay"><ref name="uint"/></attribute>
+ </optional>
<oneOrMore>
<choice>
<ref name="bare-ethernet-interface"/>
diff --git a/tests/initscripts/bridge.xml b/tests/initscripts/bridge.xml
index 3bfd573..5734315 100644
--- a/tests/initscripts/bridge.xml
+++ b/tests/initscripts/bridge.xml
@@ -10,6 +10,7 @@
<node label="TYPE" value="Bridge"/>
<node label="BOOTPROTO" value="dhcp"/>
<node label="STP" value="off"/>
+ <node label="DELAY" value="0"/>
</tree>
<tree path="/files/etc/sysconfig/network-scripts/ifcfg-eth0">
<node label="DEVICE" value="eth0"/>
diff --git a/tests/interface/bridge.xml b/tests/interface/bridge.xml
index fb0c35b..c04e24a 100644
--- a/tests/interface/bridge.xml
+++ b/tests/interface/bridge.xml
@@ -4,7 +4,7 @@
<protocol family="ipv4">
<dhcp/>
</protocol>
- <bridge stp="off">
+ <bridge stp="off" delay="0">
<interface type="ethernet" name="eth0">
<mac address="ab:bb:cc:dd:ee:ff"/>
</interface>
--
1.6.0.6
14 years, 9 months
ANNOUNCE: netcf 0.1.0
by David Lutterkort
I am pleased to announce the release of netcf 0.1.0.
With this release, both the API and XML schema for describing interfaces
are stable, and from now on API and schema changes will only be done in
a backwards-compatible manner.
Since the last preliminary release, 0.0.2, many things have changed, in
particular the API and XML schema; lots of very valuable feedback was
received, most notably from Jonas Eriksson, Laine Stump, Daniel
Veillard, and Dan Berrange.
On a functional level, the following was added (see the NEWS file for a
detailed list):
* MII or ARP monitoring of bonds
* VLAN interfaces as toplevel interfaces and as slaves in a bridge
* addresses on toplevel interfaces are optional
* cleanly bring bridges up/down
Tarball: https://fedorahosted.org/released/netcf/netcf-0.1.0.tar.gz
GPG signature[1]: https://fedorahosted.org/released/netcf/netcf-0.1.0.tar.gz.sig
RPM's for Fedora and EPEL are making their way through the build systems
and will be available shortly from the usual repos.
David
[1] To verify the signature, first download both the .sig file and the
corresponding tarball. Then, run a command like this:
gpg --verify netcf-0.1.0.tar.gz.sig
If that command fails because you don't have the required public key,
then run this command to import it:
gpg --keyserver keys.gnupg.net --recv-keys FC6E8A22
and rerun the `gpg --verify' command.
14 years, 9 months
[PATCH 1/1] API: ncf_close now returns an int
by David Lutterkort
Since struct netcf is ref counted, users can call it before they release
all references to it - that needs to be flagged as an error.
* src/netcf.h (ncf_close): changed prototype
* src/netcf.c: new error code NETCF_EINUSE, check refcount in ncf_close
---
src/netcf.c | 10 ++++++++--
src/netcf.h | 15 +++++++++++++--
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/netcf.c b/src/netcf.c
index b1a5c4e..f2c545d 100644
--- a/src/netcf.c
+++ b/src/netcf.c
@@ -50,7 +50,8 @@ static const char *const errmsgs[] = {
"XML parser failed", /* EXMLPARSER */
"XML invalid", /* EXMLINVALID */
"required entry missing", /* ENOENT */
- "failed to execute external program" /* EEXEC */
+ "failed to execute external program", /* EEXEC */
+ "instance still in use" /* EINUSE */
};
static void free_netcf(struct netcf *ncf) {
@@ -91,11 +92,16 @@ int ncf_init(struct netcf **ncf, const char *root) {
return -2;
}
-void ncf_close(struct netcf *ncf) {
+int ncf_close(struct netcf *ncf) {
API_ENTRY(ncf);
+ ERR_COND_BAIL(ncf->ref > 1, ncf, EINUSE);
+
drv_close(ncf);
unref(ncf, netcf);
+ return 0;
+ error:
+ return -1;
}
/* Number of known interfaces and list of them.
diff --git a/src/netcf.h b/src/netcf.h
index e9978fd..786aac5 100644
--- a/src/netcf.h
+++ b/src/netcf.h
@@ -46,7 +46,10 @@ typedef enum {
NETCF_EXMLPARSER, /* XML parser choked */
NETCF_EXMLINVALID, /* XML invalid in some form */
NETCF_ENOENT, /* Required entry in a tree is missing */
- NETCF_EEXEC /* external program execution failed or returned non-0 */
+ NETCF_EEXEC, /* external program execution failed or returned
+ * non-0 */
+ NETCF_EINUSE /* attempt to close a netcf instance that is still
+ * used by other data structures */
} netcf_errcode_t;
@@ -72,7 +75,15 @@ typedef enum {
*/
int ncf_init(struct netcf **netcf, const char *root);
-void ncf_close(struct netcf *);
+/* Close the connection to netcf and release any resources associated with
+ * it. It is an error to call this function before all data structeres
+ * retrieved using this netcf instance have been free'd; in particular, any
+ * struct netcf_if retrieved with this netcf instance must be cleaned up
+ * with NCF_IF_FREE before calling this function.
+ *
+ * Returns 0 on success, and -1 on error.
+ */
+int ncf_close(struct netcf *);
/* Number of known interfaces and list of them. For listing, interfaces are
* identified by their name.
--
1.6.0.6
14 years, 9 months