The things I have tried worked. I have a few problems, though:
' and " are not handeled. As bridges SuSE have their own ifcfg-file, the BRIDGE_PORT contain the interfaces used in that bridge, separated by whitespaces. My problem is mostly whether to extend shellvars.aug to handle ' and ", or if this should be handled in C/XSLT.
In SLE10, interfaces could be named after their mac address using eth-id-<mac>. This is now defunct and handled by udev-rules, but I have somewhat retained handling of this, but convert it to ethX-syntax. However, when facing an interface-element in netcf-xml with a mac address, I convert this to a eth-id-interface in XSLT, and then to ethX in C by looking up the mac. Instead of doing this, the driver should retain the original ethX-name as an fallback in case the mac is not found.
Whenever using define in netcf, i get: error: unspecified error error: aug_save failed After doing some dry runs with augtool, I have concluded that this is most probably due to augeas trying to save the address-files in /sys. Is this a problem when using the initscript-driver as well?
Currently, there is a mix between functions that accept name or path. I am planning to clean up this mess, preferably standardizing on name as argument.
I also have a few questions/opinions:
Should we include some general debug macros? I stole the ones from libvirt and removed them before this post, and i had alot of use for them, for example to see the <forest>-xml when doing a define.
Alot of code are the same between the initscript driver, and probably will be for other drivers as well. I recommend that this code is broken out, for example to a Linux-helper? The same goes for both C-code (e.g. handling of mac-address lookups) and XSLT (e.g. bonding-options).
I changed the augeas save-behaviour to "backup" in the augeas init function (which probably also could be lifted out to a general helper file). I left it in there, since I like it alot, but we should probably discuss it further.
Regards, Jonas
-- Jonas Eriksson Consultant at AS/EAB/FLJ/IL Combitech AB Älvsjö, Sweden
Added autoconf to configure.ac Makefile.am and src/Makefile.am to detect what driver to use.
-- Jonas Eriksson Consultant at AS/EAB/FLJ/IL Combitech AB Älvsjö, Sweden
On Fri, 2009-06-26 at 10:20 +0200, Jonas Eriksson wrote:
Added autoconf to configure.ac Makefile.am and src/Makefile.am to detect what driver to use.
This is ok; though it needs to be the last patch in the series, since it mentions files that don't exist yet.
David
These files are heavily based on their initscript-counterpart. However, posting a diff would be mostly messy. I therefore post the whole files.
Mailman stopped the mail because it was too large, so i am resending the 2nd part of the patch splitted in two.
-- Jonas Eriksson Consultant at AS/EAB/FLJ/IL Combitech AB Älvsjö, Sweden
On Fri, 2009-06-26 at 12:44 +0200, Jonas Eriksson wrote:
These files are heavily based on their initscript-counterpart. However, posting a diff would be mostly messy. I therefore post the whole files.
Having the diff makes it easier to apply to a git tree (plus, you get to write your own commit message ;).
Mailman stopped the mail because it was too large, so i am resending the 2nd part of the patch splitted in two.
Strange ... did you try sending it with git-send-email, i.e. as the body of the email rather than in attachments ?
Assuming you are doing your work on a local 'dev/suse' topic branch off of master, do the folllowing to send your patch series:
rm -rf /tmp/suse-patches && mkdir /tmp/suse-patches git checkout master git pull git checkout dev/suse git rebase master git format-patch -o /tmp/suse-patches -n master git send-email --compose --subject 'SuSE driver' --thread /tmp/suse-patches
As for drv_suse.c itself, my main comment is that as much of the code that is common between drv_initscripts.c and drv_suse.c should be factored out into one or more utility files. Now that we (well, you, really ;) know the general direction of where things need to go for SuSE support, it's probably best to start with those refactoring patches.
David
These files are heavily based on their initscript-counterpart. However, posting a diff would be mostly messy. I therefore post the whole files.
Mailman stopped the mail because it was too large, so i am resending the 2nd part of the patch splitted in two.
-- Jonas Eriksson Consultant at AS/EAB/FLJ/IL Combitech AB Älvsjö, Sweden
On Fri, 2009-06-26 at 12:45 +0200, Jonas Eriksson wrote:
These files are heavily based on their initscript-counterpart.
They look ok overall (of course, I am relying on you that they actually work on SuSE ;)
Besides the refactoring discussed for the C driver, one minor thing that I simplified the other day is that instead of writing
<node label="GATEWAY"> <xsl:attribute name="value"><xsl:value-of select="route/@gateway"/></xsl:attribute> </node>
you can use an 'attribute value template' and write
<node label="GATEWAY" value="{route/@gateway}"/>
which is much more readable (if such a thing can be said of XSLT)
Thinking about it some more, I am actually not sure whether refactoring is all that useful for the stylesheets, since they have most of the backend-specific logic. Of course, where it's easy and seems safe for future changes (like constructing BONDING_OPTS), it makes sense; in other places, it might be ok to have the same things repeated across several stylesheets.
David
Hi Jonas,
On Fri, 2009-06-26 at 10:16 +0200, Jonas Eriksson wrote:
The things I have tried worked. I have a few problems, though:
' and " are not handeled. As bridges SuSE have their own ifcfg-file, the BRIDGE_PORT contain the interfaces used in that bridge, separated by whitespaces. My problem is mostly whether to extend shellvars.aug to handle ' and ", or if this should be handled in C/XSLT.
I currently have a similar problem with BONDING_OPTS in the initscripts driver; I worked around that with an XSLT extension function bond:option - though I am thinking that changing the Shellvars lens to handle that would be much cleaner. It would require shipping a custom shellvars.aug with netcf, but that's not a big issue.
In SLE10, interfaces could be named after their mac address using eth-id-<mac>. This is now defunct and handled by udev-rules, but I have somewhat retained handling of this, but convert it to ethX-syntax.
Are you sure that this is worth it ? According to [1], openSuSE 10.3 will be EOL'd at the end of October, and the eth-id-<mac> form seems to lead to quite a bit of internal headaches. At the very least, it's probably better to write the driver for OpenSuSE 11.x first, and add support for OpenSuSE 10 later.
However, when facing an interface-element in netcf-xml with a mac address, I convert this to a eth-id-interface in XSLT, and then to ethX in C by looking up the mac. Instead of doing this, the driver should retain the original ethX-name as an fallback in case the mac is not found.
So the eth-id-<mac> files contain no indication what the device name should be ? You could also address the above headache with an XSLT extension function - that all assumes of course that the MAC uniquely identifies one device.
Whenever using define in netcf, i get: error: unspecified error error: aug_save failed After doing some dry runs with augtool, I have concluded that this is most probably due to augeas trying to save the address-files in /sys. Is this a problem when using the initscript-driver as well?
No, not at all; if your driver doesn't modify entries under /files/sys/, Augeas won't ever try to write them. If you have an augtool script that reproduces the problem, do a 'print /augeas//error' after the faild save.
Currently, there is a mix between functions that accept name or path. I am planning to clean up this mess, preferably standardizing on name as argument.
I also have a few questions/opinions:
Should we include some general debug macros? I stole the ones from libvirt and removed them before this post, and i had alot of use for them, for example to see the <forest>-xml when doing a define.
Yeah, that might be generally useful. For the specific issue of debugging the XSL transforms, I added a little helper program in git (ncftransform) that lets you run either direction, e.g.
netcf>NETCF_DATADIR=$PWD/data/ ./src/ncftransform put tests/initscripts/bridge.xml netcf>NETCF_DATADIR=$PWD/data/ ./src/ncftransform get tests/interface/bridge.xml
We also need to run the transformation tests in tests/test-initscripts.c for the SuSE driver, which means we need files for tests/suse/
Alot of code are the same between the initscript driver, and probably will be for other drivers as well. I recommend that this code is broken out, for example to a Linux-helper? The same goes for both C-code (e.g. handling of mac-address lookups) and XSLT (e.g. bonding-options).
Yes, that's my biggest comment about your patches: we need to reduce the amount of duplicated code, since that will just become an enormous maintenance headache. The code needs to be broken up in a major way. For review purposes it's best if you structure that into lots of small patches.
I changed the augeas save-behaviour to "backup" in the augeas init function (which probably also could be lifted out to a general helper file).
Yes, get_augeas needs to be factored out; it needs to get the augeas_xfm array as an additional input parameter, but other than that, there should be no difference between the two drivers.
I left it in there, since I like it alot, but we should probably discuss it further.
For debugging/testing it's definitely helpful, though for production use, I would feel very uncomfortable littering the filesystem with lots of .augsave files. This would be a perfect use for a debug macro.
David
netcf-devel@lists.fedorahosted.org