[libvirt] [PATCHv2 0/7] Sharing code for domain and network routes
Hi guys, Here are a few patches to have common route definitions for domains and networks. What has changed since v1: * Split into several patches for backportability as suggested by Laine * Moved the virNetworkRouteDef struct definition in the c file to make sure all routes are created with the virNetworkRouteDefCreate (or Parse) function. This way we can get rid of the redundant checks in the format function. * Moved the prefix computing code into a new virNetworkRouteDefGetPrefix function. * Applied and tweaked John's patches. * Fix the virSocketAddrGetIpPrefix 0.0.0.0 special case as suggested by Laine. Cédric Bosdonnat (5): Fix ipv6 regex in RNG schemas to match '::' Move network route definition to networkcommon.rng Move code related to network routes to networkcommon_conf.[ch] Use the network route definitions for domains virSocketAddrGetIpPrefix 0.0.0.0 special case John Ferlan (2): domain_conf: Resolve Coverity RESOURCE_LEAK domain_conf: Check errors from virSocketAddrFormat docs/formatdomain.html.in | 9 +- docs/schemas/basictypes.rng | 2 +- docs/schemas/domaincommon.rng | 29 +- docs/schemas/network.rng | 20 +- docs/schemas/networkcommon.rng | 22 ++ po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/domain_conf.c | 136 ++----- src/conf/domain_conf.h | 14 +- src/conf/network_conf.c | 297 +--------------- src/conf/network_conf.h | 22 +- src/conf/networkcommon_conf.c | 414 ++++++++++++++++++++++ src/conf/networkcommon_conf.h | 72 ++++ src/libvirt_private.syms | 11 + src/lxc/lxc_container.c | 22 +- src/lxc/lxc_native.c | 20 +- src/network/bridge_driver.c | 44 +-- src/util/virsocketaddr.c | 6 + tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml | 4 +- tests/lxcconf2xmldata/lxcconf2xml-simple.xml | 4 +- tests/lxcxml2xmldata/lxc-hostdev.xml | 4 +- tests/lxcxml2xmldata/lxc-idmap.xml | 4 +- 22 files changed, 633 insertions(+), 527 deletions(-) create mode 100644 src/conf/networkcommon_conf.c create mode 100644 src/conf/networkcommon_conf.h -- 2.1.2
--- docs/schemas/basictypes.rng | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 9ddd92b..efc9da4 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -174,7 +174,7 @@ <define name="ipv6Addr"> <data type="string"> <!-- To understand this better, take apart the toplevel "|"s --> -<param name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param> +<param name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)|(::)</param> </data> </define> -- 2.1.2
On 01/15/2015 04:25 AM, Cédric Bosdonnat wrote:
--- docs/schemas/basictypes.rng | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 9ddd92b..efc9da4 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -174,7 +174,7 @@ <define name="ipv6Addr"> <data type="string"> <!-- To understand this better, take apart the toplevel "|"s --> -<param name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param> +<param name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)|(::)</param> </data> </define>
ACK
Moving network route to the network common schema will allow reusing it. --- docs/schemas/network.rng | 20 +------------------- docs/schemas/networkcommon.rng | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 9a7d156..63d81c1 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -371,25 +371,7 @@ </zeroOrMore> <!-- <route> element --> <zeroOrMore> - <!-- The (static) route element specifies a network address and gateway - address to access that network. Both the network address and - the gateway address must be specified. --> - <element name="route"> - <optional> - <attribute name="family"><ref name="addr-family"/></attribute> - </optional> - <attribute name="address"><ref name="ipAddr"/></attribute> - <optional> - <choice> - <attribute name="netmask"><ref name="ipv4Addr"/></attribute> - <attribute name="prefix"><ref name="ipPrefix"/></attribute> - </choice> - </optional> - <attribute name="gateway"><ref name="ipAddr"/></attribute> - <optional> - <attribute name="metric"><ref name="unsignedInt"/></attribute> - </optional> - </element> + <ref name="routex"/> </zeroOrMore> </interleave> </element> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index e26b7f3..cbcae91 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -224,4 +224,26 @@ <param name='maxInclusive'>65535</param> </data> </define> + + <!-- The (static) route element specifies a network address and gateway + address to access that network. Both the network address and + the gateway address must be specified. --> + <define name='routex'> + <element name="route"> + <optional> + <attribute name="family"><ref name="addr-family"/></attribute> + </optional> + <attribute name="address"><ref name="ipAddr"/></attribute> + <optional> + <choice> + <attribute name="netmask"><ref name="ipv4Addr"/></attribute> + <attribute name="prefix"><ref name="ipPrefix"/></attribute> + </choice> + </optional> + <attribute name="gateway"><ref name="ipAddr"/></attribute> + <optional> + <attribute name="metric"><ref name="unsignedInt"/></attribute> + </optional> + </element> + </define> </grammar> -- 2.1.2
On 15.01.2015 10:25, Cédric Bosdonnat wrote:
Moving network route to the network common schema will allow reusing it. --- docs/schemas/network.rng | 20 +------------------- docs/schemas/networkcommon.rng | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 9a7d156..63d81c1 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -371,25 +371,7 @@ </zeroOrMore> <!-- <route> element --> <zeroOrMore> - <!-- The (static) route element specifies a network address and gateway - address to access that network. Both the network address and - the gateway address must be specified. --> - <element name="route"> - <optional> - <attribute name="family"><ref name="addr-family"/></attribute> - </optional> - <attribute name="address"><ref name="ipAddr"/></attribute> - <optional> - <choice> - <attribute name="netmask"><ref name="ipv4Addr"/></attribute> - <attribute name="prefix"><ref name="ipPrefix"/></attribute> - </choice> - </optional> - <attribute name="gateway"><ref name="ipAddr"/></attribute> - <optional> - <attribute name="metric"><ref name="unsignedInt"/></attribute> - </optional> - </element> + <ref name="routex"/> </zeroOrMore> </interleave> </element> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index e26b7f3..cbcae91 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -224,4 +224,26 @@ <param name='maxInclusive'>65535</param> </data> </define> + + <!-- The (static) route element specifies a network address and gateway + address to access that network. Both the network address and + the gateway address must be specified. --> + <define name='routex'>
routex? That's an odd name. However, looking into the future at 4/7 I can see why yo need to do it this way.
+ <element name="route"> + <optional> + <attribute name="family"><ref name="addr-family"/></attribute> + </optional> + <attribute name="address"><ref name="ipAddr"/></attribute> + <optional> + <choice> + <attribute name="netmask"><ref name="ipv4Addr"/></attribute> + <attribute name="prefix"><ref name="ipPrefix"/></attribute> + </choice> + </optional> + <attribute name="gateway"><ref name="ipAddr"/></attribute> + <optional> + <attribute name="metric"><ref name="unsignedInt"/></attribute> + </optional> + </element> + </define> </grammar>
Michal
On Thu, 2015-01-15 at 11:58 +0100, Michal Privoznik wrote:
On 15.01.2015 10:25, Cédric Bosdonnat wrote:
Moving network route to the network common schema will allow reusing it. --- docs/schemas/network.rng | 20 +------------------- docs/schemas/networkcommon.rng | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 9a7d156..63d81c1 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -371,25 +371,7 @@ </zeroOrMore> <!-- <route> element --> <zeroOrMore> - <!-- The (static) route element specifies a network address and gateway - address to access that network. Both the network address and - the gateway address must be specified. --> - <element name="route"> - <optional> - <attribute name="family"><ref name="addr-family"/></attribute> - </optional> - <attribute name="address"><ref name="ipAddr"/></attribute> - <optional> - <choice> - <attribute name="netmask"><ref name="ipv4Addr"/></attribute> - <attribute name="prefix"><ref name="ipPrefix"/></attribute> - </choice> - </optional> - <attribute name="gateway"><ref name="ipAddr"/></attribute> - <optional> - <attribute name="metric"><ref name="unsignedInt"/></attribute> - </optional> - </element> + <ref name="routex"/> </zeroOrMore> </interleave> </element> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index e26b7f3..cbcae91 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -224,4 +224,26 @@ <param name='maxInclusive'>65535</param> </data> </define> + + <!-- The (static) route element specifies a network address and gateway + address to access that network. Both the network address and + the gateway address must be specified. --> + <define name='routex'>
routex? That's an odd name. However, looking into the future at 4/7 I can see why yo need to do it this way.
Such a name was proposed by Laine to avoid the definition conflict when moving to networkcommon.rng. May be a better name could still be appreciated, even for a temporary renaming. -- Cedric
+ <element name="route"> + <optional> + <attribute name="family"><ref name="addr-family"/></attribute> + </optional> + <attribute name="address"><ref name="ipAddr"/></attribute> + <optional> + <choice> + <attribute name="netmask"><ref name="ipv4Addr"/></attribute> + <attribute name="prefix"><ref name="ipPrefix"/></attribute> + </choice> + </optional> + <attribute name="gateway"><ref name="ipAddr"/></attribute> + <optional> + <attribute name="metric"><ref name="unsignedInt"/></attribute> + </optional> + </element> + </define> </grammar>
Michal
On 15.01.2015 13:11, Cedric Bosdonnat wrote:
On Thu, 2015-01-15 at 11:58 +0100, Michal Privoznik wrote:
On 15.01.2015 10:25, Cédric Bosdonnat wrote:
Moving network route to the network common schema will allow reusing it. --- docs/schemas/network.rng | 20 +------------------- docs/schemas/networkcommon.rng | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 9a7d156..63d81c1 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -371,25 +371,7 @@ </zeroOrMore> <!-- <route> element --> <zeroOrMore> - <!-- The (static) route element specifies a network address and gateway - address to access that network. Both the network address and - the gateway address must be specified. --> - <element name="route"> - <optional> - <attribute name="family"><ref name="addr-family"/></attribute> - </optional> - <attribute name="address"><ref name="ipAddr"/></attribute> - <optional> - <choice> - <attribute name="netmask"><ref name="ipv4Addr"/></attribute> - <attribute name="prefix"><ref name="ipPrefix"/></attribute> - </choice> - </optional> - <attribute name="gateway"><ref name="ipAddr"/></attribute> - <optional> - <attribute name="metric"><ref name="unsignedInt"/></attribute> - </optional> - </element> + <ref name="routex"/> </zeroOrMore> </interleave> </element> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index e26b7f3..cbcae91 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -224,4 +224,26 @@ <param name='maxInclusive'>65535</param> </data> </define> + + <!-- The (static) route element specifies a network address and gateway + address to access that network. Both the network address and + the gateway address must be specified. --> + <define name='routex'>
routex? That's an odd name. However, looking into the future at 4/7 I can see why yo need to do it this way.
Such a name was proposed by Laine to avoid the definition conflict when moving to networkcommon.rng. May be a better name could still be appreciated, even for a temporary renaming.
No need for that. It's just that I was curious when I first saw it. Michal
On 01/15/2015 07:11 AM, Cedric Bosdonnat wrote:
On Thu, 2015-01-15 at 11:58 +0100, Michal Privoznik wrote:
On 15.01.2015 10:25, Cédric Bosdonnat wrote:
Moving network route to the network common schema will allow reusing it. --- docs/schemas/network.rng | 20 +------------------- docs/schemas/networkcommon.rng | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 9a7d156..63d81c1 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -371,25 +371,7 @@ </zeroOrMore> <!-- <route> element --> <zeroOrMore> - <!-- The (static) route element specifies a network address and gateway - address to access that network. Both the network address and - the gateway address must be specified. --> - <element name="route"> - <optional> - <attribute name="family"><ref name="addr-family"/></attribute> - </optional> - <attribute name="address"><ref name="ipAddr"/></attribute> - <optional> - <choice> - <attribute name="netmask"><ref name="ipv4Addr"/></attribute> - <attribute name="prefix"><ref name="ipPrefix"/></attribute> - </choice> - </optional> - <attribute name="gateway"><ref name="ipAddr"/></attribute> - <optional> - <attribute name="metric"><ref name="unsignedInt"/></attribute> - </optional> - </element> + <ref name="routex"/> </zeroOrMore> </interleave> </element> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index e26b7f3..cbcae91 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -224,4 +224,26 @@ <param name='maxInclusive'>65535</param> </data> </define> + + <!-- The (static) route element specifies a network address and gateway + address to access that network. Both the network address and + the gateway address must be specified. --> + <define name='routex'> routex? That's an odd name. However, looking into the future at 4/7 I can see why yo need to do it this way. Such a name was proposed by Laine to avoid the definition conflict when moving to networkcommon.rng. May be a better name could still be appreciated, even for a temporary renaming.
I *think* what I wanted to suggest was a tiny patch beforehand to rename the "route" in domaincommon.rng into "routex", then making this patch with the name "route" - that way this patch could be backported by itself to an older stable branch if necessary, without the followup patch that renames to "route". I'm really starting to... what's the term? "bikeshed"? No, that's not it... "Be a pain in the %&@&#"? yeah, that's the one... though, so if that's the only issue, I wouldn't worry about a respin :-) ACK either way.
Moving code for parsing and formatting network routes to networkcommon_conf helps reusing those routes for domains. The route definition has been hidden to help reducing the number of unnecessary checks in the format function. --- po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/network_conf.c | 297 ++---------------------------- src/conf/network_conf.h | 22 +-- src/conf/networkcommon_conf.c | 414 ++++++++++++++++++++++++++++++++++++++++++ src/conf/networkcommon_conf.h | 72 ++++++++ src/libvirt_private.syms | 11 ++ src/network/bridge_driver.c | 44 ++--- 8 files changed, 526 insertions(+), 338 deletions(-) create mode 100644 src/conf/networkcommon_conf.c create mode 100644 src/conf/networkcommon_conf.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 094c8e3..3064037 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -25,6 +25,7 @@ src/conf/netdev_bandwidth_conf.c src/conf/netdev_vlan_conf.c src/conf/netdev_vport_profile_conf.c src/conf/network_conf.c +src/conf/networkcommon_conf.c src/conf/node_device_conf.c src/conf/numatune_conf.c src/conf/nwfilter_conf.c diff --git a/src/Makefile.am b/src/Makefile.am index 216abac..4bba536 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -287,7 +287,8 @@ NETWORK_EVENT_SOURCES = \ # Network driver generic impl APIs NETWORK_CONF_SOURCES = \ - conf/network_conf.c conf/network_conf.h + conf/network_conf.c conf/network_conf.h \ + conf/networkcommon_conf.c conf/networkcommon_conf.h # Network filter driver generic impl APIs NWFILTER_PARAM_CONF_SOURCES = \ diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 26fe18d..66f9b6c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -163,12 +163,6 @@ virNetworkIpDefClear(virNetworkIpDefPtr def) } static void -virNetworkRouteDefClear(virNetworkRouteDefPtr def) -{ - VIR_FREE(def->family); -} - -static void virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def) { VIR_FREE(def->name); @@ -251,7 +245,7 @@ virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def->ips); for (i = 0; i < def->nroutes && def->routes; i++) - virNetworkRouteDefClear(&def->routes[i]); + virNetworkRouteDefFree(def->routes[i]); VIR_FREE(def->routes); for (i = 0; i < def->nPortGroups && def->portGroups; i++) @@ -1371,232 +1365,6 @@ virNetworkIPDefParseXML(const char *networkName, } static int -virNetworkRouteDefParseXML(const char *networkName, - xmlNodePtr node, - xmlXPathContextPtr ctxt, - virNetworkRouteDefPtr def) -{ - /* - * virNetworkRouteDef object is already allocated as part - * of an array. On failure clear: it out, but don't free it. - */ - - xmlNodePtr save; - char *address = NULL, *netmask = NULL; - char *gateway = NULL; - unsigned long prefix = 0, metric = 0; - int result = -1; - int prefixRc, metricRc; - virSocketAddr testAddr; - - save = ctxt->node; - ctxt->node = node; - - /* grab raw data from XML */ - def->family = virXPathString("string(./@family)", ctxt); - address = virXPathString("string(./@address)", ctxt); - netmask = virXPathString("string(./@netmask)", ctxt); - gateway = virXPathString("string(./@gateway)", ctxt); - prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix); - if (prefixRc == -2) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix specified " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - def->has_prefix = (prefixRc == 0); - def->prefix = prefix; - metricRc = virXPathULong("string(./@metric)", ctxt, &metric); - if (metricRc == -2) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid metric specified " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - if (metricRc == 0) { - def->has_metric = true; - if (metric == 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid metric value, must be > 0 " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - } - def->metric = metric; - - /* Note: both network and gateway addresses must be specified */ - - if (!address) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required address attribute " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - - if (!gateway) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required gateway attribute " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - - if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad network address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - - if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad gateway address '%s' " - "in route definition of network '%s'"), - gateway, networkName); - goto cleanup; - } - - /* validate network address, etc. for each family */ - if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) { - if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) || - VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) { - virReportError(VIR_ERR_XML_ERROR, - def->family == NULL ? - _("No family specified for non-IPv4 address '%s' " - "in route definition of network '%s'") : - _("IPv4 family specified for non-IPv4 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) { - virReportError(VIR_ERR_XML_ERROR, - def->family == NULL ? - _("No family specified for non-IPv4 gateway '%s' " - "in route definition of network '%s'") : - _("IPv4 family specified for non-IPv4 gateway '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (netmask) { - if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad netmask address '%s' " - "in route definition of network '%s'"), - netmask, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) { - virReportError(VIR_ERR_XML_ERROR, - _("Network '%s' has invalid netmask '%s' " - "for address '%s' (both must be IPv4)"), - networkName, netmask, address); - goto cleanup; - } - if (def->has_prefix) { - /* can't have both netmask and prefix at the same time */ - virReportError(VIR_ERR_XML_ERROR, - _("Route definition '%s' cannot have both " - "a prefix and a netmask"), - networkName); - goto cleanup; - } - } - if (def->prefix > 32) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix %u specified " - "in route definition of network '%s', " - "must be 0 - 32"), - def->prefix, networkName); - goto cleanup; - } - } else if (STREQ(def->family, "ipv6")) { - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { - virReportError(VIR_ERR_XML_ERROR, - _("ipv6 family specified for non-IPv6 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (netmask) { - virReportError(VIR_ERR_XML_ERROR, - _("Specifying netmask invalid for IPv6 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) { - virReportError(VIR_ERR_XML_ERROR, - _("ipv6 specified for non-IPv6 gateway address '%s' " - "in route definition of network '%s'"), - gateway, networkName); - goto cleanup; - } - if (def->prefix > 128) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix %u specified " - "in route definition of network '%s', " - "must be 0 - 128"), - def->prefix, networkName); - goto cleanup; - } - } else { - virReportError(VIR_ERR_XML_ERROR, - _("Unrecognized family '%s' " - "in route definition of network'%s'"), - def->family, networkName); - goto cleanup; - } - - /* make sure the address is a network address */ - if (netmask) { - if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("error converting address '%s' with netmask '%s' " - "to network-address " - "in route definition of network '%s'"), - address, netmask, networkName); - goto cleanup; - } - } else { - if (virSocketAddrMaskByPrefix(&def->address, - def->prefix, &testAddr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("error converting address '%s' with prefix %u " - "to network-address " - "in route definition of network '%s'"), - address, def->prefix, networkName); - goto cleanup; - } - } - if (!virSocketAddrEqual(&def->address, &testAddr)) { - virReportError(VIR_ERR_XML_ERROR, - _("address '%s' in route definition of network '%s' " - "is not a network address"), - address, networkName); - goto cleanup; - } - - result = 0; - - cleanup: - if (result < 0) - virNetworkRouteDefClear(def); - VIR_FREE(address); - VIR_FREE(netmask); - VIR_FREE(gateway); - - ctxt->node = save; - return result; -} - -static int virNetworkPortGroupParseXML(virPortGroupDefPtr def, xmlNodePtr node, xmlXPathContextPtr ctxt) @@ -2209,11 +1977,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; /* parse each definition */ for (i = 0; i < nRoutes; i++) { - if (virNetworkRouteDefParseXML(def->name, + virNetworkRouteDefPtr route = NULL; + + if (!(route = virNetworkRouteDefParseXML(def->name, routeNodes[i], - ctxt, - &def->routes[i]) < 0) + ctxt))) goto error; + def->routes[i] = route; def->nroutes++; } @@ -2229,17 +1999,18 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) size_t j; virSocketAddr testAddr, testGw; bool addrMatch; - virNetworkRouteDefPtr gwdef = &def->routes[i]; + virNetworkRouteDefPtr gwdef = def->routes[i]; + virSocketAddrPtr gateway = virNetworkRouteDefGetGateway(gwdef); addrMatch = false; for (j = 0; j < nIps; j++) { virNetworkIpDefPtr def2 = &def->ips[j]; - if (VIR_SOCKET_ADDR_FAMILY(&gwdef->gateway) + if (VIR_SOCKET_ADDR_FAMILY(gateway) != VIR_SOCKET_ADDR_FAMILY(&def2->address)) { continue; } int prefix = virNetworkIpDefPrefix(def2); virSocketAddrMaskByPrefix(&def2->address, prefix, &testAddr); - virSocketAddrMaskByPrefix(&gwdef->gateway, prefix, &testGw); + virSocketAddrMaskByPrefix(gateway, prefix, &testGw); if (VIR_SOCKET_ADDR_VALID(&testAddr) && VIR_SOCKET_ADDR_VALID(&testGw) && virSocketAddrEqual(&testAddr, &testGw)) { @@ -2248,7 +2019,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) } } if (!addrMatch) { - char *gw = virSocketAddrFormat(&gwdef->gateway); + char *gw = virSocketAddrFormat(gateway); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unreachable static route gateway '%s' specified for network '%s'"), gw, def->name); @@ -2585,50 +2356,6 @@ virNetworkIpDefFormat(virBufferPtr buf, } static int -virNetworkRouteDefFormat(virBufferPtr buf, - const virNetworkRouteDef *def) -{ - int result = -1; - - virBufferAddLit(buf, "<route"); - - if (def->family) - virBufferAsprintf(buf, " family='%s'", def->family); - if (VIR_SOCKET_ADDR_VALID(&def->address)) { - char *addr = virSocketAddrFormat(&def->address); - - if (!addr) - goto error; - virBufferAsprintf(buf, " address='%s'", addr); - VIR_FREE(addr); - } - if (VIR_SOCKET_ADDR_VALID(&def->netmask)) { - char *addr = virSocketAddrFormat(&def->netmask); - - if (!addr) - goto error; - virBufferAsprintf(buf, " netmask='%s'", addr); - VIR_FREE(addr); - } - if (def->has_prefix) - virBufferAsprintf(buf, " prefix='%u'", def->prefix); - if (VIR_SOCKET_ADDR_VALID(&def->gateway)) { - char *addr = virSocketAddrFormat(&def->gateway); - if (!addr) - goto error; - virBufferAsprintf(buf, " gateway='%s'", addr); - VIR_FREE(addr); - } - if (def->has_metric && def->metric > 0) - virBufferAsprintf(buf, " metric='%u'", def->metric); - virBufferAddLit(buf, "/>\n"); - - result = 0; - error: - return result; -} - -static int virPortGroupDefFormat(virBufferPtr buf, const virPortGroupDef *def) { @@ -2850,7 +2577,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, } for (i = 0; i < def->nroutes; i++) { - if (virNetworkRouteDefFormat(buf, &def->routes[i]) < 0) + if (virNetworkRouteDefFormat(buf, def->routes[i]) < 0) goto error; } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 8110028..b113e14 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -39,6 +39,7 @@ # include "virmacaddr.h" # include "device_conf.h" # include "virbitmap.h" +# include "networkcommon_conf.h" typedef enum { VIR_NETWORK_FORWARD_NONE = 0, @@ -162,25 +163,6 @@ struct _virNetworkIpDef { virSocketAddr bootserver; }; -typedef struct _virNetworkRouteDef virNetworkRouteDef; -typedef virNetworkRouteDef *virNetworkRouteDefPtr; -struct _virNetworkRouteDef { - char *family; /* ipv4 or ipv6 - default is ipv4 */ - virSocketAddr address; /* Routed Network IP address */ - - /* One or the other of the following two will be used for a given - * Network address, but never both. The parser guarantees this. - * The virSocketAddrGetIpPrefix() can be used to get a - * valid prefix. - */ - virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */ - unsigned int prefix; /* ipv6 - only prefix allowed */ - bool has_prefix; /* prefix= was specified */ - unsigned int metric; /* value for metric (defaults to 1) */ - bool has_metric; /* metric= was specified */ - virSocketAddr gateway; /* gateway IP address for ip-route */ - }; - typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr; struct _virNetworkForwardIfDef { @@ -259,7 +241,7 @@ struct _virNetworkDef { virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this network */ size_t nroutes; - virNetworkRouteDefPtr routes; /* ptr to array of static routes on this interface */ + virNetworkRouteDefPtr *routes; /* ptr to array of static routes on this interface */ virNetworkDNSDef dns; /* dns related configuration */ virNetDevVPortProfilePtr virtPortProfile; diff --git a/src/conf/networkcommon_conf.c b/src/conf/networkcommon_conf.c new file mode 100644 index 0000000..1545367 --- /dev/null +++ b/src/conf/networkcommon_conf.c @@ -0,0 +1,414 @@ +/* + * networkcommon_conf.c: network XML handling + * + * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2008 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include "virerror.h" +#include "datatypes.h" +#include "networkcommon_conf.h" +#include "viralloc.h" +#include "virxml.h" + +#define VIR_FROM_THIS VIR_FROM_NETWORK + +struct _virNetworkRouteDef { + char *family; /* ipv4 or ipv6 - default is ipv4 */ + virSocketAddr address; /* Routed Network IP address */ + + /* One or the other of the following two will be used for a given + * Network address, but never both. The parser guarantees this. + * The virSocketAddrGetIpPrefix() can be used to get a + * valid prefix. + */ + virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */ + unsigned int prefix; /* ipv6 - only prefix allowed */ + bool has_prefix; /* prefix= was specified */ + unsigned int metric; /* value for metric (defaults to 1) */ + bool has_metric; /* metric= was specified */ + virSocketAddr gateway; /* gateway IP address for ip-route */ +}; + +void +virNetworkRouteDefFree(virNetworkRouteDefPtr def) +{ + VIR_FREE(def->family); + VIR_FREE(def); +} + +virNetworkRouteDefPtr +virNetworkRouteDefCreate(const char *errorDetail, + char *family, + char *address, + char *netmask, + char *gateway, + unsigned int prefix, + bool hasPrefix, + unsigned int metric, + bool hasMetric) +{ + virNetworkRouteDefPtr def = NULL; + virSocketAddr testAddr; + + if (VIR_ALLOC(def) < 0) + return NULL; + + def->family = family; + def->prefix = prefix; + def->has_prefix = hasPrefix; + def->metric = metric; + def->has_metric = hasMetric; + + /* Note: both network and gateway addresses must be specified */ + + if (!address) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Missing required address attribute " + "in route definition"), + errorDetail); + goto error; + } + + if (!gateway) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Missing required gateway attribute " + "in route definition"), + errorDetail); + goto error; + } + + if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Bad network address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + + if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Bad gateway address '%s' " + "in route definition"), + errorDetail, gateway); + goto error; + } + + /* validate network address, etc. for each family */ + if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) { + if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) || + VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) { + virReportError(VIR_ERR_XML_ERROR, + def->family == NULL ? + _("%s: No family specified for non-IPv4 address '%s' " + "in route definition") : + _("%s: IPv4 family specified for non-IPv4 address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) { + virReportError(VIR_ERR_XML_ERROR, + def->family == NULL ? + _("%s: No family specified for non-IPv4 gateway '%s' " + "in route definition") : + _("%s: IPv4 family specified for non-IPv4 gateway '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (netmask) { + if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Bad netmask address '%s' " + "in route definition"), + errorDetail, netmask); + goto error; + } + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid netmask '%s' " + "for address '%s' (both must be IPv4)"), + errorDetail, netmask, address); + goto error; + } + if (def->has_prefix) { + /* can't have both netmask and prefix at the same time */ + virReportError(VIR_ERR_XML_ERROR, + _("%s: Route definition cannot have both " + "a prefix and a netmask"), + errorDetail); + goto error; + } + } + if (def->prefix > 32) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid prefix %u specified " + "in route definition, " + "must be 0 - 32"), + errorDetail, def->prefix); + goto error; + } + } else if (STREQ(def->family, "ipv6")) { + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: ipv6 family specified for non-IPv6 address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (netmask) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Specifying netmask invalid for IPv6 address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: ipv6 specified for non-IPv6 gateway address '%s' " + "in route definition"), + errorDetail, gateway); + goto error; + } + if (def->prefix > 128) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid prefix %u specified " + "in route definition, " + "must be 0 - 128"), + errorDetail, def->prefix); + goto error; + } + } else { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Unrecognized family '%s' " + "in route definition"), + errorDetail, def->family); + goto error; + } + + /* make sure the address is a network address */ + if (netmask) { + if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: Error converting address '%s' with netmask '%s' " + "to network-address " + "in route definition"), + errorDetail, address, netmask); + goto error; + } + } else { + if (virSocketAddrMaskByPrefix(&def->address, + def->prefix, &testAddr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: Error converting address '%s' with prefix %u " + "to network-address " + "in route definition"), + errorDetail, address, def->prefix); + goto error; + } + } + if (!virSocketAddrEqual(&def->address, &testAddr)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Address '%s' in route definition " + "is not a network address"), + errorDetail, address); + goto error; + } + + return def; + + error: + virNetworkRouteDefFree(def); + return NULL; +} + +virNetworkRouteDefPtr +virNetworkRouteDefParseXML(const char *errorDetail, + xmlNodePtr node, + xmlXPathContextPtr ctxt) +{ + /* + * virNetworkRouteDef object is already allocated as part + * of an array. On failure clear: it out, but don't free it. + */ + + virNetworkRouteDefPtr def = NULL; + xmlNodePtr save; + char *family = NULL; + char *address = NULL, *netmask = NULL; + char *gateway = NULL; + unsigned long prefix = 0, metric = 0; + int prefixRc, metricRc; + bool hasPrefix = false; + bool hasMetric = false; + + save = ctxt->node; + ctxt->node = node; + + /* grab raw data from XML */ + family = virXPathString("string(./@family)", ctxt); + address = virXPathString("string(./@address)", ctxt); + netmask = virXPathString("string(./@netmask)", ctxt); + gateway = virXPathString("string(./@gateway)", ctxt); + prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix); + if (prefixRc == -2) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid prefix specified " + "in route definition"), + errorDetail); + goto cleanup; + } + hasPrefix = (prefixRc == 0); + metricRc = virXPathULong("string(./@metric)", ctxt, &metric); + if (metricRc == -2) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid metric specified " + "in route definition"), + errorDetail); + goto cleanup; + } + if (metricRc == 0) { + hasMetric = true; + if (metric == 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid metric value, must be > 0 " + "in route definition"), + errorDetail); + goto cleanup; + } + } + + def = virNetworkRouteDefCreate(errorDetail, family, address, netmask, + gateway, prefix, hasPrefix, metric, + hasMetric); + + cleanup: + ctxt->node = save; + VIR_FREE(address); + VIR_FREE(netmask); + VIR_FREE(gateway); + return def; +} + +int +virNetworkRouteDefFormat(virBufferPtr buf, + const virNetworkRouteDef *def) +{ + int result = -1; + char *addr = NULL; + + virBufferAddLit(buf, "<route"); + + if (def->family) + virBufferAsprintf(buf, " family='%s'", def->family); + + addr = virSocketAddrFormat(&def->address); + + if (!addr) + goto error; + virBufferAsprintf(buf, " address='%s'", addr); + VIR_FREE(addr); + + if (VIR_SOCKET_ADDR_VALID(&def->netmask)) { + addr = virSocketAddrFormat(&def->netmask); + + if (!addr) + goto error; + virBufferAsprintf(buf, " netmask='%s'", addr); + VIR_FREE(addr); + } + if (def->has_prefix) + virBufferAsprintf(buf, " prefix='%u'", def->prefix); + + addr = virSocketAddrFormat(&def->gateway); + if (!addr) + goto error; + virBufferAsprintf(buf, " gateway='%s'", addr); + VIR_FREE(addr); + + if (def->has_metric && def->metric > 0) + virBufferAsprintf(buf, " metric='%u'", def->metric); + virBufferAddLit(buf, "/>\n"); + + result = 0; + error: + return result; +} + +virSocketAddrPtr +virNetworkRouteDefGetAddress(virNetworkRouteDefPtr def) +{ + if (def) + return &def->address; + + return NULL; +} + +int +virNetworkRouteDefGetPrefix(virNetworkRouteDefPtr def) +{ + int prefix = 0; + virSocketAddr zero; + + if (!def) + return -1; + + /* this creates an all-0 address of the appropriate family */ + ignore_value(virSocketAddrParse(&zero, + (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) + ? VIR_SOCKET_ADDR_IPV4_ALL + : VIR_SOCKET_ADDR_IPV6_ALL), + VIR_SOCKET_ADDR_FAMILY(&def->address))); + + if (virSocketAddrEqual(&def->address, &zero)) { + if (def->has_prefix && def->prefix == 0) + prefix = 0; + else if ((VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET) && + virSocketAddrEqual(&def->netmask, &zero))) + prefix = 0; + else + prefix = virSocketAddrGetIpPrefix(&def->address, &def->netmask, + def->prefix); + } else { + prefix = virSocketAddrGetIpPrefix(&def->address, &def->netmask, + def->prefix); + } + + return prefix; +} + +unsigned int +virNetworkRouteDefGetMetric(virNetworkRouteDefPtr def) +{ + if (def && def->has_metric && def->metric > 0) + return def->metric; + + return 1; +} + +virSocketAddrPtr +virNetworkRouteDefGetGateway(virNetworkRouteDefPtr def) +{ + if (def) + return &def->gateway; + return NULL; +} diff --git a/src/conf/networkcommon_conf.h b/src/conf/networkcommon_conf.h new file mode 100644 index 0000000..4d2b1d2 --- /dev/null +++ b/src/conf/networkcommon_conf.h @@ -0,0 +1,72 @@ +/* + * networkcommon_conf.h: network XML handling + * + * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#ifndef __NETWORKCOMMON_CONF_H__ +# define __NETWORKCOMMON_CONF_H__ + +# include <libxml/tree.h> +# include <libxml/xpath.h> + +# include "internal.h" +# include "virbuffer.h" +# include "virsocketaddr.h" + +typedef struct _virNetworkRouteDef virNetworkRouteDef; +typedef virNetworkRouteDef *virNetworkRouteDefPtr; + +void +virNetworkRouteDefFree(virNetworkRouteDefPtr def); + +virNetworkRouteDefPtr +virNetworkRouteDefCreate(const char *networkName, + char *family, + char *address, + char *netmask, + char *gateway, + unsigned int prefix, + bool hasPrefix, + unsigned int metric, + bool hasMetric); + +virNetworkRouteDefPtr +virNetworkRouteDefParseXML(const char *networkName, + xmlNodePtr node, + xmlXPathContextPtr ctxt); +int +virNetworkRouteDefFormat(virBufferPtr buf, + const virNetworkRouteDef *def); + +virSocketAddrPtr +virNetworkRouteDefGetAddress(virNetworkRouteDefPtr def); + +int +virNetworkRouteDefGetPrefix(virNetworkRouteDefPtr def); + +unsigned int +virNetworkRouteDefGetMetric(virNetworkRouteDefPtr def); + +virSocketAddrPtr +virNetworkRouteDefGetGateway(virNetworkRouteDefPtr def); + +#endif /* __NETWORKCOMMON_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7ceb54d..a35f077 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -582,6 +582,17 @@ virNetworkEventLifecycleNew; virNetworkEventStateRegisterID; +# conf/networkcommon_conf.h +virNetworkRouteDefCreate; +virNetworkRouteDefFormat; +virNetworkRouteDefFree; +virNetworkRouteDefGetAddress; +virNetworkRouteDefGetGateway; +virNetworkRouteDefGetMetric; +virNetworkRouteDefGetPrefix; +virNetworkRouteDefParseXML; + + # conf/node_device_conf.h virNodeDevCapsDefFree; virNodeDevCapTypeFromString; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fca60f1..7b84e27 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1934,29 +1934,10 @@ static int networkAddRouteToBridge(virNetworkObjPtr network, virNetworkRouteDefPtr routedef) { - int prefix = 0; - unsigned int metric; - virSocketAddrPtr addr = &routedef->address; - virSocketAddrPtr mask = &routedef->netmask; - virSocketAddr zero; - - /* this creates an all-0 address of the appropriate family */ - ignore_value(virSocketAddrParse(&zero, - (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET) - ? "0.0.0.0" : "::"), - VIR_SOCKET_ADDR_FAMILY(addr))); - - if (virSocketAddrEqual(addr, &zero)) { - if (routedef->has_prefix && routedef->prefix == 0) - prefix = 0; - else if ((VIR_SOCKET_ADDR_IS_FAMILY(mask, AF_INET) && - virSocketAddrEqual(mask, &zero))) - prefix = 0; - else - prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); - } else { - prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); - } + int prefix = virNetworkRouteDefGetPrefix(routedef); + unsigned int metric = virNetworkRouteDefGetMetric(routedef); + virSocketAddrPtr addr = virNetworkRouteDefGetAddress(routedef); + virSocketAddrPtr gateway = virNetworkRouteDefGetGateway(routedef); if (prefix < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1966,13 +1947,8 @@ networkAddRouteToBridge(virNetworkObjPtr network, return -1; } - if (routedef->has_metric && routedef->metric > 0) - metric = routedef->metric; - else - metric = 1; - - if (virNetDevAddRoute(network->def->bridge, &routedef->address, - prefix, &routedef->gateway, metric) < 0) { + if (virNetDevAddRoute(network->def->bridge, addr, + prefix, gateway, metric) < 0) { return -1; } return 0; @@ -2063,11 +2039,15 @@ networkStartNetworkVirtual(virNetworkObjPtr network) goto err2; for (i = 0; i < network->def->nroutes; i++) { - routedef = &network->def->routes[i]; + virSocketAddrPtr gateway = NULL; + + routedef = network->def->routes[i]; + gateway = virNetworkRouteDefGetGateway(routedef); + /* Add the IP route to the bridge */ /* ignore errors, error msg will be generated */ /* but libvirt will not know and net-destroy will work. */ - if (VIR_SOCKET_ADDR_VALID(&routedef->gateway)) { + if (VIR_SOCKET_ADDR_VALID(gateway)) { if (networkAddRouteToBridge(network, routedef) < 0) { /* an error occurred adding the static route */ continue; /* for now, do nothing */ -- 2.1.2
On 15.01.2015 10:25, Cédric Bosdonnat wrote:
Moving code for parsing and formatting network routes to networkcommon_conf helps reusing those routes for domains. The route definition has been hidden to help reducing the number of unnecessary checks in the format function. --- po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/network_conf.c | 297 ++---------------------------- src/conf/network_conf.h | 22 +-- src/conf/networkcommon_conf.c | 414 ++++++++++++++++++++++++++++++++++++++++++ src/conf/networkcommon_conf.h | 72 ++++++++ src/libvirt_private.syms | 11 ++ src/network/bridge_driver.c | 44 ++--- 8 files changed, 526 insertions(+), 338 deletions(-) create mode 100644 src/conf/networkcommon_conf.c create mode 100644 src/conf/networkcommon_conf.h
diff --git a/po/POTFILES.in b/po/POTFILES.in index 094c8e3..3064037 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -25,6 +25,7 @@ src/conf/netdev_bandwidth_conf.c src/conf/netdev_vlan_conf.c src/conf/netdev_vport_profile_conf.c src/conf/network_conf.c +src/conf/networkcommon_conf.c src/conf/node_device_conf.c src/conf/numatune_conf.c src/conf/nwfilter_conf.c diff --git a/src/Makefile.am b/src/Makefile.am index 216abac..4bba536 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -287,7 +287,8 @@ NETWORK_EVENT_SOURCES = \
# Network driver generic impl APIs NETWORK_CONF_SOURCES = \ - conf/network_conf.c conf/network_conf.h + conf/network_conf.c conf/network_conf.h \ + conf/networkcommon_conf.c conf/networkcommon_conf.h
# Network filter driver generic impl APIs NWFILTER_PARAM_CONF_SOURCES = \ diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 26fe18d..66f9b6c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -163,12 +163,6 @@ virNetworkIpDefClear(virNetworkIpDefPtr def) }
static void -virNetworkRouteDefClear(virNetworkRouteDefPtr def) -{ - VIR_FREE(def->family); -} - -static void virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def) { VIR_FREE(def->name); @@ -251,7 +245,7 @@ virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def->ips);
for (i = 0; i < def->nroutes && def->routes; i++) - virNetworkRouteDefClear(&def->routes[i]); + virNetworkRouteDefFree(def->routes[i]); VIR_FREE(def->routes);
for (i = 0; i < def->nPortGroups && def->portGroups; i++) @@ -1371,232 +1365,6 @@ virNetworkIPDefParseXML(const char *networkName, }
static int -virNetworkRouteDefParseXML(const char *networkName, - xmlNodePtr node, - xmlXPathContextPtr ctxt, - virNetworkRouteDefPtr def) -{ - /* - * virNetworkRouteDef object is already allocated as part - * of an array. On failure clear: it out, but don't free it. - */ - - xmlNodePtr save; - char *address = NULL, *netmask = NULL; - char *gateway = NULL; - unsigned long prefix = 0, metric = 0; - int result = -1; - int prefixRc, metricRc; - virSocketAddr testAddr; - - save = ctxt->node; - ctxt->node = node; - - /* grab raw data from XML */ - def->family = virXPathString("string(./@family)", ctxt); - address = virXPathString("string(./@address)", ctxt); - netmask = virXPathString("string(./@netmask)", ctxt); - gateway = virXPathString("string(./@gateway)", ctxt); - prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix); - if (prefixRc == -2) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix specified " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - def->has_prefix = (prefixRc == 0); - def->prefix = prefix; - metricRc = virXPathULong("string(./@metric)", ctxt, &metric); - if (metricRc == -2) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid metric specified " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - if (metricRc == 0) { - def->has_metric = true; - if (metric == 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid metric value, must be > 0 " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - } - def->metric = metric; - - /* Note: both network and gateway addresses must be specified */ - - if (!address) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required address attribute " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - - if (!gateway) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required gateway attribute " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - - if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad network address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - - if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad gateway address '%s' " - "in route definition of network '%s'"), - gateway, networkName); - goto cleanup; - } - - /* validate network address, etc. for each family */ - if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) { - if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) || - VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) { - virReportError(VIR_ERR_XML_ERROR, - def->family == NULL ? - _("No family specified for non-IPv4 address '%s' " - "in route definition of network '%s'") : - _("IPv4 family specified for non-IPv4 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) { - virReportError(VIR_ERR_XML_ERROR, - def->family == NULL ? - _("No family specified for non-IPv4 gateway '%s' " - "in route definition of network '%s'") : - _("IPv4 family specified for non-IPv4 gateway '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (netmask) { - if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad netmask address '%s' " - "in route definition of network '%s'"), - netmask, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) { - virReportError(VIR_ERR_XML_ERROR, - _("Network '%s' has invalid netmask '%s' " - "for address '%s' (both must be IPv4)"), - networkName, netmask, address); - goto cleanup; - } - if (def->has_prefix) { - /* can't have both netmask and prefix at the same time */ - virReportError(VIR_ERR_XML_ERROR, - _("Route definition '%s' cannot have both " - "a prefix and a netmask"), - networkName); - goto cleanup; - } - } - if (def->prefix > 32) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix %u specified " - "in route definition of network '%s', " - "must be 0 - 32"), - def->prefix, networkName); - goto cleanup; - } - } else if (STREQ(def->family, "ipv6")) { - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { - virReportError(VIR_ERR_XML_ERROR, - _("ipv6 family specified for non-IPv6 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (netmask) { - virReportError(VIR_ERR_XML_ERROR, - _("Specifying netmask invalid for IPv6 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) { - virReportError(VIR_ERR_XML_ERROR, - _("ipv6 specified for non-IPv6 gateway address '%s' " - "in route definition of network '%s'"), - gateway, networkName); - goto cleanup; - } - if (def->prefix > 128) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix %u specified " - "in route definition of network '%s', " - "must be 0 - 128"), - def->prefix, networkName); - goto cleanup; - } - } else { - virReportError(VIR_ERR_XML_ERROR, - _("Unrecognized family '%s' " - "in route definition of network'%s'"), - def->family, networkName); - goto cleanup; - } - - /* make sure the address is a network address */ - if (netmask) { - if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("error converting address '%s' with netmask '%s' " - "to network-address " - "in route definition of network '%s'"), - address, netmask, networkName); - goto cleanup; - } - } else { - if (virSocketAddrMaskByPrefix(&def->address, - def->prefix, &testAddr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("error converting address '%s' with prefix %u " - "to network-address " - "in route definition of network '%s'"), - address, def->prefix, networkName); - goto cleanup; - } - } - if (!virSocketAddrEqual(&def->address, &testAddr)) { - virReportError(VIR_ERR_XML_ERROR, - _("address '%s' in route definition of network '%s' " - "is not a network address"), - address, networkName); - goto cleanup; - } - - result = 0; - - cleanup: - if (result < 0) - virNetworkRouteDefClear(def); - VIR_FREE(address); - VIR_FREE(netmask); - VIR_FREE(gateway); - - ctxt->node = save; - return result; -} - -static int virNetworkPortGroupParseXML(virPortGroupDefPtr def, xmlNodePtr node, xmlXPathContextPtr ctxt) @@ -2209,11 +1977,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; /* parse each definition */ for (i = 0; i < nRoutes; i++) { - if (virNetworkRouteDefParseXML(def->name, + virNetworkRouteDefPtr route = NULL; + + if (!(route = virNetworkRouteDefParseXML(def->name, routeNodes[i], - ctxt, - &def->routes[i]) < 0) + ctxt)))
Indentation's off.
goto error; + def->routes[i] = route; def->nroutes++; }
Michal
On Thu, 2015-01-15 at 11:58 +0100, Michal Privoznik wrote:
On 15.01.2015 10:25, Cédric Bosdonnat wrote:
Moving code for parsing and formatting network routes to networkcommon_conf helps reusing those routes for domains. The route definition has been hidden to help reducing the number of unnecessary checks in the format function. --- po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/network_conf.c | 297 ++---------------------------- src/conf/network_conf.h | 22 +-- src/conf/networkcommon_conf.c | 414 ++++++++++++++++++++++++++++++++++++++++++ src/conf/networkcommon_conf.h | 72 ++++++++ src/libvirt_private.syms | 11 ++ src/network/bridge_driver.c | 44 ++--- 8 files changed, 526 insertions(+), 338 deletions(-) create mode 100644 src/conf/networkcommon_conf.c create mode 100644 src/conf/networkcommon_conf.h
diff --git a/po/POTFILES.in b/po/POTFILES.in index 094c8e3..3064037 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -25,6 +25,7 @@ src/conf/netdev_bandwidth_conf.c src/conf/netdev_vlan_conf.c src/conf/netdev_vport_profile_conf.c src/conf/network_conf.c +src/conf/networkcommon_conf.c src/conf/node_device_conf.c src/conf/numatune_conf.c src/conf/nwfilter_conf.c diff --git a/src/Makefile.am b/src/Makefile.am index 216abac..4bba536 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -287,7 +287,8 @@ NETWORK_EVENT_SOURCES = \
# Network driver generic impl APIs NETWORK_CONF_SOURCES = \ - conf/network_conf.c conf/network_conf.h + conf/network_conf.c conf/network_conf.h \ + conf/networkcommon_conf.c conf/networkcommon_conf.h
# Network filter driver generic impl APIs NWFILTER_PARAM_CONF_SOURCES = \ diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 26fe18d..66f9b6c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -163,12 +163,6 @@ virNetworkIpDefClear(virNetworkIpDefPtr def) }
static void -virNetworkRouteDefClear(virNetworkRouteDefPtr def) -{ - VIR_FREE(def->family); -} - -static void virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def) { VIR_FREE(def->name); @@ -251,7 +245,7 @@ virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def->ips);
for (i = 0; i < def->nroutes && def->routes; i++) - virNetworkRouteDefClear(&def->routes[i]); + virNetworkRouteDefFree(def->routes[i]); VIR_FREE(def->routes);
for (i = 0; i < def->nPortGroups && def->portGroups; i++) @@ -1371,232 +1365,6 @@ virNetworkIPDefParseXML(const char *networkName, }
static int -virNetworkRouteDefParseXML(const char *networkName, - xmlNodePtr node, - xmlXPathContextPtr ctxt, - virNetworkRouteDefPtr def) -{ - /* - * virNetworkRouteDef object is already allocated as part - * of an array. On failure clear: it out, but don't free it. - */ - - xmlNodePtr save; - char *address = NULL, *netmask = NULL; - char *gateway = NULL; - unsigned long prefix = 0, metric = 0; - int result = -1; - int prefixRc, metricRc; - virSocketAddr testAddr; - - save = ctxt->node; - ctxt->node = node; - - /* grab raw data from XML */ - def->family = virXPathString("string(./@family)", ctxt); - address = virXPathString("string(./@address)", ctxt); - netmask = virXPathString("string(./@netmask)", ctxt); - gateway = virXPathString("string(./@gateway)", ctxt); - prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix); - if (prefixRc == -2) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix specified " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - def->has_prefix = (prefixRc == 0); - def->prefix = prefix; - metricRc = virXPathULong("string(./@metric)", ctxt, &metric); - if (metricRc == -2) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid metric specified " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - if (metricRc == 0) { - def->has_metric = true; - if (metric == 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid metric value, must be > 0 " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - } - def->metric = metric; - - /* Note: both network and gateway addresses must be specified */ - - if (!address) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required address attribute " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - - if (!gateway) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required gateway attribute " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - - if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad network address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - - if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad gateway address '%s' " - "in route definition of network '%s'"), - gateway, networkName); - goto cleanup; - } - - /* validate network address, etc. for each family */ - if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) { - if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) || - VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) { - virReportError(VIR_ERR_XML_ERROR, - def->family == NULL ? - _("No family specified for non-IPv4 address '%s' " - "in route definition of network '%s'") : - _("IPv4 family specified for non-IPv4 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) { - virReportError(VIR_ERR_XML_ERROR, - def->family == NULL ? - _("No family specified for non-IPv4 gateway '%s' " - "in route definition of network '%s'") : - _("IPv4 family specified for non-IPv4 gateway '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (netmask) { - if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad netmask address '%s' " - "in route definition of network '%s'"), - netmask, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) { - virReportError(VIR_ERR_XML_ERROR, - _("Network '%s' has invalid netmask '%s' " - "for address '%s' (both must be IPv4)"), - networkName, netmask, address); - goto cleanup; - } - if (def->has_prefix) { - /* can't have both netmask and prefix at the same time */ - virReportError(VIR_ERR_XML_ERROR, - _("Route definition '%s' cannot have both " - "a prefix and a netmask"), - networkName); - goto cleanup; - } - } - if (def->prefix > 32) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix %u specified " - "in route definition of network '%s', " - "must be 0 - 32"), - def->prefix, networkName); - goto cleanup; - } - } else if (STREQ(def->family, "ipv6")) { - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { - virReportError(VIR_ERR_XML_ERROR, - _("ipv6 family specified for non-IPv6 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (netmask) { - virReportError(VIR_ERR_XML_ERROR, - _("Specifying netmask invalid for IPv6 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) { - virReportError(VIR_ERR_XML_ERROR, - _("ipv6 specified for non-IPv6 gateway address '%s' " - "in route definition of network '%s'"), - gateway, networkName); - goto cleanup; - } - if (def->prefix > 128) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix %u specified " - "in route definition of network '%s', " - "must be 0 - 128"), - def->prefix, networkName); - goto cleanup; - } - } else { - virReportError(VIR_ERR_XML_ERROR, - _("Unrecognized family '%s' " - "in route definition of network'%s'"), - def->family, networkName); - goto cleanup; - } - - /* make sure the address is a network address */ - if (netmask) { - if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("error converting address '%s' with netmask '%s' " - "to network-address " - "in route definition of network '%s'"), - address, netmask, networkName); - goto cleanup; - } - } else { - if (virSocketAddrMaskByPrefix(&def->address, - def->prefix, &testAddr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("error converting address '%s' with prefix %u " - "to network-address " - "in route definition of network '%s'"), - address, def->prefix, networkName); - goto cleanup; - } - } - if (!virSocketAddrEqual(&def->address, &testAddr)) { - virReportError(VIR_ERR_XML_ERROR, - _("address '%s' in route definition of network '%s' " - "is not a network address"), - address, networkName); - goto cleanup; - } - - result = 0; - - cleanup: - if (result < 0) - virNetworkRouteDefClear(def); - VIR_FREE(address); - VIR_FREE(netmask); - VIR_FREE(gateway); - - ctxt->node = save; - return result; -} - -static int virNetworkPortGroupParseXML(virPortGroupDefPtr def, xmlNodePtr node, xmlXPathContextPtr ctxt) @@ -2209,11 +1977,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; /* parse each definition */ for (i = 0; i < nRoutes; i++) { - if (virNetworkRouteDefParseXML(def->name, + virNetworkRouteDefPtr route = NULL; + + if (!(route = virNetworkRouteDefParseXML(def->name, routeNodes[i], - ctxt, - &def->routes[i]) < 0) + ctxt)))
Indentation's off.
Nice catch, I just fixed that on my local branch.
goto error; + def->routes[i] = route; def->nroutes++; }
Michal
(Gene - I Cc'ed you because of one question I have for you down in the bowels of the review. Just search for "Gene" and you'll get to it). On 01/15/2015 04:25 AM, Cédric Bosdonnat wrote:
Moving code for parsing and formatting network routes to networkcommon_conf helps reusing those routes for domains. The route definition has been hidden to help reducing the number of unnecessary checks in the format function. --- po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/network_conf.c | 297 ++---------------------------- src/conf/network_conf.h | 22 +-- src/conf/networkcommon_conf.c | 414 ++++++++++++++++++++++++++++++++++++++++++ src/conf/networkcommon_conf.h | 72 ++++++++ src/libvirt_private.syms | 11 ++ src/network/bridge_driver.c | 44 ++--- 8 files changed, 526 insertions(+), 338 deletions(-) create mode 100644 src/conf/networkcommon_conf.c create mode 100644 src/conf/networkcommon_conf.h
diff --git a/po/POTFILES.in b/po/POTFILES.in index 094c8e3..3064037 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -25,6 +25,7 @@ src/conf/netdev_bandwidth_conf.c src/conf/netdev_vlan_conf.c src/conf/netdev_vport_profile_conf.c src/conf/network_conf.c +src/conf/networkcommon_conf.c src/conf/node_device_conf.c src/conf/numatune_conf.c src/conf/nwfilter_conf.c diff --git a/src/Makefile.am b/src/Makefile.am index 216abac..4bba536 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -287,7 +287,8 @@ NETWORK_EVENT_SOURCES = \
# Network driver generic impl APIs NETWORK_CONF_SOURCES = \ - conf/network_conf.c conf/network_conf.h + conf/network_conf.c conf/network_conf.h \ + conf/networkcommon_conf.c conf/networkcommon_conf.h
# Network filter driver generic impl APIs NWFILTER_PARAM_CONF_SOURCES = \ diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 26fe18d..66f9b6c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -163,12 +163,6 @@ virNetworkIpDefClear(virNetworkIpDefPtr def) }
static void -virNetworkRouteDefClear(virNetworkRouteDefPtr def) -{ - VIR_FREE(def->family); -} - -static void virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def) { VIR_FREE(def->name); @@ -251,7 +245,7 @@ virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def->ips);
for (i = 0; i < def->nroutes && def->routes; i++) - virNetworkRouteDefClear(&def->routes[i]); + virNetworkRouteDefFree(def->routes[i]); VIR_FREE(def->routes);
for (i = 0; i < def->nPortGroups && def->portGroups; i++) @@ -1371,232 +1365,6 @@ virNetworkIPDefParseXML(const char *networkName, }
static int -virNetworkRouteDefParseXML(const char *networkName, - xmlNodePtr node, - xmlXPathContextPtr ctxt, - virNetworkRouteDefPtr def) -{ - /* - * virNetworkRouteDef object is already allocated as part - * of an array. On failure clear: it out, but don't free it. - */ - - xmlNodePtr save; - char *address = NULL, *netmask = NULL; - char *gateway = NULL; - unsigned long prefix = 0, metric = 0; - int result = -1; - int prefixRc, metricRc; - virSocketAddr testAddr; - - save = ctxt->node; - ctxt->node = node; - - /* grab raw data from XML */ - def->family = virXPathString("string(./@family)", ctxt); - address = virXPathString("string(./@address)", ctxt); - netmask = virXPathString("string(./@netmask)", ctxt); - gateway = virXPathString("string(./@gateway)", ctxt); - prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix); - if (prefixRc == -2) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix specified " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - def->has_prefix = (prefixRc == 0); - def->prefix = prefix; - metricRc = virXPathULong("string(./@metric)", ctxt, &metric); - if (metricRc == -2) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid metric specified " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - if (metricRc == 0) { - def->has_metric = true; - if (metric == 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid metric value, must be > 0 " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - } - def->metric = metric; - - /* Note: both network and gateway addresses must be specified */ - - if (!address) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required address attribute " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - - if (!gateway) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required gateway attribute " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - - if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad network address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - - if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad gateway address '%s' " - "in route definition of network '%s'"), - gateway, networkName); - goto cleanup; - } - - /* validate network address, etc. for each family */ - if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) { - if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) || - VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) { - virReportError(VIR_ERR_XML_ERROR, - def->family == NULL ? - _("No family specified for non-IPv4 address '%s' " - "in route definition of network '%s'") : - _("IPv4 family specified for non-IPv4 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) { - virReportError(VIR_ERR_XML_ERROR, - def->family == NULL ? - _("No family specified for non-IPv4 gateway '%s' " - "in route definition of network '%s'") : - _("IPv4 family specified for non-IPv4 gateway '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (netmask) { - if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad netmask address '%s' " - "in route definition of network '%s'"), - netmask, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) { - virReportError(VIR_ERR_XML_ERROR, - _("Network '%s' has invalid netmask '%s' " - "for address '%s' (both must be IPv4)"), - networkName, netmask, address); - goto cleanup; - } - if (def->has_prefix) { - /* can't have both netmask and prefix at the same time */ - virReportError(VIR_ERR_XML_ERROR, - _("Route definition '%s' cannot have both " - "a prefix and a netmask"), - networkName); - goto cleanup; - } - } - if (def->prefix > 32) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix %u specified " - "in route definition of network '%s', " - "must be 0 - 32"), - def->prefix, networkName); - goto cleanup; - } - } else if (STREQ(def->family, "ipv6")) { - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { - virReportError(VIR_ERR_XML_ERROR, - _("ipv6 family specified for non-IPv6 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (netmask) { - virReportError(VIR_ERR_XML_ERROR, - _("Specifying netmask invalid for IPv6 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) { - virReportError(VIR_ERR_XML_ERROR, - _("ipv6 specified for non-IPv6 gateway address '%s' " - "in route definition of network '%s'"), - gateway, networkName); - goto cleanup; - } - if (def->prefix > 128) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix %u specified " - "in route definition of network '%s', " - "must be 0 - 128"), - def->prefix, networkName); - goto cleanup; - } - } else { - virReportError(VIR_ERR_XML_ERROR, - _("Unrecognized family '%s' " - "in route definition of network'%s'"), - def->family, networkName); - goto cleanup; - } - - /* make sure the address is a network address */ - if (netmask) { - if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("error converting address '%s' with netmask '%s' " - "to network-address " - "in route definition of network '%s'"), - address, netmask, networkName); - goto cleanup; - } - } else { - if (virSocketAddrMaskByPrefix(&def->address, - def->prefix, &testAddr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("error converting address '%s' with prefix %u " - "to network-address " - "in route definition of network '%s'"), - address, def->prefix, networkName); - goto cleanup; - } - } - if (!virSocketAddrEqual(&def->address, &testAddr)) { - virReportError(VIR_ERR_XML_ERROR, - _("address '%s' in route definition of network '%s' " - "is not a network address"), - address, networkName); - goto cleanup; - } - - result = 0; - - cleanup: - if (result < 0) - virNetworkRouteDefClear(def); - VIR_FREE(address); - VIR_FREE(netmask); - VIR_FREE(gateway); - - ctxt->node = save; - return result; -} - -static int virNetworkPortGroupParseXML(virPortGroupDefPtr def, xmlNodePtr node, xmlXPathContextPtr ctxt) @@ -2209,11 +1977,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; /* parse each definition */ for (i = 0; i < nRoutes; i++) { - if (virNetworkRouteDefParseXML(def->name, + virNetworkRouteDefPtr route = NULL; + + if (!(route = virNetworkRouteDefParseXML(def->name, routeNodes[i], - ctxt, - &def->routes[i]) < 0) + ctxt))) goto error; + def->routes[i] = route; def->nroutes++; }
@@ -2229,17 +1999,18 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) size_t j; virSocketAddr testAddr, testGw; bool addrMatch; - virNetworkRouteDefPtr gwdef = &def->routes[i]; + virNetworkRouteDefPtr gwdef = def->routes[i]; + virSocketAddrPtr gateway = virNetworkRouteDefGetGateway(gwdef); addrMatch = false; for (j = 0; j < nIps; j++) { virNetworkIpDefPtr def2 = &def->ips[j]; - if (VIR_SOCKET_ADDR_FAMILY(&gwdef->gateway) + if (VIR_SOCKET_ADDR_FAMILY(gateway) != VIR_SOCKET_ADDR_FAMILY(&def2->address)) { continue; } int prefix = virNetworkIpDefPrefix(def2); virSocketAddrMaskByPrefix(&def2->address, prefix, &testAddr); - virSocketAddrMaskByPrefix(&gwdef->gateway, prefix, &testGw); + virSocketAddrMaskByPrefix(gateway, prefix, &testGw); if (VIR_SOCKET_ADDR_VALID(&testAddr) && VIR_SOCKET_ADDR_VALID(&testGw) && virSocketAddrEqual(&testAddr, &testGw)) { @@ -2248,7 +2019,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) } } if (!addrMatch) { - char *gw = virSocketAddrFormat(&gwdef->gateway); + char *gw = virSocketAddrFormat(gateway); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unreachable static route gateway '%s' specified for network '%s'"), gw, def->name); @@ -2585,50 +2356,6 @@ virNetworkIpDefFormat(virBufferPtr buf, }
static int -virNetworkRouteDefFormat(virBufferPtr buf, - const virNetworkRouteDef *def) -{ - int result = -1; - - virBufferAddLit(buf, "<route"); - - if (def->family) - virBufferAsprintf(buf, " family='%s'", def->family); - if (VIR_SOCKET_ADDR_VALID(&def->address)) { - char *addr = virSocketAddrFormat(&def->address); - - if (!addr) - goto error; - virBufferAsprintf(buf, " address='%s'", addr); - VIR_FREE(addr); - } - if (VIR_SOCKET_ADDR_VALID(&def->netmask)) { - char *addr = virSocketAddrFormat(&def->netmask); - - if (!addr) - goto error; - virBufferAsprintf(buf, " netmask='%s'", addr); - VIR_FREE(addr); - } - if (def->has_prefix) - virBufferAsprintf(buf, " prefix='%u'", def->prefix); - if (VIR_SOCKET_ADDR_VALID(&def->gateway)) { - char *addr = virSocketAddrFormat(&def->gateway); - if (!addr) - goto error; - virBufferAsprintf(buf, " gateway='%s'", addr); - VIR_FREE(addr); - } - if (def->has_metric && def->metric > 0) - virBufferAsprintf(buf, " metric='%u'", def->metric); - virBufferAddLit(buf, "/>\n"); - - result = 0; - error: - return result; -} - -static int virPortGroupDefFormat(virBufferPtr buf, const virPortGroupDef *def) { @@ -2850,7 +2577,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, }
for (i = 0; i < def->nroutes; i++) { - if (virNetworkRouteDefFormat(buf, &def->routes[i]) < 0) + if (virNetworkRouteDefFormat(buf, def->routes[i]) < 0) goto error; }
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 8110028..b113e14 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -39,6 +39,7 @@ # include "virmacaddr.h" # include "device_conf.h" # include "virbitmap.h" +# include "networkcommon_conf.h"
typedef enum { VIR_NETWORK_FORWARD_NONE = 0, @@ -162,25 +163,6 @@ struct _virNetworkIpDef { virSocketAddr bootserver; };
-typedef struct _virNetworkRouteDef virNetworkRouteDef; -typedef virNetworkRouteDef *virNetworkRouteDefPtr; -struct _virNetworkRouteDef { - char *family; /* ipv4 or ipv6 - default is ipv4 */ - virSocketAddr address; /* Routed Network IP address */ - - /* One or the other of the following two will be used for a given - * Network address, but never both. The parser guarantees this. - * The virSocketAddrGetIpPrefix() can be used to get a - * valid prefix. - */ - virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */ - unsigned int prefix; /* ipv6 - only prefix allowed */ - bool has_prefix; /* prefix= was specified */ - unsigned int metric; /* value for metric (defaults to 1) */ - bool has_metric; /* metric= was specified */ - virSocketAddr gateway; /* gateway IP address for ip-route */ - }; - typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr; struct _virNetworkForwardIfDef { @@ -259,7 +241,7 @@ struct _virNetworkDef { virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this network */
size_t nroutes; - virNetworkRouteDefPtr routes; /* ptr to array of static routes on this interface */ + virNetworkRouteDefPtr *routes; /* ptr to array of static routes on this interface */
virNetworkDNSDef dns; /* dns related configuration */ virNetDevVPortProfilePtr virtPortProfile; diff --git a/src/conf/networkcommon_conf.c b/src/conf/networkcommon_conf.c new file mode 100644 index 0000000..1545367 --- /dev/null +++ b/src/conf/networkcommon_conf.c @@ -0,0 +1,414 @@ +/* + * networkcommon_conf.c: network XML handling + * + * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2008 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include "virerror.h" +#include "datatypes.h" +#include "networkcommon_conf.h" +#include "viralloc.h" +#include "virxml.h" + +#define VIR_FROM_THIS VIR_FROM_NETWORK + +struct _virNetworkRouteDef { + char *family; /* ipv4 or ipv6 - default is ipv4 */ + virSocketAddr address; /* Routed Network IP address */ + + /* One or the other of the following two will be used for a given + * Network address, but never both. The parser guarantees this. + * The virSocketAddrGetIpPrefix() can be used to get a + * valid prefix. + */ + virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */ + unsigned int prefix; /* ipv6 - only prefix allowed */ + bool has_prefix; /* prefix= was specified */ + unsigned int metric; /* value for metric (defaults to 1) */ + bool has_metric; /* metric= was specified */ + virSocketAddr gateway; /* gateway IP address for ip-route */ +}; + +void +virNetworkRouteDefFree(virNetworkRouteDefPtr def) +{ + VIR_FREE(def->family); + VIR_FREE(def); +} + +virNetworkRouteDefPtr +virNetworkRouteDefCreate(const char *errorDetail, + char *family, + char *address, + char *netmask, + char *gateway, + unsigned int prefix, + bool hasPrefix, + unsigned int metric, + bool hasMetric) +{ + virNetworkRouteDefPtr def = NULL; + virSocketAddr testAddr; + + if (VIR_ALLOC(def) < 0) + return NULL; + + def->family = family;
moving family rather than copying it works, but it complicates things down in the caller, which then has to make some intelligent decisions about whether or not the original pointer has been moved to somewhere and is still in use, or if it should be free'd (see below). For that reason, I think family should be VIR_STRDUP'ed here and the original unconditionally VIR_FREE'd in the caller. (this wasn't an issue in the original code, because family was being parsed directly into the object)
+ def->prefix = prefix; + def->has_prefix = hasPrefix; + def->metric = metric; + def->has_metric = hasMetric; + + /* Note: both network and gateway addresses must be specified */ + + if (!address) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Missing required address attribute " + "in route definition"), + errorDetail); + goto error; + } + + if (!gateway) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Missing required gateway attribute " + "in route definition"), + errorDetail); + goto error; + } + + if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Bad network address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + + if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Bad gateway address '%s' " + "in route definition"), + errorDetail, gateway); + goto error; + } + + /* validate network address, etc. for each family */ + if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) { + if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) || + VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) { + virReportError(VIR_ERR_XML_ERROR, + def->family == NULL ? + _("%s: No family specified for non-IPv4 address '%s' " + "in route definition") : + _("%s: IPv4 family specified for non-IPv4 address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) { + virReportError(VIR_ERR_XML_ERROR, + def->family == NULL ? + _("%s: No family specified for non-IPv4 gateway '%s' " + "in route definition") : + _("%s: IPv4 family specified for non-IPv4 gateway '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (netmask) { + if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Bad netmask address '%s' " + "in route definition"), + errorDetail, netmask); + goto error; + } + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid netmask '%s' " + "for address '%s' (both must be IPv4)"), + errorDetail, netmask, address); + goto error; + } + if (def->has_prefix) { + /* can't have both netmask and prefix at the same time */ + virReportError(VIR_ERR_XML_ERROR, + _("%s: Route definition cannot have both " + "a prefix and a netmask"), + errorDetail); + goto error; + } + } + if (def->prefix > 32) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid prefix %u specified " + "in route definition, " + "must be 0 - 32"), + errorDetail, def->prefix); + goto error; + } + } else if (STREQ(def->family, "ipv6")) { + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: ipv6 family specified for non-IPv6 address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (netmask) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Specifying netmask invalid for IPv6 address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: ipv6 specified for non-IPv6 gateway address '%s' " + "in route definition"), + errorDetail, gateway); + goto error; + } + if (def->prefix > 128) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid prefix %u specified " + "in route definition, " + "must be 0 - 128"), + errorDetail, def->prefix); + goto error; + } + } else { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Unrecognized family '%s' " + "in route definition"), + errorDetail, def->family); + goto error; + } + + /* make sure the address is a network address */ + if (netmask) { + if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: Error converting address '%s' with netmask '%s' " + "to network-address " + "in route definition"), + errorDetail, address, netmask); + goto error; + } + } else { + if (virSocketAddrMaskByPrefix(&def->address, + def->prefix, &testAddr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: Error converting address '%s' with prefix %u " + "to network-address " + "in route definition"), + errorDetail, address, def->prefix); + goto error; + } + } + if (!virSocketAddrEqual(&def->address, &testAddr)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Address '%s' in route definition " + "is not a network address"), + errorDetail, address); + goto error; + } + + return def; + + error: + virNetworkRouteDefFree(def); + return NULL; +} + +virNetworkRouteDefPtr +virNetworkRouteDefParseXML(const char *errorDetail, + xmlNodePtr node, + xmlXPathContextPtr ctxt) +{ + /* + * virNetworkRouteDef object is already allocated as part + * of an array. On failure clear: it out, but don't free it. + */ + + virNetworkRouteDefPtr def = NULL; + xmlNodePtr save; + char *family = NULL; + char *address = NULL, *netmask = NULL; + char *gateway = NULL; + unsigned long prefix = 0, metric = 0; + int prefixRc, metricRc; + bool hasPrefix = false; + bool hasMetric = false; + + save = ctxt->node; + ctxt->node = node; + + /* grab raw data from XML */ + family = virXPathString("string(./@family)", ctxt); + address = virXPathString("string(./@address)", ctxt); + netmask = virXPathString("string(./@netmask)", ctxt); + gateway = virXPathString("string(./@gateway)", ctxt); + prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix); + if (prefixRc == -2) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid prefix specified " + "in route definition"), + errorDetail); + goto cleanup; + } + hasPrefix = (prefixRc == 0); + metricRc = virXPathULong("string(./@metric)", ctxt, &metric); + if (metricRc == -2) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid metric specified " + "in route definition"), + errorDetail); + goto cleanup; + } + if (metricRc == 0) { + hasMetric = true; + if (metric == 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid metric value, must be > 0 " + "in route definition"), + errorDetail); + goto cleanup; + } + } + + def = virNetworkRouteDefCreate(errorDetail, family, address, netmask, + gateway, prefix, hasPrefix, metric, + hasMetric); + + cleanup: + ctxt->node = save; + VIR_FREE(address); + VIR_FREE(netmask); + VIR_FREE(gateway);
You haven't free'd family, which will cause a leak if an error is found prior to calling virNetworkRouteDefCreate(). Of course since virNetworkRouteDefCreate() *moves* family into the object rather than copying it, if you indiscriminately free the original pointer here, you've just free'd something that's still in use in the case of non-failure. (more explanation of why I think that virNetworkRouteDefCreate() should VIR_STRDUP family instead of moving it)
+ return def; +} + +int +virNetworkRouteDefFormat(virBufferPtr buf, + const virNetworkRouteDef *def) +{ + int result = -1; + char *addr = NULL; + + virBufferAddLit(buf, "<route"); + + if (def->family) + virBufferAsprintf(buf, " family='%s'", def->family); + + addr = virSocketAddrFormat(&def->address); + + if (!addr) + goto error;
Since you need to change the handling of family anyway, while you're moving this, you could change these usages to: if (!(addr = virSocketAddrFormat(&def->address))) goto error; to save lines.
+ virBufferAsprintf(buf, " address='%s'", addr); + VIR_FREE(addr); + + if (VIR_SOCKET_ADDR_VALID(&def->netmask)) { + addr = virSocketAddrFormat(&def->netmask); + + if (!addr) + goto error;
same here
+ virBufferAsprintf(buf, " netmask='%s'", addr); + VIR_FREE(addr); + } + if (def->has_prefix) + virBufferAsprintf(buf, " prefix='%u'", def->prefix); + + addr = virSocketAddrFormat(&def->gateway); + if (!addr) + goto error;
and here. etc.
+ virBufferAsprintf(buf, " gateway='%s'", addr); + VIR_FREE(addr); + + if (def->has_metric && def->metric > 0) + virBufferAsprintf(buf, " metric='%u'", def->metric); + virBufferAddLit(buf, "/>\n"); + + result = 0; + error: + return result;
Also, while you're moving it, can you change this error label to cleanup? We didn't use to pay too much attention to this, but for awhile now we've been trying to use "error" only for codepaths that are taken exclusively in case of an error, and "cleanup" for those that are executed in all cases.
+} + +virSocketAddrPtr +virNetworkRouteDefGetAddress(virNetworkRouteDefPtr def) +{ + if (def) + return &def->address; + + return NULL; +} + +int +virNetworkRouteDefGetPrefix(virNetworkRouteDefPtr def) +{ + int prefix = 0; + virSocketAddr zero; + + if (!def) + return -1; + + /* this creates an all-0 address of the appropriate family */ + ignore_value(virSocketAddrParse(&zero, + (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) + ? VIR_SOCKET_ADDR_IPV4_ALL + : VIR_SOCKET_ADDR_IPV6_ALL), + VIR_SOCKET_ADDR_FAMILY(&def->address))); + + if (virSocketAddrEqual(&def->address, &zero)) {
+ if (def->has_prefix && def->prefix == 0) + prefix = 0;
I understand that you created this function by just moving existing code, so I hesitate to even get into the subject of refactoring it here (as a matter of fact, the more I think about it, the more I think it's better to *not* refactor as I will suggest below, in order to avoid unintentional regressions when backporting to branches. But I've already typed it, so I'll leave it in :-) I think the above chunk should be outside all of the conditionals and shouldn't check for == 0 - no matter what else is true, if the user specified a prefix, that is what should be returned. Likewise, if they specified a netmask, then the netmask should be converted to a prefix no matter what else was supplied. Only if there is no valid prefix and no valid netmask, *then* we should create a prefix based on the class/family of address. The following seems like a proper way to implement this (although again - I don't think I want such a substantial change in what is essentially a code movement patch. Instead just leave it as is, and I'll make a patch implementing my idea after you've pushed yours): 1) change virSocketAddrGetIpPrefix() to add the check for address == 0 (separately for ipv6 and ipv4) and set prefix to 0 in those cases. 2) virNetworkRouteDefGetPrefix() can be simplified to do this: * if prefix_specified is true, just return prefix (even if it's 0), otherwise return the result of virSocketAddrGetIpPrefix().
+ else if ((VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET) && + virSocketAddrEqual(&def->netmask, &zero))) + prefix = 0; + else + prefix = virSocketAddrGetIpPrefix(&def->address, &def->netmask, + def->prefix); + } else { + prefix = virSocketAddrGetIpPrefix(&def->address, &def->netmask, + def->prefix); + } + + return prefix; +} + +unsigned int +virNetworkRouteDefGetMetric(virNetworkRouteDefPtr def) +{ + if (def && def->has_metric && def->metric > 0) + return def->metric; + + return 1;
Hmm. Looking at the existing code, it does look like "1" is the default setting for metric. But when I look at "man route" I see that 0 is the default metric for IPv4 and 1 for IPv6. So why don't we allow a metric of 0? Gene?
+} + +virSocketAddrPtr +virNetworkRouteDefGetGateway(virNetworkRouteDefPtr def) +{ + if (def) + return &def->gateway; + return NULL; +} diff --git a/src/conf/networkcommon_conf.h b/src/conf/networkcommon_conf.h new file mode 100644 index 0000000..4d2b1d2 --- /dev/null +++ b/src/conf/networkcommon_conf.h @@ -0,0 +1,72 @@ +/* + * networkcommon_conf.h: network XML handling + * + * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#ifndef __NETWORKCOMMON_CONF_H__ +# define __NETWORKCOMMON_CONF_H__ + +# include <libxml/tree.h> +# include <libxml/xpath.h> + +# include "internal.h" +# include "virbuffer.h" +# include "virsocketaddr.h" + +typedef struct _virNetworkRouteDef virNetworkRouteDef; +typedef virNetworkRouteDef *virNetworkRouteDefPtr; + +void +virNetworkRouteDefFree(virNetworkRouteDefPtr def); + +virNetworkRouteDefPtr +virNetworkRouteDefCreate(const char *networkName, + char *family, + char *address, + char *netmask, + char *gateway, + unsigned int prefix, + bool hasPrefix, + unsigned int metric, + bool hasMetric); + +virNetworkRouteDefPtr +virNetworkRouteDefParseXML(const char *networkName, + xmlNodePtr node, + xmlXPathContextPtr ctxt); +int +virNetworkRouteDefFormat(virBufferPtr buf, + const virNetworkRouteDef *def); + +virSocketAddrPtr +virNetworkRouteDefGetAddress(virNetworkRouteDefPtr def); + +int +virNetworkRouteDefGetPrefix(virNetworkRouteDefPtr def); + +unsigned int +virNetworkRouteDefGetMetric(virNetworkRouteDefPtr def); + +virSocketAddrPtr +virNetworkRouteDefGetGateway(virNetworkRouteDefPtr def); + +#endif /* __NETWORKCOMMON_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7ceb54d..a35f077 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -582,6 +582,17 @@ virNetworkEventLifecycleNew; virNetworkEventStateRegisterID;
+# conf/networkcommon_conf.h +virNetworkRouteDefCreate; +virNetworkRouteDefFormat; +virNetworkRouteDefFree; +virNetworkRouteDefGetAddress; +virNetworkRouteDefGetGateway; +virNetworkRouteDefGetMetric; +virNetworkRouteDefGetPrefix; +virNetworkRouteDefParseXML; + + # conf/node_device_conf.h virNodeDevCapsDefFree; virNodeDevCapTypeFromString; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fca60f1..7b84e27 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1934,29 +1934,10 @@ static int networkAddRouteToBridge(virNetworkObjPtr network, virNetworkRouteDefPtr routedef) { - int prefix = 0; - unsigned int metric; - virSocketAddrPtr addr = &routedef->address; - virSocketAddrPtr mask = &routedef->netmask; - virSocketAddr zero; - - /* this creates an all-0 address of the appropriate family */ - ignore_value(virSocketAddrParse(&zero, - (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET) - ? "0.0.0.0" : "::"), - VIR_SOCKET_ADDR_FAMILY(addr))); - - if (virSocketAddrEqual(addr, &zero)) { - if (routedef->has_prefix && routedef->prefix == 0) - prefix = 0; - else if ((VIR_SOCKET_ADDR_IS_FAMILY(mask, AF_INET) && - virSocketAddrEqual(mask, &zero))) - prefix = 0; - else - prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); - } else { - prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); - } + int prefix = virNetworkRouteDefGetPrefix(routedef); + unsigned int metric = virNetworkRouteDefGetMetric(routedef); + virSocketAddrPtr addr = virNetworkRouteDefGetAddress(routedef); + virSocketAddrPtr gateway = virNetworkRouteDefGetGateway(routedef);
if (prefix < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1966,13 +1947,8 @@ networkAddRouteToBridge(virNetworkObjPtr network, return -1; }
- if (routedef->has_metric && routedef->metric > 0) - metric = routedef->metric; - else - metric = 1; - - if (virNetDevAddRoute(network->def->bridge, &routedef->address, - prefix, &routedef->gateway, metric) < 0) { + if (virNetDevAddRoute(network->def->bridge, addr, + prefix, gateway, metric) < 0) { return -1; } return 0; @@ -2063,11 +2039,15 @@ networkStartNetworkVirtual(virNetworkObjPtr network) goto err2;
for (i = 0; i < network->def->nroutes; i++) { - routedef = &network->def->routes[i]; + virSocketAddrPtr gateway = NULL; + + routedef = network->def->routes[i]; + gateway = virNetworkRouteDefGetGateway(routedef); + /* Add the IP route to the bridge */ /* ignore errors, error msg will be generated */ /* but libvirt will not know and net-destroy will work. */ - if (VIR_SOCKET_ADDR_VALID(&routedef->gateway)) { + if (VIR_SOCKET_ADDR_VALID(gateway)) {
I wonder why this check was here in the first place - wouldn't we have disallowed the route definition in the first place if there wasn't a valid gateway? (again, this is just idle speculation. The task of this patch was to reorganize the existing code, not change behavior :-)
if (networkAddRouteToBridge(network, routedef) < 0) { /* an error occurred adding the static route */ continue; /* for now, do nothing */
In the end, ACK with the change to handling of family during parsing, and combining the few virSocketAddrFormat() calls with the subsequent conditionals (and the indentation problem Michal pointed out). The other issues should be handled in a separate patch which you needn't worry about (you've gone far enough beyond the call of duty already :-)
Hi Laine, On Thu, 2015-01-15 at 13:34 -0500, Laine Stump wrote:
(Gene - I Cc'ed you because of one question I have for you down in the bowels of the review. Just search for "Gene" and you'll get to it).
Looks like you forgot to actually add Gene to CC ;)
On 01/15/2015 04:25 AM, Cédric Bosdonnat wrote:
Moving code for parsing and formatting network routes to networkcommon_conf helps reusing those routes for domains. The route definition has been hidden to help reducing the number of unnecessary checks in the format function. --- po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/network_conf.c | 297 ++---------------------------- src/conf/network_conf.h | 22 +-- src/conf/networkcommon_conf.c | 414 ++++++++++++++++++++++++++++++++++++++++++ src/conf/networkcommon_conf.h | 72 ++++++++ src/libvirt_private.syms | 11 ++ src/network/bridge_driver.c | 44 ++--- 8 files changed, 526 insertions(+), 338 deletions(-) create mode 100644 src/conf/networkcommon_conf.c create mode 100644 src/conf/networkcommon_conf.h
diff --git a/po/POTFILES.in b/po/POTFILES.in index 094c8e3..3064037 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -25,6 +25,7 @@ src/conf/netdev_bandwidth_conf.c src/conf/netdev_vlan_conf.c src/conf/netdev_vport_profile_conf.c src/conf/network_conf.c +src/conf/networkcommon_conf.c src/conf/node_device_conf.c src/conf/numatune_conf.c src/conf/nwfilter_conf.c diff --git a/src/Makefile.am b/src/Makefile.am index 216abac..4bba536 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -287,7 +287,8 @@ NETWORK_EVENT_SOURCES = \
# Network driver generic impl APIs NETWORK_CONF_SOURCES = \ - conf/network_conf.c conf/network_conf.h + conf/network_conf.c conf/network_conf.h \ + conf/networkcommon_conf.c conf/networkcommon_conf.h
# Network filter driver generic impl APIs NWFILTER_PARAM_CONF_SOURCES = \ diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 26fe18d..66f9b6c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -163,12 +163,6 @@ virNetworkIpDefClear(virNetworkIpDefPtr def) }
static void -virNetworkRouteDefClear(virNetworkRouteDefPtr def) -{ - VIR_FREE(def->family); -} - -static void virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def) { VIR_FREE(def->name); @@ -251,7 +245,7 @@ virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def->ips);
for (i = 0; i < def->nroutes && def->routes; i++) - virNetworkRouteDefClear(&def->routes[i]); + virNetworkRouteDefFree(def->routes[i]); VIR_FREE(def->routes);
for (i = 0; i < def->nPortGroups && def->portGroups; i++) @@ -1371,232 +1365,6 @@ virNetworkIPDefParseXML(const char *networkName, }
static int -virNetworkRouteDefParseXML(const char *networkName, - xmlNodePtr node, - xmlXPathContextPtr ctxt, - virNetworkRouteDefPtr def) -{ - /* - * virNetworkRouteDef object is already allocated as part - * of an array. On failure clear: it out, but don't free it. - */ - - xmlNodePtr save; - char *address = NULL, *netmask = NULL; - char *gateway = NULL; - unsigned long prefix = 0, metric = 0; - int result = -1; - int prefixRc, metricRc; - virSocketAddr testAddr; - - save = ctxt->node; - ctxt->node = node; - - /* grab raw data from XML */ - def->family = virXPathString("string(./@family)", ctxt); - address = virXPathString("string(./@address)", ctxt); - netmask = virXPathString("string(./@netmask)", ctxt); - gateway = virXPathString("string(./@gateway)", ctxt); - prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix); - if (prefixRc == -2) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix specified " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - def->has_prefix = (prefixRc == 0); - def->prefix = prefix; - metricRc = virXPathULong("string(./@metric)", ctxt, &metric); - if (metricRc == -2) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid metric specified " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - if (metricRc == 0) { - def->has_metric = true; - if (metric == 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid metric value, must be > 0 " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - } - def->metric = metric; - - /* Note: both network and gateway addresses must be specified */ - - if (!address) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required address attribute " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - - if (!gateway) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing required gateway attribute " - "in route definition of network '%s'"), - networkName); - goto cleanup; - } - - if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad network address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - - if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad gateway address '%s' " - "in route definition of network '%s'"), - gateway, networkName); - goto cleanup; - } - - /* validate network address, etc. for each family */ - if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) { - if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) || - VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) { - virReportError(VIR_ERR_XML_ERROR, - def->family == NULL ? - _("No family specified for non-IPv4 address '%s' " - "in route definition of network '%s'") : - _("IPv4 family specified for non-IPv4 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) { - virReportError(VIR_ERR_XML_ERROR, - def->family == NULL ? - _("No family specified for non-IPv4 gateway '%s' " - "in route definition of network '%s'") : - _("IPv4 family specified for non-IPv4 gateway '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (netmask) { - if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Bad netmask address '%s' " - "in route definition of network '%s'"), - netmask, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) { - virReportError(VIR_ERR_XML_ERROR, - _("Network '%s' has invalid netmask '%s' " - "for address '%s' (both must be IPv4)"), - networkName, netmask, address); - goto cleanup; - } - if (def->has_prefix) { - /* can't have both netmask and prefix at the same time */ - virReportError(VIR_ERR_XML_ERROR, - _("Route definition '%s' cannot have both " - "a prefix and a netmask"), - networkName); - goto cleanup; - } - } - if (def->prefix > 32) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix %u specified " - "in route definition of network '%s', " - "must be 0 - 32"), - def->prefix, networkName); - goto cleanup; - } - } else if (STREQ(def->family, "ipv6")) { - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { - virReportError(VIR_ERR_XML_ERROR, - _("ipv6 family specified for non-IPv6 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (netmask) { - virReportError(VIR_ERR_XML_ERROR, - _("Specifying netmask invalid for IPv6 address '%s' " - "in route definition of network '%s'"), - address, networkName); - goto cleanup; - } - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) { - virReportError(VIR_ERR_XML_ERROR, - _("ipv6 specified for non-IPv6 gateway address '%s' " - "in route definition of network '%s'"), - gateway, networkName); - goto cleanup; - } - if (def->prefix > 128) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid prefix %u specified " - "in route definition of network '%s', " - "must be 0 - 128"), - def->prefix, networkName); - goto cleanup; - } - } else { - virReportError(VIR_ERR_XML_ERROR, - _("Unrecognized family '%s' " - "in route definition of network'%s'"), - def->family, networkName); - goto cleanup; - } - - /* make sure the address is a network address */ - if (netmask) { - if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("error converting address '%s' with netmask '%s' " - "to network-address " - "in route definition of network '%s'"), - address, netmask, networkName); - goto cleanup; - } - } else { - if (virSocketAddrMaskByPrefix(&def->address, - def->prefix, &testAddr) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("error converting address '%s' with prefix %u " - "to network-address " - "in route definition of network '%s'"), - address, def->prefix, networkName); - goto cleanup; - } - } - if (!virSocketAddrEqual(&def->address, &testAddr)) { - virReportError(VIR_ERR_XML_ERROR, - _("address '%s' in route definition of network '%s' " - "is not a network address"), - address, networkName); - goto cleanup; - } - - result = 0; - - cleanup: - if (result < 0) - virNetworkRouteDefClear(def); - VIR_FREE(address); - VIR_FREE(netmask); - VIR_FREE(gateway); - - ctxt->node = save; - return result; -} - -static int virNetworkPortGroupParseXML(virPortGroupDefPtr def, xmlNodePtr node, xmlXPathContextPtr ctxt) @@ -2209,11 +1977,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; /* parse each definition */ for (i = 0; i < nRoutes; i++) { - if (virNetworkRouteDefParseXML(def->name, + virNetworkRouteDefPtr route = NULL; + + if (!(route = virNetworkRouteDefParseXML(def->name, routeNodes[i], - ctxt, - &def->routes[i]) < 0) + ctxt))) goto error; + def->routes[i] = route; def->nroutes++; }
@@ -2229,17 +1999,18 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) size_t j; virSocketAddr testAddr, testGw; bool addrMatch; - virNetworkRouteDefPtr gwdef = &def->routes[i]; + virNetworkRouteDefPtr gwdef = def->routes[i]; + virSocketAddrPtr gateway = virNetworkRouteDefGetGateway(gwdef); addrMatch = false; for (j = 0; j < nIps; j++) { virNetworkIpDefPtr def2 = &def->ips[j]; - if (VIR_SOCKET_ADDR_FAMILY(&gwdef->gateway) + if (VIR_SOCKET_ADDR_FAMILY(gateway) != VIR_SOCKET_ADDR_FAMILY(&def2->address)) { continue; } int prefix = virNetworkIpDefPrefix(def2); virSocketAddrMaskByPrefix(&def2->address, prefix, &testAddr); - virSocketAddrMaskByPrefix(&gwdef->gateway, prefix, &testGw); + virSocketAddrMaskByPrefix(gateway, prefix, &testGw); if (VIR_SOCKET_ADDR_VALID(&testAddr) && VIR_SOCKET_ADDR_VALID(&testGw) && virSocketAddrEqual(&testAddr, &testGw)) { @@ -2248,7 +2019,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) } } if (!addrMatch) { - char *gw = virSocketAddrFormat(&gwdef->gateway); + char *gw = virSocketAddrFormat(gateway); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unreachable static route gateway '%s' specified for network '%s'"), gw, def->name); @@ -2585,50 +2356,6 @@ virNetworkIpDefFormat(virBufferPtr buf, }
static int -virNetworkRouteDefFormat(virBufferPtr buf, - const virNetworkRouteDef *def) -{ - int result = -1; - - virBufferAddLit(buf, "<route"); - - if (def->family) - virBufferAsprintf(buf, " family='%s'", def->family); - if (VIR_SOCKET_ADDR_VALID(&def->address)) { - char *addr = virSocketAddrFormat(&def->address); - - if (!addr) - goto error; - virBufferAsprintf(buf, " address='%s'", addr); - VIR_FREE(addr); - } - if (VIR_SOCKET_ADDR_VALID(&def->netmask)) { - char *addr = virSocketAddrFormat(&def->netmask); - - if (!addr) - goto error; - virBufferAsprintf(buf, " netmask='%s'", addr); - VIR_FREE(addr); - } - if (def->has_prefix) - virBufferAsprintf(buf, " prefix='%u'", def->prefix); - if (VIR_SOCKET_ADDR_VALID(&def->gateway)) { - char *addr = virSocketAddrFormat(&def->gateway); - if (!addr) - goto error; - virBufferAsprintf(buf, " gateway='%s'", addr); - VIR_FREE(addr); - } - if (def->has_metric && def->metric > 0) - virBufferAsprintf(buf, " metric='%u'", def->metric); - virBufferAddLit(buf, "/>\n"); - - result = 0; - error: - return result; -} - -static int virPortGroupDefFormat(virBufferPtr buf, const virPortGroupDef *def) { @@ -2850,7 +2577,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, }
for (i = 0; i < def->nroutes; i++) { - if (virNetworkRouteDefFormat(buf, &def->routes[i]) < 0) + if (virNetworkRouteDefFormat(buf, def->routes[i]) < 0) goto error; }
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 8110028..b113e14 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -39,6 +39,7 @@ # include "virmacaddr.h" # include "device_conf.h" # include "virbitmap.h" +# include "networkcommon_conf.h"
typedef enum { VIR_NETWORK_FORWARD_NONE = 0, @@ -162,25 +163,6 @@ struct _virNetworkIpDef { virSocketAddr bootserver; };
-typedef struct _virNetworkRouteDef virNetworkRouteDef; -typedef virNetworkRouteDef *virNetworkRouteDefPtr; -struct _virNetworkRouteDef { - char *family; /* ipv4 or ipv6 - default is ipv4 */ - virSocketAddr address; /* Routed Network IP address */ - - /* One or the other of the following two will be used for a given - * Network address, but never both. The parser guarantees this. - * The virSocketAddrGetIpPrefix() can be used to get a - * valid prefix. - */ - virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */ - unsigned int prefix; /* ipv6 - only prefix allowed */ - bool has_prefix; /* prefix= was specified */ - unsigned int metric; /* value for metric (defaults to 1) */ - bool has_metric; /* metric= was specified */ - virSocketAddr gateway; /* gateway IP address for ip-route */ - }; - typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr; struct _virNetworkForwardIfDef { @@ -259,7 +241,7 @@ struct _virNetworkDef { virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this network */
size_t nroutes; - virNetworkRouteDefPtr routes; /* ptr to array of static routes on this interface */ + virNetworkRouteDefPtr *routes; /* ptr to array of static routes on this interface */
virNetworkDNSDef dns; /* dns related configuration */ virNetDevVPortProfilePtr virtPortProfile; diff --git a/src/conf/networkcommon_conf.c b/src/conf/networkcommon_conf.c new file mode 100644 index 0000000..1545367 --- /dev/null +++ b/src/conf/networkcommon_conf.c @@ -0,0 +1,414 @@ +/* + * networkcommon_conf.c: network XML handling + * + * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2008 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include "virerror.h" +#include "datatypes.h" +#include "networkcommon_conf.h" +#include "viralloc.h" +#include "virxml.h" + +#define VIR_FROM_THIS VIR_FROM_NETWORK + +struct _virNetworkRouteDef { + char *family; /* ipv4 or ipv6 - default is ipv4 */ + virSocketAddr address; /* Routed Network IP address */ + + /* One or the other of the following two will be used for a given + * Network address, but never both. The parser guarantees this. + * The virSocketAddrGetIpPrefix() can be used to get a + * valid prefix. + */ + virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */ + unsigned int prefix; /* ipv6 - only prefix allowed */ + bool has_prefix; /* prefix= was specified */ + unsigned int metric; /* value for metric (defaults to 1) */ + bool has_metric; /* metric= was specified */ + virSocketAddr gateway; /* gateway IP address for ip-route */ +}; + +void +virNetworkRouteDefFree(virNetworkRouteDefPtr def) +{ + VIR_FREE(def->family); + VIR_FREE(def); +} + +virNetworkRouteDefPtr +virNetworkRouteDefCreate(const char *errorDetail, + char *family, + char *address, + char *netmask, + char *gateway, + unsigned int prefix, + bool hasPrefix, + unsigned int metric, + bool hasMetric) +{ + virNetworkRouteDefPtr def = NULL; + virSocketAddr testAddr; + + if (VIR_ALLOC(def) < 0) + return NULL; + + def->family = family;
moving family rather than copying it works, but it complicates things down in the caller, which then has to make some intelligent decisions about whether or not the original pointer has been moved to somewhere and is still in use, or if it should be free'd (see below). For that reason, I think family should be VIR_STRDUP'ed here and the original unconditionally VIR_FREE'd in the caller.
(this wasn't an issue in the original code, because family was being parsed directly into the object)
That sounded weird to me too... I was hesitating to VIR_STRDUP it, but then it's one more point towards it. I'll change that.
+ def->prefix = prefix; + def->has_prefix = hasPrefix; + def->metric = metric; + def->has_metric = hasMetric; + + /* Note: both network and gateway addresses must be specified */ + + if (!address) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Missing required address attribute " + "in route definition"), + errorDetail); + goto error; + } + + if (!gateway) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Missing required gateway attribute " + "in route definition"), + errorDetail); + goto error; + } + + if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Bad network address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + + if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Bad gateway address '%s' " + "in route definition"), + errorDetail, gateway); + goto error; + } + + /* validate network address, etc. for each family */ + if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) { + if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) || + VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) { + virReportError(VIR_ERR_XML_ERROR, + def->family == NULL ? + _("%s: No family specified for non-IPv4 address '%s' " + "in route definition") : + _("%s: IPv4 family specified for non-IPv4 address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) { + virReportError(VIR_ERR_XML_ERROR, + def->family == NULL ? + _("%s: No family specified for non-IPv4 gateway '%s' " + "in route definition") : + _("%s: IPv4 family specified for non-IPv4 gateway '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (netmask) { + if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Bad netmask address '%s' " + "in route definition"), + errorDetail, netmask); + goto error; + } + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid netmask '%s' " + "for address '%s' (both must be IPv4)"), + errorDetail, netmask, address); + goto error; + } + if (def->has_prefix) { + /* can't have both netmask and prefix at the same time */ + virReportError(VIR_ERR_XML_ERROR, + _("%s: Route definition cannot have both " + "a prefix and a netmask"), + errorDetail); + goto error; + } + } + if (def->prefix > 32) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid prefix %u specified " + "in route definition, " + "must be 0 - 32"), + errorDetail, def->prefix); + goto error; + } + } else if (STREQ(def->family, "ipv6")) { + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: ipv6 family specified for non-IPv6 address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (netmask) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Specifying netmask invalid for IPv6 address '%s' " + "in route definition"), + errorDetail, address); + goto error; + } + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: ipv6 specified for non-IPv6 gateway address '%s' " + "in route definition"), + errorDetail, gateway); + goto error; + } + if (def->prefix > 128) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid prefix %u specified " + "in route definition, " + "must be 0 - 128"), + errorDetail, def->prefix); + goto error; + } + } else { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Unrecognized family '%s' " + "in route definition"), + errorDetail, def->family); + goto error; + } + + /* make sure the address is a network address */ + if (netmask) { + if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: Error converting address '%s' with netmask '%s' " + "to network-address " + "in route definition"), + errorDetail, address, netmask); + goto error; + } + } else { + if (virSocketAddrMaskByPrefix(&def->address, + def->prefix, &testAddr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: Error converting address '%s' with prefix %u " + "to network-address " + "in route definition"), + errorDetail, address, def->prefix); + goto error; + } + } + if (!virSocketAddrEqual(&def->address, &testAddr)) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Address '%s' in route definition " + "is not a network address"), + errorDetail, address); + goto error; + } + + return def; + + error: + virNetworkRouteDefFree(def); + return NULL; +} + +virNetworkRouteDefPtr +virNetworkRouteDefParseXML(const char *errorDetail, + xmlNodePtr node, + xmlXPathContextPtr ctxt) +{ + /* + * virNetworkRouteDef object is already allocated as part + * of an array. On failure clear: it out, but don't free it. + */ + + virNetworkRouteDefPtr def = NULL; + xmlNodePtr save; + char *family = NULL; + char *address = NULL, *netmask = NULL; + char *gateway = NULL; + unsigned long prefix = 0, metric = 0; + int prefixRc, metricRc; + bool hasPrefix = false; + bool hasMetric = false; + + save = ctxt->node; + ctxt->node = node; + + /* grab raw data from XML */ + family = virXPathString("string(./@family)", ctxt); + address = virXPathString("string(./@address)", ctxt); + netmask = virXPathString("string(./@netmask)", ctxt); + gateway = virXPathString("string(./@gateway)", ctxt); + prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix); + if (prefixRc == -2) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid prefix specified " + "in route definition"), + errorDetail); + goto cleanup; + } + hasPrefix = (prefixRc == 0); + metricRc = virXPathULong("string(./@metric)", ctxt, &metric); + if (metricRc == -2) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid metric specified " + "in route definition"), + errorDetail); + goto cleanup; + } + if (metricRc == 0) { + hasMetric = true; + if (metric == 0) { + virReportError(VIR_ERR_XML_ERROR, + _("%s: Invalid metric value, must be > 0 " + "in route definition"), + errorDetail); + goto cleanup; + } + } + + def = virNetworkRouteDefCreate(errorDetail, family, address, netmask, + gateway, prefix, hasPrefix, metric, + hasMetric); + + cleanup: + ctxt->node = save; + VIR_FREE(address); + VIR_FREE(netmask); + VIR_FREE(gateway);
You haven't free'd family, which will cause a leak if an error is found prior to calling virNetworkRouteDefCreate(). Of course since virNetworkRouteDefCreate() *moves* family into the object rather than copying it, if you indiscriminately free the original pointer here, you've just free'd something that's still in use in the case of non-failure. (more explanation of why I think that virNetworkRouteDefCreate() should VIR_STRDUP family instead of moving it)
+ return def; +} + +int +virNetworkRouteDefFormat(virBufferPtr buf, + const virNetworkRouteDef *def) +{ + int result = -1; + char *addr = NULL; + + virBufferAddLit(buf, "<route"); + + if (def->family) + virBufferAsprintf(buf, " family='%s'", def->family); + + addr = virSocketAddrFormat(&def->address); + + if (!addr) + goto error;
Since you need to change the handling of family anyway, while you're moving this, you could change these usages to:
if (!(addr = virSocketAddrFormat(&def->address))) goto error;
to save lines.
Ok indeed.
+ virBufferAsprintf(buf, " address='%s'", addr); + VIR_FREE(addr); + + if (VIR_SOCKET_ADDR_VALID(&def->netmask)) { + addr = virSocketAddrFormat(&def->netmask); + + if (!addr) + goto error;
same here
+ virBufferAsprintf(buf, " netmask='%s'", addr); + VIR_FREE(addr); + } + if (def->has_prefix) + virBufferAsprintf(buf, " prefix='%u'", def->prefix); + + addr = virSocketAddrFormat(&def->gateway); + if (!addr) + goto error;
and here. etc.
+ virBufferAsprintf(buf, " gateway='%s'", addr); + VIR_FREE(addr); + + if (def->has_metric && def->metric > 0) + virBufferAsprintf(buf, " metric='%u'", def->metric); + virBufferAddLit(buf, "/>\n"); + + result = 0; + error: + return result;
Also, while you're moving it, can you change this error label to cleanup? We didn't use to pay too much attention to this, but for awhile now we've been trying to use "error" only for codepaths that are taken exclusively in case of an error, and "cleanup" for those that are executed in all cases.
Ok. I'll rename it.
+} + +virSocketAddrPtr +virNetworkRouteDefGetAddress(virNetworkRouteDefPtr def) +{ + if (def) + return &def->address; + + return NULL; +} + +int +virNetworkRouteDefGetPrefix(virNetworkRouteDefPtr def) +{ + int prefix = 0; + virSocketAddr zero; + + if (!def) + return -1; + + /* this creates an all-0 address of the appropriate family */ + ignore_value(virSocketAddrParse(&zero, + (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) + ? VIR_SOCKET_ADDR_IPV4_ALL + : VIR_SOCKET_ADDR_IPV6_ALL), + VIR_SOCKET_ADDR_FAMILY(&def->address))); + + if (virSocketAddrEqual(&def->address, &zero)) {
+ if (def->has_prefix && def->prefix == 0) + prefix = 0;
I understand that you created this function by just moving existing code, so I hesitate to even get into the subject of refactoring it here (as a matter of fact, the more I think about it, the more I think it's better to *not* refactor as I will suggest below, in order to avoid unintentional regressions when backporting to branches. But I've already typed it, so I'll leave it in :-)
I think the above chunk should be outside all of the conditionals and shouldn't check for == 0 - no matter what else is true, if the user specified a prefix, that is what should be returned. Likewise, if they specified a netmask, then the netmask should be converted to a prefix no matter what else was supplied. Only if there is no valid prefix and no valid netmask, *then* we should create a prefix based on the class/family of address.
The following seems like a proper way to implement this (although again - I don't think I want such a substantial change in what is essentially a code movement patch. Instead just leave it as is, and I'll make a patch implementing my idea after you've pushed yours):
1) change virSocketAddrGetIpPrefix() to add the check for address == 0 (separately for ipv6 and ipv4) and set prefix to 0 in those cases. 2) virNetworkRouteDefGetPrefix() can be simplified to do this: * if prefix_specified is true, just return prefix (even if it's 0), otherwise return the result of virSocketAddrGetIpPrefix().
+ else if ((VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET) && + virSocketAddrEqual(&def->netmask, &zero))) + prefix = 0; + else + prefix = virSocketAddrGetIpPrefix(&def->address, &def->netmask, + def->prefix); + } else { + prefix = virSocketAddrGetIpPrefix(&def->address, &def->netmask, + def->prefix); + } + + return prefix; +} + +unsigned int +virNetworkRouteDefGetMetric(virNetworkRouteDefPtr def) +{ + if (def && def->has_metric && def->metric > 0) + return def->metric; + + return 1;
Hmm. Looking at the existing code, it does look like "1" is the default setting for metric. But when I look at "man route" I see that 0 is the default metric for IPv4 and 1 for IPv6. So why don't we allow a metric of 0? Gene?
+} + +virSocketAddrPtr +virNetworkRouteDefGetGateway(virNetworkRouteDefPtr def) +{ + if (def) + return &def->gateway; + return NULL; +} diff --git a/src/conf/networkcommon_conf.h b/src/conf/networkcommon_conf.h new file mode 100644 index 0000000..4d2b1d2 --- /dev/null +++ b/src/conf/networkcommon_conf.h @@ -0,0 +1,72 @@ +/* + * networkcommon_conf.h: network XML handling + * + * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#ifndef __NETWORKCOMMON_CONF_H__ +# define __NETWORKCOMMON_CONF_H__ + +# include <libxml/tree.h> +# include <libxml/xpath.h> + +# include "internal.h" +# include "virbuffer.h" +# include "virsocketaddr.h" + +typedef struct _virNetworkRouteDef virNetworkRouteDef; +typedef virNetworkRouteDef *virNetworkRouteDefPtr; + +void +virNetworkRouteDefFree(virNetworkRouteDefPtr def); + +virNetworkRouteDefPtr +virNetworkRouteDefCreate(const char *networkName, + char *family, + char *address, + char *netmask, + char *gateway, + unsigned int prefix, + bool hasPrefix, + unsigned int metric, + bool hasMetric); + +virNetworkRouteDefPtr +virNetworkRouteDefParseXML(const char *networkName, + xmlNodePtr node, + xmlXPathContextPtr ctxt); +int +virNetworkRouteDefFormat(virBufferPtr buf, + const virNetworkRouteDef *def); + +virSocketAddrPtr +virNetworkRouteDefGetAddress(virNetworkRouteDefPtr def); + +int +virNetworkRouteDefGetPrefix(virNetworkRouteDefPtr def); + +unsigned int +virNetworkRouteDefGetMetric(virNetworkRouteDefPtr def); + +virSocketAddrPtr +virNetworkRouteDefGetGateway(virNetworkRouteDefPtr def); + +#endif /* __NETWORKCOMMON_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7ceb54d..a35f077 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -582,6 +582,17 @@ virNetworkEventLifecycleNew; virNetworkEventStateRegisterID;
+# conf/networkcommon_conf.h +virNetworkRouteDefCreate; +virNetworkRouteDefFormat; +virNetworkRouteDefFree; +virNetworkRouteDefGetAddress; +virNetworkRouteDefGetGateway; +virNetworkRouteDefGetMetric; +virNetworkRouteDefGetPrefix; +virNetworkRouteDefParseXML; + + # conf/node_device_conf.h virNodeDevCapsDefFree; virNodeDevCapTypeFromString; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fca60f1..7b84e27 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1934,29 +1934,10 @@ static int networkAddRouteToBridge(virNetworkObjPtr network, virNetworkRouteDefPtr routedef) { - int prefix = 0; - unsigned int metric; - virSocketAddrPtr addr = &routedef->address; - virSocketAddrPtr mask = &routedef->netmask; - virSocketAddr zero; - - /* this creates an all-0 address of the appropriate family */ - ignore_value(virSocketAddrParse(&zero, - (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET) - ? "0.0.0.0" : "::"), - VIR_SOCKET_ADDR_FAMILY(addr))); - - if (virSocketAddrEqual(addr, &zero)) { - if (routedef->has_prefix && routedef->prefix == 0) - prefix = 0; - else if ((VIR_SOCKET_ADDR_IS_FAMILY(mask, AF_INET) && - virSocketAddrEqual(mask, &zero))) - prefix = 0; - else - prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); - } else { - prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); - } + int prefix = virNetworkRouteDefGetPrefix(routedef); + unsigned int metric = virNetworkRouteDefGetMetric(routedef); + virSocketAddrPtr addr = virNetworkRouteDefGetAddress(routedef); + virSocketAddrPtr gateway = virNetworkRouteDefGetGateway(routedef);
if (prefix < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1966,13 +1947,8 @@ networkAddRouteToBridge(virNetworkObjPtr network, return -1; }
- if (routedef->has_metric && routedef->metric > 0) - metric = routedef->metric; - else - metric = 1; - - if (virNetDevAddRoute(network->def->bridge, &routedef->address, - prefix, &routedef->gateway, metric) < 0) { + if (virNetDevAddRoute(network->def->bridge, addr, + prefix, gateway, metric) < 0) { return -1; } return 0; @@ -2063,11 +2039,15 @@ networkStartNetworkVirtual(virNetworkObjPtr network) goto err2;
for (i = 0; i < network->def->nroutes; i++) { - routedef = &network->def->routes[i]; + virSocketAddrPtr gateway = NULL; + + routedef = network->def->routes[i]; + gateway = virNetworkRouteDefGetGateway(routedef); + /* Add the IP route to the bridge */ /* ignore errors, error msg will be generated */ /* but libvirt will not know and net-destroy will work. */ - if (VIR_SOCKET_ADDR_VALID(&routedef->gateway)) { + if (VIR_SOCKET_ADDR_VALID(gateway)) {
I wonder why this check was here in the first place - wouldn't we have disallowed the route definition in the first place if there wasn't a valid gateway? (again, this is just idle speculation. The task of this patch was to reorganize the existing code, not change behavior :-)
if (networkAddRouteToBridge(network, routedef) < 0) { /* an error occurred adding the static route */ continue; /* for now, do nothing */
In the end, ACK with the change to handling of family during parsing, and combining the few virSocketAddrFormat() calls with the subsequent conditionals (and the indentation problem Michal pointed out). The other issues should be handled in a separate patch which you needn't worry about (you've gone far enough beyond the call of duty already :-)
Ok, I'll push with those changes. -- Cedric
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/16/2015 02:45 AM, Cedric Bosdonnat wrote:
Hi Laine,
On Thu, 2015-01-15 at 13:34 -0500, Laine Stump wrote:
(Gene - I Cc'ed you because of one question I have for you down in the bowels of the review. Just search for "Gene" and you'll get to it). Looks like you forgot to actually add Gene to CC ;)
You had me concerned, but then I checked my Sent folder and found that I had just accidentally done a Bcc instead of Cc. Thanks for noticing, though!
--- docs/formatdomain.html.in | 9 +- docs/schemas/domaincommon.rng | 29 +----- docs/schemas/network.rng | 2 +- docs/schemas/networkcommon.rng | 2 +- src/conf/domain_conf.c | 121 +++++----------------- src/conf/domain_conf.h | 14 +-- src/conf/networkcommon_conf.c | 6 +- src/conf/networkcommon_conf.h | 6 +- src/lxc/lxc_container.c | 22 ++-- src/lxc/lxc_native.c | 20 ++-- tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml | 4 +- tests/lxcconf2xmldata/lxcconf2xml-simple.xml | 4 +- tests/lxcxml2xmldata/lxc-hostdev.xml | 4 +- tests/lxcxml2xmldata/lxc-idmap.xml | 4 +- 14 files changed, 74 insertions(+), 173 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0c3343e..1d69ca6 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4371,12 +4371,9 @@ qemu-kvm -net nic,model=? /dev/null <p> <span class="since">Since 1.2.12</span> route elements can also be added - to define the network routes to use for the network device. This element - has a <code>family</code> attribute set either to <code>ipv4</code> or - <code>ipv6</code>, a mandatory <code>via</code> attribute defining the - IP address to route throught and optional <code>address</code> and <code>prefix</code> - attributes defining the target network range. If those aren't given, then - a default route will be set. + to define the network routes to use for the network device. The attributes + of this element are described in the documentation for the <code>route</code> + element in <a href="formatnetwork.html#elementsStaticroute">network definitions</a>. This is only used by the LXC driver. </p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 85b3709..af7abd2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2330,9 +2330,7 @@ </element> </zeroOrMore> <zeroOrMore> - <element name="route"> - <ref name="route"/> - </element> + <ref name="route"/> </zeroOrMore> <optional> <element name="script"> @@ -3602,27 +3600,6 @@ </element> </define> - <define name="route"> - <interleave> - <attribute name="family"> - <ref name="addr-family"/> - </attribute> - <attribute name="via"> - <ref name="ipAddr"/> - </attribute> - <optional> - <attribute name="address"> - <ref name="ipAddr"/> - </attribute> - </optional> - <optional> - <attribute name="prefix"> - <ref name="ipPrefix"/> - </attribute> - </optional> - </interleave> - </define> - <define name="hostdev"> <element name="hostdev"> <interleave> @@ -3859,9 +3836,7 @@ </element> </zeroOrMore> <zeroOrMore> - <element name="route"> - <ref name="route"/> - </element> + <ref name="route"/> </zeroOrMore> </interleave> </define> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 63d81c1..a6b8cb2 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -371,7 +371,7 @@ </zeroOrMore> <!-- <route> element --> <zeroOrMore> - <ref name="routex"/> + <ref name="route"/> </zeroOrMore> </interleave> </element> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index cbcae91..162ea3d 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -228,7 +228,7 @@ <!-- The (static) route element specifies a network address and gateway address to access that network. Both the network address and the gateway address must be specified. --> - <define name='routex'> + <define name='route'> <element name="route"> <optional> <attribute name="family"><ref name="addr-family"/></attribute> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 714ad56..3c5f279 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3,6 +3,7 @@ * * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -1447,10 +1448,10 @@ void virDomainNetDefFree(virDomainNetDefPtr def) VIR_FREE(def->ips); for (i = 0; i < def->nroutes; i++) - VIR_FREE(def->routes[i]); + virNetworkRouteDefFree(def->routes[i]); VIR_FREE(def->routes); - virDomainDeviceInfoClear(&def->info); + virDomainDeviceInfoClear(&def->info); VIR_FREE(def->filter); virNWFilterHashTableFree(def->filterparams); @@ -4809,64 +4810,6 @@ virDomainNetIpParseXML(xmlNodePtr node) return NULL; } -static virDomainNetRouteDefPtr -virDomainNetRouteParse(xmlNodePtr node) -{ - virDomainNetRouteDefPtr route = NULL; - char *familyStr = NULL; - int family = AF_UNSPEC; - char *via = NULL; - char *to = NULL; - char *prefixStr = NULL; - - to = virXMLPropString(node, "address"); - if (!(via = virXMLPropString(node, "via"))) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Missing route address")); - goto error; - } - - familyStr = virXMLPropString(node, "family"); - if (familyStr && STREQ(familyStr, "ipv4")) - family = AF_INET; - else if (familyStr && STREQ(familyStr, "ipv6")) - family = AF_INET6; - else - family = virSocketAddrNumericFamily(via); - - if (VIR_ALLOC(route) < 0) - goto error; - - if (virSocketAddrParse(&route->via, via, family) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Failed to parse IP address: '%s'"), - via); - goto error; - } - - if (to && virSocketAddrParse(&route->to, to, family) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Failed to parse IP address: '%s'"), - to); - goto error; - } - - if (!(prefixStr = virXMLPropString(node, "prefix")) || - (virStrToLong_ui(prefixStr, NULL, 10, &route->prefix) < 0)) { - } - - return route; - - error: - VIR_FREE(familyStr); - VIR_FREE(via); - VIR_FREE(to); - VIR_FREE(prefixStr); - VIR_FREE(route); - - return NULL; -} - static int virDomainHostdevDefParseXMLCaps(xmlNodePtr node ATTRIBUTE_UNUSED, xmlXPathContextPtr ctxt, @@ -4960,14 +4903,17 @@ virDomainHostdevDefParseXMLCaps(xmlNodePtr node ATTRIBUTE_UNUSED, if (nroutenodes) { size_t i; for (i = 0; i < nroutenodes; i++) { - virDomainNetRouteDefPtr route = virDomainNetRouteParse(routenodes[i]); + virNetworkRouteDefPtr route = NULL; - if (!route) + if (!(route = virNetworkRouteDefParseXML(_("Domain hostdev device"), + routenodes[i], + ctxt))) goto error; + if (VIR_APPEND_ELEMENT(def->source.caps.u.net.routes, def->source.caps.u.net.nroutes, route) < 0) { - VIR_FREE(route); + virNetworkRouteDefFree(route); goto error; } } @@ -7428,7 +7374,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, size_t nips = 0; virDomainNetIpDefPtr *ips = NULL; size_t nroutes = 0; - virDomainNetRouteDefPtr *routes = NULL; + virNetworkRouteDefPtr *routes = NULL; if (VIR_ALLOC(def) < 0) return NULL; @@ -7526,12 +7472,15 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, if (VIR_APPEND_ELEMENT(ips, nips, ip) < 0) goto error; } else if (xmlStrEqual(cur->name, BAD_CAST "route")) { - virDomainNetRouteDefPtr route = NULL; - if (!(route = virDomainNetRouteParse(cur))) + virNetworkRouteDefPtr route = NULL; + if (!(route = virNetworkRouteDefParseXML(_("Domain interface"), + cur, ctxt))) goto error; - if (VIR_APPEND_ELEMENT(routes, nroutes, route) < 0) + if (VIR_APPEND_ELEMENT(routes, nroutes, route) < 0) { + virNetworkRouteDefFree(route); goto error; + } } else if (!ifname && xmlStrEqual(cur->name, BAD_CAST "target")) { ifname = virXMLPropString(cur, "dev"); @@ -17257,35 +17206,17 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) } } -static void +static int virDomainNetRoutesFormat(virBufferPtr buf, - virDomainNetRouteDefPtr *routes, + virNetworkRouteDefPtr *routes, size_t nroutes) { size_t i; - for (i = 0; i < nroutes; i++) { - virDomainNetRouteDefPtr route = routes[i]; - const char *familyStr = NULL; - char *via = virSocketAddrFormat(&route->via); - char *to = NULL; - - if (VIR_SOCKET_ADDR_IS_FAMILY(&route->via, AF_INET6)) - familyStr = "ipv6"; - else if (VIR_SOCKET_ADDR_IS_FAMILY(&route->via, AF_INET)) - familyStr = "ipv4"; - virBufferAsprintf(buf, "<route family='%s' via='%s'", familyStr, via); - - if (VIR_SOCKET_ADDR_VALID(&route->to)) { - to = virSocketAddrFormat(&route->to); - virBufferAsprintf(buf, " address='%s'", to); - } - - if (route->prefix > 0) - virBufferAsprintf(buf, " prefix='%d'", route->prefix); - - virBufferAddLit(buf, "/>\n"); - } + for (i = 0; i < nroutes; i++) + if (virNetworkRouteDefFormat(buf, routes[i]) < 0) + return -1; + return 0; } static int @@ -17443,8 +17374,9 @@ virDomainHostdevDefFormatCaps(virBufferPtr buf, if (def->source.caps.type == VIR_DOMAIN_HOSTDEV_CAPS_TYPE_NET) { virDomainNetIpsFormat(buf, def->source.caps.u.net.ips, def->source.caps.u.net.nips); - virDomainNetRoutesFormat(buf, def->source.caps.u.net.routes, - def->source.caps.u.net.nroutes); + if (virDomainNetRoutesFormat(buf, def->source.caps.u.net.routes, + def->source.caps.u.net.nroutes) < 0) + return -1; } return 0; @@ -17834,7 +17766,8 @@ virDomainNetDefFormat(virBufferPtr buf, } virDomainNetIpsFormat(buf, def->ips, def->nips); - virDomainNetRoutesFormat(buf, def->routes, def->nroutes); + if (virDomainNetRoutesFormat(buf, def->routes, def->nroutes) < 0) + return -1; virBufferEscapeString(buf, "<script path='%s'/>\n", def->script); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1153497..1e0d1b4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3,6 +3,7 @@ * * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -35,6 +36,7 @@ # include "virthread.h" # include "virhash.h" # include "virsocketaddr.h" +# include "networkcommon_conf.h" # include "nwfilter_params.h" # include "numatune_conf.h" # include "virnetdevmacvlan.h" @@ -485,14 +487,6 @@ struct _virDomainNetIpDef { unsigned int prefix; /* number of 1 bits in the net mask */ }; -typedef struct _virDomainNetRouteDef virDomainNetRouteDef; -typedef virDomainNetRouteDef *virDomainNetRouteDefPtr; -struct _virDomainNetRouteDef { - virSocketAddr via; - virSocketAddr to; - unsigned int prefix; -}; - typedef struct _virDomainHostdevCaps virDomainHostdevCaps; typedef virDomainHostdevCaps *virDomainHostdevCapsPtr; struct _virDomainHostdevCaps { @@ -509,7 +503,7 @@ struct _virDomainHostdevCaps { size_t nips; virDomainNetIpDefPtr *ips; size_t nroutes; - virDomainNetRouteDefPtr *routes; + virNetworkRouteDefPtr *routes; } net; } u; }; @@ -1013,7 +1007,7 @@ struct _virDomainNetDef { size_t nips; virDomainNetIpDefPtr *ips; size_t nroutes; - virDomainNetRouteDefPtr *routes; + virNetworkRouteDefPtr *routes; }; /* Used for prefix of ifname of any network name generated dynamically diff --git a/src/conf/networkcommon_conf.c b/src/conf/networkcommon_conf.c index 1545367..b6e4ff4 100644 --- a/src/conf/networkcommon_conf.c +++ b/src/conf/networkcommon_conf.c @@ -58,9 +58,9 @@ virNetworkRouteDefFree(virNetworkRouteDefPtr def) virNetworkRouteDefPtr virNetworkRouteDefCreate(const char *errorDetail, char *family, - char *address, - char *netmask, - char *gateway, + const char *address, + const char *netmask, + const char *gateway, unsigned int prefix, bool hasPrefix, unsigned int metric, diff --git a/src/conf/networkcommon_conf.h b/src/conf/networkcommon_conf.h index 4d2b1d2..1500d0f 100644 --- a/src/conf/networkcommon_conf.h +++ b/src/conf/networkcommon_conf.h @@ -41,9 +41,9 @@ virNetworkRouteDefFree(virNetworkRouteDefPtr def); virNetworkRouteDefPtr virNetworkRouteDefCreate(const char *networkName, char *family, - char *address, - char *netmask, - char *gateway, + const char *address, + const char *netmask, + const char *gateway, unsigned int prefix, bool hasPrefix, unsigned int metric, diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 0e6cdfd..e848e8e 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1,6 +1,7 @@ /* * Copyright (C) 2008-2014 Red Hat, Inc. * Copyright (C) 2008 IBM Corp. + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * * lxc_container.c: file description * @@ -544,20 +545,13 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, /* Set the routes */ for (j = 0; j < netDef->nroutes; j++) { - virDomainNetRouteDefPtr route = netDef->routes[j]; - if (VIR_SOCKET_ADDR_VALID(&route->to)) - toStr = virSocketAddrFormat(&route->to); - else - if (VIR_STRDUP(toStr, "default") < 0) - goto error_out; - viaStr = virSocketAddrFormat(&route->via); - VIR_DEBUG("Adding route %s/%d via %s", toStr, route->prefix, viaStr); - - if (virNetDevAddRoute(newname, &route->to, route->prefix, - &route->via, 0) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("Failed to add route %s/%d via %s"), - toStr, route->prefix, viaStr); + virNetworkRouteDefPtr route = netDef->routes[j]; + + if (virNetDevAddRoute(newname, + virNetworkRouteDefGetAddress(route), + virNetworkRouteDefGetPrefix(route), + virNetworkRouteDefGetGateway(route), + virNetworkRouteDefGetMetric(route)) < 0) { goto error_out; } VIR_FREE(toStr); diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index d7cd1d5..c9b1567 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -2,7 +2,7 @@ * lxc_native.c: LXC native configuration import * * Copyright (c) 2014 Red Hat, Inc. - * Copyright (c) 2013 SUSE LINUX Products GmbH, Nuernberg, Germany. + * Copyright (c) 2013-2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -433,15 +433,23 @@ typedef struct { static int lxcAddNetworkRouteDefinition(const char *address, int family, - virDomainNetRouteDefPtr **routes, + virNetworkRouteDefPtr **routes, size_t *nroutes) { - virDomainNetRouteDefPtr route = NULL; + virNetworkRouteDefPtr route = NULL; + char *familyStr = NULL; + char *zero = NULL; - if (VIR_ALLOC(route) < 0) + if (VIR_STRDUP(zero, family == AF_INET ? VIR_SOCKET_ADDR_IPV4_ALL + : VIR_SOCKET_ADDR_IPV6_ALL) < 0) goto error; - if (virSocketAddrParse(&route->via, address, family) < 0) + if (VIR_STRDUP(familyStr, family == AF_INET ? "ipv4" : "ipv6") < 0) + goto error; + + if (!(route = virNetworkRouteDefCreate(_("Domain interface"), familyStr, + zero, NULL, address, 0, false, + 0, false))) goto error; if (VIR_APPEND_ELEMENT(*routes, *nroutes, route) < 0) @@ -450,7 +458,7 @@ lxcAddNetworkRouteDefinition(const char *address, return 0; error: - VIR_FREE(route); + virNetworkRouteDefFree(route); return -1; } diff --git a/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml b/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml index d2cec8f..79bcfa0 100644 --- a/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml +++ b/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml @@ -27,8 +27,8 @@ </source> <ip address='192.168.122.2' family='ipv4' prefix='24'/> <ip address='2003:db8:1:0:214:1234:fe0b:3596' family='ipv6' prefix='64'/> - <route family='ipv4' via='192.168.122.1'/> - <route family='ipv6' via='2003:db8:1:0:214:1234:fe0b:3595'/> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/> + <route family='ipv6' address='::' gateway='2003:db8:1:0:214:1234:fe0b:3595'/> </hostdev> </devices> </domain> diff --git a/tests/lxcconf2xmldata/lxcconf2xml-simple.xml b/tests/lxcconf2xmldata/lxcconf2xml-simple.xml index b1210e5..45a2012 100644 --- a/tests/lxcconf2xmldata/lxcconf2xml-simple.xml +++ b/tests/lxcconf2xmldata/lxcconf2xml-simple.xml @@ -39,8 +39,8 @@ <source bridge='virbr0'/> <ip address='192.168.122.2' family='ipv4' prefix='24'/> <ip address='2003:db8:1:0:214:1234:fe0b:3596' family='ipv6' prefix='64'/> - <route family='ipv4' via='192.168.122.1'/> - <route family='ipv6' via='2003:db8:1:0:214:1234:fe0b:3595'/> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/> + <route family='ipv6' address='::' gateway='2003:db8:1:0:214:1234:fe0b:3595'/> <guest dev='eth0'/> <link state='up'/> </interface> diff --git a/tests/lxcxml2xmldata/lxc-hostdev.xml b/tests/lxcxml2xmldata/lxc-hostdev.xml index 61e8655..3972594 100644 --- a/tests/lxcxml2xmldata/lxc-hostdev.xml +++ b/tests/lxcxml2xmldata/lxc-hostdev.xml @@ -37,8 +37,8 @@ </source> <ip address='192.168.122.2' family='ipv4'/> <ip address='2003:db8:1:0:214:1234:fe0b:3596' family='ipv6' prefix='24'/> - <route family='ipv4' via='192.168.122.1'/> - <route family='ipv6' via='2003:db8:1:0:214:1234:fe0b:3595'/> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/> + <route family='ipv6' address='::' gateway='2003:db8:1:0:214:1234:fe0b:3595'/> </hostdev> </devices> </domain> diff --git a/tests/lxcxml2xmldata/lxc-idmap.xml b/tests/lxcxml2xmldata/lxc-idmap.xml index 2b04a65..b477636 100644 --- a/tests/lxcxml2xmldata/lxc-idmap.xml +++ b/tests/lxcxml2xmldata/lxc-idmap.xml @@ -30,8 +30,8 @@ <source bridge='bri0'/> <ip address='192.168.122.12' family='ipv4' prefix='24'/> <ip address='192.168.122.13' family='ipv4' prefix='24'/> - <route family='ipv4' via='192.168.122.1'/> - <route family='ipv4' via='192.168.124.1' address='192.168.124.0' prefix='24'/> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/> + <route family='ipv4' address='192.168.124.0' prefix='24' gateway='192.168.124.1'/> <target dev='veth0'/> <guest dev='eth2'/> </interface> -- 2.1.2
On 01/15/2015 04:25 AM, Cédric Bosdonnat wrote:
--- docs/formatdomain.html.in | 9 +- docs/schemas/domaincommon.rng | 29 +----- docs/schemas/network.rng | 2 +- docs/schemas/networkcommon.rng | 2 +- src/conf/domain_conf.c | 121 +++++----------------- src/conf/domain_conf.h | 14 +-- src/conf/networkcommon_conf.c | 6 +- src/conf/networkcommon_conf.h | 6 +- src/lxc/lxc_container.c | 22 ++-- src/lxc/lxc_native.c | 20 ++-- tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml | 4 +- tests/lxcconf2xmldata/lxcconf2xml-simple.xml | 4 +- tests/lxcxml2xmldata/lxc-hostdev.xml | 4 +- tests/lxcxml2xmldata/lxc-idmap.xml | 4 +- 14 files changed, 74 insertions(+), 173 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0c3343e..1d69ca6 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4371,12 +4371,9 @@ qemu-kvm -net nic,model=? /dev/null
<p> <span class="since">Since 1.2.12</span> route elements can also be added - to define the network routes to use for the network device. This element - has a <code>family</code> attribute set either to <code>ipv4</code> or - <code>ipv6</code>, a mandatory <code>via</code> attribute defining the - IP address to route throught and optional <code>address</code> and <code>prefix</code> - attributes defining the target network range. If those aren't given, then - a default route will be set. + to define the network routes to use for the network device. The attributes + of this element are described in the documentation for the <code>route</code> + element in <a href="formatnetwork.html#elementsStaticroute">network definitions</a>. This is only used by the LXC driver. </p>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 85b3709..af7abd2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2330,9 +2330,7 @@ </element> </zeroOrMore> <zeroOrMore> - <element name="route"> - <ref name="route"/> - </element> + <ref name="route"/> </zeroOrMore> <optional> <element name="script"> @@ -3602,27 +3600,6 @@ </element> </define>
- <define name="route"> - <interleave> - <attribute name="family"> - <ref name="addr-family"/> - </attribute> - <attribute name="via"> - <ref name="ipAddr"/> - </attribute> - <optional> - <attribute name="address"> - <ref name="ipAddr"/> - </attribute> - </optional> - <optional> - <attribute name="prefix"> - <ref name="ipPrefix"/> - </attribute> - </optional> - </interleave> - </define> - <define name="hostdev"> <element name="hostdev"> <interleave> @@ -3859,9 +3836,7 @@ </element> </zeroOrMore> <zeroOrMore> - <element name="route"> - <ref name="route"/> - </element> + <ref name="route"/> </zeroOrMore> </interleave> </define> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 63d81c1..a6b8cb2 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -371,7 +371,7 @@ </zeroOrMore> <!-- <route> element --> <zeroOrMore> - <ref name="routex"/> + <ref name="route"/> </zeroOrMore> </interleave> </element> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index cbcae91..162ea3d 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -228,7 +228,7 @@ <!-- The (static) route element specifies a network address and gateway address to access that network. Both the network address and the gateway address must be specified. --> - <define name='routex'> + <define name='route'> <element name="route"> <optional> <attribute name="family"><ref name="addr-family"/></attribute> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 714ad56..3c5f279 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3,6 +3,7 @@ * * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -1447,10 +1448,10 @@ void virDomainNetDefFree(virDomainNetDefPtr def) VIR_FREE(def->ips);
for (i = 0; i < def->nroutes; i++) - VIR_FREE(def->routes[i]); + virNetworkRouteDefFree(def->routes[i]); VIR_FREE(def->routes);
- virDomainDeviceInfoClear(&def->info); + virDomainDeviceInfoClear(&def->info);
VIR_FREE(def->filter); virNWFilterHashTableFree(def->filterparams); @@ -4809,64 +4810,6 @@ virDomainNetIpParseXML(xmlNodePtr node) return NULL; }
-static virDomainNetRouteDefPtr -virDomainNetRouteParse(xmlNodePtr node) -{ - virDomainNetRouteDefPtr route = NULL; - char *familyStr = NULL; - int family = AF_UNSPEC; - char *via = NULL; - char *to = NULL; - char *prefixStr = NULL; - - to = virXMLPropString(node, "address"); - if (!(via = virXMLPropString(node, "via"))) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Missing route address")); - goto error; - } - - familyStr = virXMLPropString(node, "family"); - if (familyStr && STREQ(familyStr, "ipv4")) - family = AF_INET; - else if (familyStr && STREQ(familyStr, "ipv6")) - family = AF_INET6; - else - family = virSocketAddrNumericFamily(via); - - if (VIR_ALLOC(route) < 0) - goto error; - - if (virSocketAddrParse(&route->via, via, family) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Failed to parse IP address: '%s'"), - via); - goto error; - } - - if (to && virSocketAddrParse(&route->to, to, family) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Failed to parse IP address: '%s'"), - to); - goto error; - } - - if (!(prefixStr = virXMLPropString(node, "prefix")) || - (virStrToLong_ui(prefixStr, NULL, 10, &route->prefix) < 0)) { - } - - return route; - - error: - VIR_FREE(familyStr); - VIR_FREE(via); - VIR_FREE(to); - VIR_FREE(prefixStr); - VIR_FREE(route); - - return NULL; -} - static int virDomainHostdevDefParseXMLCaps(xmlNodePtr node ATTRIBUTE_UNUSED, xmlXPathContextPtr ctxt, @@ -4960,14 +4903,17 @@ virDomainHostdevDefParseXMLCaps(xmlNodePtr node ATTRIBUTE_UNUSED, if (nroutenodes) { size_t i; for (i = 0; i < nroutenodes; i++) { - virDomainNetRouteDefPtr route = virDomainNetRouteParse(routenodes[i]); + virNetworkRouteDefPtr route = NULL;
- if (!route) + if (!(route = virNetworkRouteDefParseXML(_("Domain hostdev device"), + routenodes[i], + ctxt))) goto error;
+ if (VIR_APPEND_ELEMENT(def->source.caps.u.net.routes, def->source.caps.u.net.nroutes, route) < 0) { - VIR_FREE(route); + virNetworkRouteDefFree(route); goto error; } } @@ -7428,7 +7374,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, size_t nips = 0; virDomainNetIpDefPtr *ips = NULL; size_t nroutes = 0; - virDomainNetRouteDefPtr *routes = NULL; + virNetworkRouteDefPtr *routes = NULL;
if (VIR_ALLOC(def) < 0) return NULL; @@ -7526,12 +7472,15 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, if (VIR_APPEND_ELEMENT(ips, nips, ip) < 0) goto error; } else if (xmlStrEqual(cur->name, BAD_CAST "route")) { - virDomainNetRouteDefPtr route = NULL; - if (!(route = virDomainNetRouteParse(cur))) + virNetworkRouteDefPtr route = NULL; + if (!(route = virNetworkRouteDefParseXML(_("Domain interface"), + cur, ctxt))) goto error;
- if (VIR_APPEND_ELEMENT(routes, nroutes, route) < 0) + if (VIR_APPEND_ELEMENT(routes, nroutes, route) < 0) { + virNetworkRouteDefFree(route); goto error; + } } else if (!ifname && xmlStrEqual(cur->name, BAD_CAST "target")) { ifname = virXMLPropString(cur, "dev"); @@ -17257,35 +17206,17 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) } }
-static void +static int virDomainNetRoutesFormat(virBufferPtr buf, - virDomainNetRouteDefPtr *routes, + virNetworkRouteDefPtr *routes, size_t nroutes) { size_t i;
- for (i = 0; i < nroutes; i++) { - virDomainNetRouteDefPtr route = routes[i]; - const char *familyStr = NULL; - char *via = virSocketAddrFormat(&route->via); - char *to = NULL; - - if (VIR_SOCKET_ADDR_IS_FAMILY(&route->via, AF_INET6)) - familyStr = "ipv6"; - else if (VIR_SOCKET_ADDR_IS_FAMILY(&route->via, AF_INET)) - familyStr = "ipv4"; - virBufferAsprintf(buf, "<route family='%s' via='%s'", familyStr, via); - - if (VIR_SOCKET_ADDR_VALID(&route->to)) { - to = virSocketAddrFormat(&route->to); - virBufferAsprintf(buf, " address='%s'", to); - } - - if (route->prefix > 0) - virBufferAsprintf(buf, " prefix='%d'", route->prefix); - - virBufferAddLit(buf, "/>\n"); - } + for (i = 0; i < nroutes; i++) + if (virNetworkRouteDefFormat(buf, routes[i]) < 0) + return -1; + return 0; }
static int @@ -17443,8 +17374,9 @@ virDomainHostdevDefFormatCaps(virBufferPtr buf, if (def->source.caps.type == VIR_DOMAIN_HOSTDEV_CAPS_TYPE_NET) { virDomainNetIpsFormat(buf, def->source.caps.u.net.ips, def->source.caps.u.net.nips); - virDomainNetRoutesFormat(buf, def->source.caps.u.net.routes, - def->source.caps.u.net.nroutes); + if (virDomainNetRoutesFormat(buf, def->source.caps.u.net.routes, + def->source.caps.u.net.nroutes) < 0) + return -1; }
return 0; @@ -17834,7 +17766,8 @@ virDomainNetDefFormat(virBufferPtr buf, }
virDomainNetIpsFormat(buf, def->ips, def->nips); - virDomainNetRoutesFormat(buf, def->routes, def->nroutes); + if (virDomainNetRoutesFormat(buf, def->routes, def->nroutes) < 0) + return -1;
virBufferEscapeString(buf, "<script path='%s'/>\n", def->script); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1153497..1e0d1b4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3,6 +3,7 @@ * * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -35,6 +36,7 @@ # include "virthread.h" # include "virhash.h" # include "virsocketaddr.h" +# include "networkcommon_conf.h" # include "nwfilter_params.h" # include "numatune_conf.h" # include "virnetdevmacvlan.h" @@ -485,14 +487,6 @@ struct _virDomainNetIpDef { unsigned int prefix; /* number of 1 bits in the net mask */ };
-typedef struct _virDomainNetRouteDef virDomainNetRouteDef; -typedef virDomainNetRouteDef *virDomainNetRouteDefPtr; -struct _virDomainNetRouteDef { - virSocketAddr via; - virSocketAddr to; - unsigned int prefix; -}; - typedef struct _virDomainHostdevCaps virDomainHostdevCaps; typedef virDomainHostdevCaps *virDomainHostdevCapsPtr; struct _virDomainHostdevCaps { @@ -509,7 +503,7 @@ struct _virDomainHostdevCaps { size_t nips; virDomainNetIpDefPtr *ips; size_t nroutes; - virDomainNetRouteDefPtr *routes; + virNetworkRouteDefPtr *routes; } net; } u; }; @@ -1013,7 +1007,7 @@ struct _virDomainNetDef { size_t nips; virDomainNetIpDefPtr *ips; size_t nroutes; - virDomainNetRouteDefPtr *routes; + virNetworkRouteDefPtr *routes; };
/* Used for prefix of ifname of any network name generated dynamically diff --git a/src/conf/networkcommon_conf.c b/src/conf/networkcommon_conf.c index 1545367..b6e4ff4 100644 --- a/src/conf/networkcommon_conf.c +++ b/src/conf/networkcommon_conf.c @@ -58,9 +58,9 @@ virNetworkRouteDefFree(virNetworkRouteDefPtr def) virNetworkRouteDefPtr virNetworkRouteDefCreate(const char *errorDetail, char *family, - char *address, - char *netmask, - char *gateway, + const char *address, + const char *netmask, + const char *gateway, unsigned int prefix, bool hasPrefix, unsigned int metric, diff --git a/src/conf/networkcommon_conf.h b/src/conf/networkcommon_conf.h index 4d2b1d2..1500d0f 100644 --- a/src/conf/networkcommon_conf.h +++ b/src/conf/networkcommon_conf.h @@ -41,9 +41,9 @@ virNetworkRouteDefFree(virNetworkRouteDefPtr def); virNetworkRouteDefPtr virNetworkRouteDefCreate(const char *networkName, char *family, - char *address, - char *netmask, - char *gateway, + const char *address, + const char *netmask, + const char *gateway, unsigned int prefix, bool hasPrefix, unsigned int metric, diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 0e6cdfd..e848e8e 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1,6 +1,7 @@ /* * Copyright (C) 2008-2014 Red Hat, Inc. * Copyright (C) 2008 IBM Corp. + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * * lxc_container.c: file description * @@ -544,20 +545,13 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef,
/* Set the routes */ for (j = 0; j < netDef->nroutes; j++) { - virDomainNetRouteDefPtr route = netDef->routes[j]; - if (VIR_SOCKET_ADDR_VALID(&route->to)) - toStr = virSocketAddrFormat(&route->to); - else - if (VIR_STRDUP(toStr, "default") < 0) - goto error_out; - viaStr = virSocketAddrFormat(&route->via); - VIR_DEBUG("Adding route %s/%d via %s", toStr, route->prefix, viaStr); - - if (virNetDevAddRoute(newname, &route->to, route->prefix, - &route->via, 0) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("Failed to add route %s/%d via %s"), - toStr, route->prefix, viaStr); + virNetworkRouteDefPtr route = netDef->routes[j]; + + if (virNetDevAddRoute(newname, + virNetworkRouteDefGetAddress(route), + virNetworkRouteDefGetPrefix(route), + virNetworkRouteDefGetGateway(route), + virNetworkRouteDefGetMetric(route)) < 0) { goto error_out; } VIR_FREE(toStr); diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index d7cd1d5..c9b1567 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -2,7 +2,7 @@ * lxc_native.c: LXC native configuration import * * Copyright (c) 2014 Red Hat, Inc. - * Copyright (c) 2013 SUSE LINUX Products GmbH, Nuernberg, Germany. + * Copyright (c) 2013-2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -433,15 +433,23 @@ typedef struct { static int lxcAddNetworkRouteDefinition(const char *address, int family, - virDomainNetRouteDefPtr **routes, + virNetworkRouteDefPtr **routes, size_t *nroutes) { - virDomainNetRouteDefPtr route = NULL; + virNetworkRouteDefPtr route = NULL; + char *familyStr = NULL; + char *zero = NULL;
- if (VIR_ALLOC(route) < 0) + if (VIR_STRDUP(zero, family == AF_INET ? VIR_SOCKET_ADDR_IPV4_ALL + : VIR_SOCKET_ADDR_IPV6_ALL) < 0) goto error;
- if (virSocketAddrParse(&route->via, address, family) < 0) + if (VIR_STRDUP(familyStr, family == AF_INET ? "ipv4" : "ipv6") < 0) + goto error; + + if (!(route = virNetworkRouteDefCreate(_("Domain interface"), familyStr, + zero, NULL, address, 0, false, + 0, false))) goto error;
if (VIR_APPEND_ELEMENT(*routes, *nroutes, route) < 0) @@ -450,7 +458,7 @@ lxcAddNetworkRouteDefinition(const char *address, return 0;
error: - VIR_FREE(route); + virNetworkRouteDefFree(route); return -1; }
diff --git a/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml b/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml index d2cec8f..79bcfa0 100644 --- a/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml +++ b/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml @@ -27,8 +27,8 @@ </source> <ip address='192.168.122.2' family='ipv4' prefix='24'/> <ip address='2003:db8:1:0:214:1234:fe0b:3596' family='ipv6' prefix='64'/> - <route family='ipv4' via='192.168.122.1'/> - <route family='ipv6' via='2003:db8:1:0:214:1234:fe0b:3595'/> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/> + <route family='ipv6' address='::' gateway='2003:db8:1:0:214:1234:fe0b:3595'/> </hostdev> </devices> </domain> diff --git a/tests/lxcconf2xmldata/lxcconf2xml-simple.xml b/tests/lxcconf2xmldata/lxcconf2xml-simple.xml index b1210e5..45a2012 100644 --- a/tests/lxcconf2xmldata/lxcconf2xml-simple.xml +++ b/tests/lxcconf2xmldata/lxcconf2xml-simple.xml @@ -39,8 +39,8 @@ <source bridge='virbr0'/> <ip address='192.168.122.2' family='ipv4' prefix='24'/> <ip address='2003:db8:1:0:214:1234:fe0b:3596' family='ipv6' prefix='64'/> - <route family='ipv4' via='192.168.122.1'/> - <route family='ipv6' via='2003:db8:1:0:214:1234:fe0b:3595'/> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/> + <route family='ipv6' address='::' gateway='2003:db8:1:0:214:1234:fe0b:3595'/> <guest dev='eth0'/> <link state='up'/> </interface> diff --git a/tests/lxcxml2xmldata/lxc-hostdev.xml b/tests/lxcxml2xmldata/lxc-hostdev.xml index 61e8655..3972594 100644 --- a/tests/lxcxml2xmldata/lxc-hostdev.xml +++ b/tests/lxcxml2xmldata/lxc-hostdev.xml @@ -37,8 +37,8 @@ </source> <ip address='192.168.122.2' family='ipv4'/> <ip address='2003:db8:1:0:214:1234:fe0b:3596' family='ipv6' prefix='24'/> - <route family='ipv4' via='192.168.122.1'/> - <route family='ipv6' via='2003:db8:1:0:214:1234:fe0b:3595'/> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/> + <route family='ipv6' address='::' gateway='2003:db8:1:0:214:1234:fe0b:3595'/> </hostdev> </devices> </domain> diff --git a/tests/lxcxml2xmldata/lxc-idmap.xml b/tests/lxcxml2xmldata/lxc-idmap.xml index 2b04a65..b477636 100644 --- a/tests/lxcxml2xmldata/lxc-idmap.xml +++ b/tests/lxcxml2xmldata/lxc-idmap.xml @@ -30,8 +30,8 @@ <source bridge='bri0'/> <ip address='192.168.122.12' family='ipv4' prefix='24'/> <ip address='192.168.122.13' family='ipv4' prefix='24'/> - <route family='ipv4' via='192.168.122.1'/> - <route family='ipv4' via='192.168.124.1' address='192.168.124.0' prefix='24'/> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/> + <route family='ipv4' address='192.168.124.0' prefix='24' gateway='192.168.124.1'/> <target dev='veth0'/> <guest dev='eth2'/> </interface>
ACK.
If 0.0.0.0 address is provided, then the returned prefix should be 0, rather than 8. --- src/util/virsocketaddr.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index c5584af..e7870bf 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -832,6 +832,12 @@ virSocketAddrGetIpPrefix(const virSocketAddr *address, */ unsigned char octet = ntohl(address->data.inet4.sin_addr.s_addr) >> 24; + + /* If address is 0.0.0.0, we surely want to have 0 prefix for + * the default route. */ + if (address->data.inet4.sin_addr.s_addr == 0) + return 0; + if ((octet & 0x80) == 0) { /* Class A network */ return 8; -- 2.1.2
On 01/15/2015 04:25 AM, Cédric Bosdonnat wrote:
If 0.0.0.0 address is provided, then the returned prefix should be 0, rather than 8. --- src/util/virsocketaddr.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index c5584af..e7870bf 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -832,6 +832,12 @@ virSocketAddrGetIpPrefix(const virSocketAddr *address, */ unsigned char octet = ntohl(address->data.inet4.sin_addr.s_addr) >> 24; + + /* If address is 0.0.0.0, we surely want to have 0 prefix for + * the default route. */ + if (address->data.inet4.sin_addr.s_addr == 0) + return 0; + if ((octet & 0x80) == 0) { /* Class A network */ return 8;
Aha! Now I see that my comments in 3/7 were a bit unneeded. ACK to this, but do we maybe need it also for IPv6 addresses that are 0?
On 01/15/2015 04:25 AM, Cédric Bosdonnat wrote:
If 0.0.0.0 address is provided, then the returned prefix should be 0, rather than 8. --- src/util/virsocketaddr.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index c5584af..e7870bf 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -832,6 +832,12 @@ virSocketAddrGetIpPrefix(const virSocketAddr *address, */ unsigned char octet = ntohl(address->data.inet4.sin_addr.s_addr) >> 24; + + /* If address is 0.0.0.0, we surely want to have 0 prefix for + * the default route. */ + if (address->data.inet4.sin_addr.s_addr == 0) + return 0; + if ((octet & 0x80) == 0) { /* Class A network */ return 8;
So it seems after Laine's comments on this you added: + if (address->data.inet6.sin6_addr.s6_addr == 0) + return 0; which Coverity flags as: 852 } else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET6)) { (1) Event array_null: Comparing an array to null is not useful: "address->data.inet6.sin6_addr.__in6_u.__u6_addr8 == NULL". 853 if (address->data.inet6.sin6_addr.s6_addr == 0) Since s6_addr is a pointer - is this supposed to be a NULL comparison or perhaps are you looking for a specific character [0] or all characters being 0? John
On Fri, 2015-01-16 at 06:50 -0500, John Ferlan wrote:
On 01/15/2015 04:25 AM, Cédric Bosdonnat wrote:
If 0.0.0.0 address is provided, then the returned prefix should be 0, rather than 8. --- src/util/virsocketaddr.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index c5584af..e7870bf 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -832,6 +832,12 @@ virSocketAddrGetIpPrefix(const virSocketAddr *address, */ unsigned char octet = ntohl(address->data.inet4.sin_addr.s_addr) >> 24; + + /* If address is 0.0.0.0, we surely want to have 0 prefix for + * the default route. */ + if (address->data.inet4.sin_addr.s_addr == 0) + return 0; + if ((octet & 0x80) == 0) { /* Class A network */ return 8;
So it seems after Laine's comments on this you added:
+ if (address->data.inet6.sin6_addr.s6_addr == 0) + return 0;
which Coverity flags as:
852 } else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET6)) {
(1) Event array_null: Comparing an array to null is not useful: "address->data.inet6.sin6_addr.__in6_u.__u6_addr8 == NULL".
853 if (address->data.inet6.sin6_addr.s6_addr == 0)
Since s6_addr is a pointer - is this supposed to be a NULL comparison or perhaps are you looking for a specific character [0] or all characters being 0?
Oops... I was looking for the 0 ipv6 address. I'll fix that now. -- Cedric
From: John Ferlan <jferlan@redhat.com> Commit id 'aa2cc721' added call to virSocketAddrFormat and did not VIR_FREE() the returned memory. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c5f279..d262d72 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17198,6 +17198,7 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) familyStr = "ipv4"; virBufferAsprintf(buf, "<ip address='%s'", ipStr); + VIR_FREE(ipStr); if (familyStr) virBufferAsprintf(buf, " family='%s'", familyStr); if (ips[i]->prefix != 0) -- 2.1.2
On 01/15/2015 04:25 AM, Cédric Bosdonnat wrote:
From: John Ferlan <jferlan@redhat.com>
Commit id 'aa2cc721' added call to virSocketAddrFormat and did not VIR_FREE() the returned memory.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c5f279..d262d72 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17198,6 +17198,7 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) familyStr = "ipv4"; virBufferAsprintf(buf, "<ip address='%s'", ipStr); + VIR_FREE(ipStr); if (familyStr) virBufferAsprintf(buf, " family='%s'", familyStr); if (ips[i]->prefix != 0)
ACK.
From: John Ferlan <jferlan@redhat.com> Commit id 'aa2cc721' added calls to virSocketAddrFormat but did not check for a NULL (error) return which could lead to bad output in the XML file. Need to check for NULL return and cause failure. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d262d72..9dd11f0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17182,7 +17182,7 @@ virDomainFSDefFormat(virBufferPtr buf, return 0; } -static void +static int virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) { size_t i; @@ -17192,6 +17192,9 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) virSocketAddrPtr address = &ips[i]->address; char *ipStr = virSocketAddrFormat(address); const char *familyStr = NULL; + + if (!ipStr) + return -1; if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET6)) familyStr = "ipv6"; else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET)) @@ -17205,6 +17208,7 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) virBufferAsprintf(buf, " prefix='%u'", ips[i]->prefix); virBufferAddLit(buf, "/>\n"); } + return 0; } static int @@ -17373,8 +17377,9 @@ virDomainHostdevDefFormatCaps(virBufferPtr buf, virBufferAddLit(buf, "</source>\n"); if (def->source.caps.type == VIR_DOMAIN_HOSTDEV_CAPS_TYPE_NET) { - virDomainNetIpsFormat(buf, def->source.caps.u.net.ips, - def->source.caps.u.net.nips); + if (virDomainNetIpsFormat(buf, def->source.caps.u.net.ips, + def->source.caps.u.net.nips) < 0) + return -1; if (virDomainNetRoutesFormat(buf, def->source.caps.u.net.routes, def->source.caps.u.net.nroutes) < 0) return -1; @@ -17766,7 +17771,8 @@ virDomainNetDefFormat(virBufferPtr buf, return -1; } - virDomainNetIpsFormat(buf, def->ips, def->nips); + if (virDomainNetIpsFormat(buf, def->ips, def->nips) < 0) + return -1; if (virDomainNetRoutesFormat(buf, def->routes, def->nroutes) < 0) return -1; -- 2.1.2
On 01/15/2015 04:25 AM, Cédric Bosdonnat wrote:
From: John Ferlan <jferlan@redhat.com>
Commit id 'aa2cc721' added calls to virSocketAddrFormat but did not check for a NULL (error) return which could lead to bad output in the XML file. Need to check for NULL return and cause failure.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d262d72..9dd11f0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17182,7 +17182,7 @@ virDomainFSDefFormat(virBufferPtr buf, return 0; }
-static void +static int virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) { size_t i; @@ -17192,6 +17192,9 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) virSocketAddrPtr address = &ips[i]->address; char *ipStr = virSocketAddrFormat(address); const char *familyStr = NULL; + + if (!ipStr) + return -1; if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET6)) familyStr = "ipv6"; else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET)) @@ -17205,6 +17208,7 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) virBufferAsprintf(buf, " prefix='%u'", ips[i]->prefix); virBufferAddLit(buf, "/>\n"); } + return 0; }
static int @@ -17373,8 +17377,9 @@ virDomainHostdevDefFormatCaps(virBufferPtr buf, virBufferAddLit(buf, "</source>\n");
if (def->source.caps.type == VIR_DOMAIN_HOSTDEV_CAPS_TYPE_NET) { - virDomainNetIpsFormat(buf, def->source.caps.u.net.ips, - def->source.caps.u.net.nips); + if (virDomainNetIpsFormat(buf, def->source.caps.u.net.ips, + def->source.caps.u.net.nips) < 0) + return -1; if (virDomainNetRoutesFormat(buf, def->source.caps.u.net.routes, def->source.caps.u.net.nroutes) < 0) return -1; @@ -17766,7 +17771,8 @@ virDomainNetDefFormat(virBufferPtr buf, return -1; }
- virDomainNetIpsFormat(buf, def->ips, def->nips); + if (virDomainNetIpsFormat(buf, def->ips, def->nips) < 0) + return -1; if (virDomainNetRoutesFormat(buf, def->routes, def->nroutes) < 0) return -1;
ACK.
On 15.01.2015 10:25, Cédric Bosdonnat wrote:
Hi guys,
Here are a few patches to have common route definitions for domains and networks. What has changed since v1: * Split into several patches for backportability as suggested by Laine * Moved the virNetworkRouteDef struct definition in the c file to make sure all routes are created with the virNetworkRouteDefCreate (or Parse) function. This way we can get rid of the redundant checks in the format function. * Moved the prefix computing code into a new virNetworkRouteDefGetPrefix function. * Applied and tweaked John's patches. * Fix the virSocketAddrGetIpPrefix 0.0.0.0 special case as suggested by Laine.
Cédric Bosdonnat (5): Fix ipv6 regex in RNG schemas to match '::' Move network route definition to networkcommon.rng Move code related to network routes to networkcommon_conf.[ch] Use the network route definitions for domains virSocketAddrGetIpPrefix 0.0.0.0 special case
John Ferlan (2): domain_conf: Resolve Coverity RESOURCE_LEAK domain_conf: Check errors from virSocketAddrFormat
docs/formatdomain.html.in | 9 +- docs/schemas/basictypes.rng | 2 +- docs/schemas/domaincommon.rng | 29 +- docs/schemas/network.rng | 20 +- docs/schemas/networkcommon.rng | 22 ++ po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/domain_conf.c | 136 ++----- src/conf/domain_conf.h | 14 +- src/conf/network_conf.c | 297 +--------------- src/conf/network_conf.h | 22 +- src/conf/networkcommon_conf.c | 414 ++++++++++++++++++++++ src/conf/networkcommon_conf.h | 72 ++++ src/libvirt_private.syms | 11 + src/lxc/lxc_container.c | 22 +- src/lxc/lxc_native.c | 20 +- src/network/bridge_driver.c | 44 +-- src/util/virsocketaddr.c | 6 + tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml | 4 +- tests/lxcconf2xmldata/lxcconf2xml-simple.xml | 4 +- tests/lxcxml2xmldata/lxc-hostdev.xml | 4 +- tests/lxcxml2xmldata/lxc-idmap.xml | 4 +- 22 files changed, 633 insertions(+), 527 deletions(-) create mode 100644 src/conf/networkcommon_conf.c create mode 100644 src/conf/networkcommon_conf.h
ACK series. Good job! Michal
On Thu, 2015-01-15 at 11:58 +0100, Michal Privoznik wrote:
On 15.01.2015 10:25, Cédric Bosdonnat wrote:
Hi guys,
Here are a few patches to have common route definitions for domains and networks. What has changed since v1: * Split into several patches for backportability as suggested by Laine * Moved the virNetworkRouteDef struct definition in the c file to make sure all routes are created with the virNetworkRouteDefCreate (or Parse) function. This way we can get rid of the redundant checks in the format function. * Moved the prefix computing code into a new virNetworkRouteDefGetPrefix function. * Applied and tweaked John's patches. * Fix the virSocketAddrGetIpPrefix 0.0.0.0 special case as suggested by Laine.
Cédric Bosdonnat (5): Fix ipv6 regex in RNG schemas to match '::' Move network route definition to networkcommon.rng Move code related to network routes to networkcommon_conf.[ch] Use the network route definitions for domains virSocketAddrGetIpPrefix 0.0.0.0 special case
John Ferlan (2): domain_conf: Resolve Coverity RESOURCE_LEAK domain_conf: Check errors from virSocketAddrFormat
docs/formatdomain.html.in | 9 +- docs/schemas/basictypes.rng | 2 +- docs/schemas/domaincommon.rng | 29 +- docs/schemas/network.rng | 20 +- docs/schemas/networkcommon.rng | 22 ++ po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/domain_conf.c | 136 ++----- src/conf/domain_conf.h | 14 +- src/conf/network_conf.c | 297 +--------------- src/conf/network_conf.h | 22 +- src/conf/networkcommon_conf.c | 414 ++++++++++++++++++++++ src/conf/networkcommon_conf.h | 72 ++++ src/libvirt_private.syms | 11 + src/lxc/lxc_container.c | 22 +- src/lxc/lxc_native.c | 20 +- src/network/bridge_driver.c | 44 +-- src/util/virsocketaddr.c | 6 + tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml | 4 +- tests/lxcconf2xmldata/lxcconf2xml-simple.xml | 4 +- tests/lxcxml2xmldata/lxc-hostdev.xml | 4 +- tests/lxcxml2xmldata/lxc-idmap.xml | 4 +- 22 files changed, 633 insertions(+), 527 deletions(-) create mode 100644 src/conf/networkcommon_conf.c create mode 100644 src/conf/networkcommon_conf.h
ACK series. Good job!
Pushed with minor changes from Lain's comments. -- Cedric
participants (5)
-
Cedric Bosdonnat -
Cédric Bosdonnat
-
John Ferlan -
Laine Stump -
Michal Privoznik