On 03/27/2015 06:01 PM, Eric Blake wrote:
On 03/26/2015 12:42 PM, Laine Stump wrote:
> netcf has always supported multiple IPv6 addresses, but only a single
> IPv4 address. This patch remedies that, at least for the redhat driver
> (also used for Fedora and CentOS).
>
> I can't claim that the xsl transforms in redhat-(get|put).xsl are the
> optimum code, but they do get the job done (no mean feat, since this
> is the first time I've ever made anything beyond a cosmetic change to
> an xsl script).
And I'm no xsl transform expert either, so take my review with a grain
of salt.
> +++ b/data/xml/redhat-get.xsl
> @@ -192,10 +192,18 @@
> </xsl:when>
> <xsl:when test="ip">
> <node label="BOOTPROTO" value="none"/>
> - <node label="IPADDR" value="{ip/@address}"/>
> - <xsl:if test="ip/@prefix">
> - <node label="NETMASK"
value="{ipcalc:netmask(ip/@prefix)}"/>
> - </xsl:if>
> + <xsl:for-each select="ip">
> + <xsl:if test="position() = 1">
> + <xsl:call-template name="ipv4-address">
> + <xsl:with-param name="index"/>
> + </xsl:call-template>
> + </xsl:if>
> + <xsl:if test="position() > 1 and position() <
100">
I think you have an off-by-one:
1 vs. 2-99...
Ah, right. All of this indexing starting at 0 instead of 1 is really a
pain for an old C-head like me.
Note that the xsl:if just above that for the case of position() = 1 is
there so that the first address will be IPADDR rather than IPADDR0 -
it's essentially calling ipv4-address with index = "" (that's another
thing I could only learn by trial and error - you can't directly set a
param to "", you have to just not set it at all). Those from 2 - 100
should be called with index = 1 - 99 , but as you've pointed out, the
last time I'll do it is for position() == 99, or index = 99 - 1 = 98.
Thanks for catching that!
> +++ b/data/xml/redhat-put.xsl
> + <xsl:for-each select="node[substring(@label, 1, 6) =
'IPADDR']">
> + <xsl:variable name="index" select="substring(@label,
7, 3)"/>
> + <xsl:if test="number($index) > 0 and number($index)
< 100">
compared to 0 vs. 1-99.
That is, if position() is 1-based, you want <= 100, or < 101, as your
terminating condition (operating on IPADDR[position()-1] for each
iteration of the for-each).
If you fix or explain the difference, then the rest of the patch looks
okay for an ACK.
Okay. I'll change the index before pushing.
Endless thanks for all the reviews.