10:09 p.m.
Martin Kletzander (2):
conf: Add getter for network routes
network: Add another collision check into networkCheckRouteCollision
src/conf/network_conf.c | 26 ++++++++++++++++++++++++++
src/conf/network_conf.h | 3 +++
src/libvirt_private.syms | 1 +
src/network/bridge_driver_linux.c | 29 +++++++++++++++++++++++++++++
4 files changed, 59 insertions(+)
--
2.4.5
10:09 p.m.
New subject: [libvirt] [PATCH 1/2] conf: Add getter for network routes
Add virNetworkDefGetRouteByIndex() similarly to
virNetworkDefGetIpByIndex(), but for routes.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/conf/network_conf.c | 26 ++++++++++++++++++++++++++
src/conf/network_conf.h | 3 +++
src/libvirt_private.syms | 1 +
3 files changed, 30 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 31d4463a475b..72006e9822d7 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -803,6 +803,32 @@ virNetworkDefGetIpByIndex(const virNetworkDef *def,
return NULL;
}
+/* return routes[index], or NULL if there aren't enough routes */
+virNetworkRouteDefPtr
+virNetworkDefGetRouteByIndex(const virNetworkDef *def,
+ int family, size_t n)
+{
+ size_t i;
+
+ if (!def->routes || n >= def->nroutes)
+ return NULL;
+
+ if (family == AF_UNSPEC)
+ return def->routes[n];
+
+ /* find the nth route of type "family" */
+ for (i = 0; i < def->nroutes; i++) {
+ virSocketAddrPtr addr = virNetworkRouteDefGetAddress(def->routes[i]);
+ if (VIR_SOCKET_ADDR_IS_FAMILY(addr, family)
+ && (n-- <= 0)) {
+ return def->routes[i];
+ }
+ }
+
+ /* failed to find enough of the right family */
+ return NULL;
+}
+
/* return number of 1 bits in netmask for the network's ipAddress,
* or -1 on error
*/
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 9411a02e32cb..1cd5100c1278 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -360,6 +360,9 @@ virPortGroupDefPtr virPortGroupFindByName(virNetworkDefPtr net,
virNetworkIpDefPtr
virNetworkDefGetIpByIndex(const virNetworkDef *def,
int family, size_t n);
+virNetworkRouteDefPtr
+virNetworkDefGetRouteByIndex(const virNetworkDef *def,
+ int family, size_t n);
int virNetworkIpDefPrefix(const virNetworkIpDef *def);
int virNetworkIpDefNetmask(const virNetworkIpDef *def,
virSocketAddrPtr netmask);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1566d11e4156..9d117acbd2bd 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -598,6 +598,7 @@ virNetworkDefFormat;
virNetworkDefFormatBuf;
virNetworkDefFree;
virNetworkDefGetIpByIndex;
+virNetworkDefGetRouteByIndex;
virNetworkDefParseFile;
virNetworkDefParseNode;
virNetworkDefParseString;
--
2.4.5
10:09 p.m.
New subject: [libvirt] [PATCH 2/2] network: Add another collision check into networkCheckRouteCollision
The comment above that function says: "This function can be a lot more
exhaustive, ...", so let's be.
Check for collisions between routes in the system and static routes
being added explicitly from the <route/> element of the network XML.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1094205
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
Laine suggested moving networkCheckRouteCollision() into
networkAddRouteToBridge() and I haven't done that simply because we
can check it where it is now. It would also mean parsing the file,
which we don't want to parse anyway, multiple times or storing the
results and I don't think it's worth neither the time nor space
complexity.
src/network/bridge_driver_linux.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index e394dafb2216..66e5902a7b6f 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -69,6 +69,7 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
char iface[17], dest[128], mask[128];
unsigned int addr_val, mask_val;
virNetworkIpDefPtr ipdef;
+ virNetworkRouteDefPtr routedef;
int num;
size_t i;
@@ -123,6 +124,34 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
goto out;
}
}
+
+ for (i = 0;
+ (routedef = virNetworkDefGetRouteByIndex(def, AF_INET, i));
+ i++) {
+
+ virSocketAddr r_mask, r_addr;
+ virSocketAddrPtr tmp_addr = virNetworkRouteDefGetAddress(routedef);
+ int r_prefix = virNetworkRouteDefGetPrefix(routedef);
+
+ if (!tmp_addr ||
+ virSocketAddrMaskByPrefix(tmp_addr, r_prefix, &r_addr) < 0 ||
+ virSocketAddrPrefixToNetmask(r_prefix, &r_mask, AF_INET) < 0)
+ continue;
+
+ if ((r_addr.data.inet4.sin_addr.s_addr == addr_val) &&
+ (r_mask.data.inet4.sin_addr.s_addr == mask_val)) {
+ char *addr_str = virSocketAddrFormat(&r_addr);
+ if (!addr_str)
+ virResetLastError();
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Route address '%s' collides with one
"
+ "that's in the system already"),
+ NULLSTR(addr_str));
+ VIR_FREE(addr_str);
+ ret = -1;
+ goto out;
+ }
+ }
}
out:
--
2.4.5
3:47 a.m.
New subject: [libvirt] [PATCH 2/2] network: Add another collision check into networkCheckRouteCollision
On 07/09/2015 10:09 AM, Martin Kletzander wrote:
The comment above that function says: "This function can be a
lot more
exhaustive, ...", so let's be.
Check for collisions between routes in the system and static routes
being added explicitly from the <route/> element of the network XML.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1094205
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
Laine suggested moving networkCheckRouteCollision() into
networkAddRouteToBridge() and I haven't done that simply because we
can check it where it is now. It would also mean parsing the file,
which we don't want to parse anyway, multiple times or storing the
results and I don't think it's worth neither the time nor space
complexity.
src/network/bridge_driver_linux.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index e394dafb2216..66e5902a7b6f 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -69,6 +69,7 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
char iface[17], dest[128], mask[128];
unsigned int addr_val, mask_val;
virNetworkIpDefPtr ipdef;
+ virNetworkRouteDefPtr routedef;
int num;
size_t i;
@@ -123,6 +124,34 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
goto out;
}
}
+
+ for (i = 0;
+ (routedef = virNetworkDefGetRouteByIndex(def, AF_INET, i));
+ i++) {
+
+ virSocketAddr r_mask, r_addr;
+ virSocketAddrPtr tmp_addr = virNetworkRouteDefGetAddress(routedef);
+ int r_prefix = virNetworkRouteDefGetPrefix(routedef);
+
+ if (!tmp_addr ||
+ virSocketAddrMaskByPrefix(tmp_addr, r_prefix, &r_addr) < 0 ||
+ virSocketAddrPrefixToNetmask(r_prefix, &r_mask, AF_INET) < 0)
+ continue;
+
+ if ((r_addr.data.inet4.sin_addr.s_addr == addr_val) &&
+ (r_mask.data.inet4.sin_addr.s_addr == mask_val)) {
+ char *addr_str = virSocketAddrFormat(&r_addr);
+ if (!addr_str)
+ virResetLastError();
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Route address '%s' collides with one
"
+ "that's in the system already"),
Could the error message could be adjusted slightly... Such as "Route
address '%s' conflicts with IP address for '%s'" (where I'm
assuming the
second %s is 'iface')... I guess some way to help point at which def is
going to be causing the problem for this def.
I also assume that the error occurs from the bz regardless of order now,
right?
Given the assumptions and noting that I'm not the expert here, both
patches seem fine to me with an adjustment to the error message.
ACK,
John
+ NULLSTR(addr_str));
+ VIR_FREE(addr_str);
+ ret = -1;
+ goto out;
+ }
+ }
}
out:
--
2.4.5
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:38 p.m.
New subject: [libvirt] [PATCH 2/2] network: Add another collision check into networkCheckRouteCollision
On Mon, Jul 13, 2015 at 03:47:29PM -0400, John Ferlan wrote:
On 07/09/2015 10:09 AM, Martin Kletzander wrote:
> The comment above that function says: "This function can be a lot more
> exhaustive, ...", so let's be.
>
> Check for collisions between routes in the system and static routes
> being added explicitly from the <route/> element of the network XML.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1094205
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> Laine suggested moving networkCheckRouteCollision() into
> networkAddRouteToBridge() and I haven't done that simply because we
> can check it where it is now. It would also mean parsing the file,
> which we don't want to parse anyway, multiple times or storing the
> results and I don't think it's worth neither the time nor space
> complexity.
>
> src/network/bridge_driver_linux.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> index e394dafb2216..66e5902a7b6f 100644
> --- a/src/network/bridge_driver_linux.c
> +++ b/src/network/bridge_driver_linux.c
> @@ -69,6 +69,7 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
> char iface[17], dest[128], mask[128];
> unsigned int addr_val, mask_val;
> virNetworkIpDefPtr ipdef;
> + virNetworkRouteDefPtr routedef;
> int num;
> size_t i;
>
> @@ -123,6 +124,34 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
> goto out;
> }
> }
> +
> + for (i = 0;
> + (routedef = virNetworkDefGetRouteByIndex(def, AF_INET, i));
> + i++) {
> +
> + virSocketAddr r_mask, r_addr;
> + virSocketAddrPtr tmp_addr = virNetworkRouteDefGetAddress(routedef);
> + int r_prefix = virNetworkRouteDefGetPrefix(routedef);
> +
> + if (!tmp_addr ||
> + virSocketAddrMaskByPrefix(tmp_addr, r_prefix, &r_addr) < 0
||
> + virSocketAddrPrefixToNetmask(r_prefix, &r_mask, AF_INET) <
0)
> + continue;
> +
> + if ((r_addr.data.inet4.sin_addr.s_addr == addr_val) &&
> + (r_mask.data.inet4.sin_addr.s_addr == mask_val)) {
> + char *addr_str = virSocketAddrFormat(&r_addr);
> + if (!addr_str)
> + virResetLastError();
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Route address '%s' collides with one
"
> + "that's in the system already"),
Could the error message could be adjusted slightly... Such as "Route
address '%s' conflicts with IP address for '%s'" (where I'm
assuming the
second %s is 'iface')... I guess some way to help point at which def is
going to be causing the problem for this def.
I might rather say "route for '%s'" instead of "IP address for
'%s'",
but that's a tiny thing that can be pushed as trivial at any later
point, so I'm going with your wording for now.
I also assume that the error occurs from the bz regardless of order
now,
right?
Yes, exactly.
Given the assumptions and noting that I'm not the expert here,
both
patches seem fine to me with an adjustment to the error message.
ACK,
Thanks, will push in a while.
John
> + NULLSTR(addr_str));
> + VIR_FREE(addr_str);
> + ret = -1;
> + goto out;
> + }
> + }
> }
>
> out:
> --
> 2.4.5
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
3384
days inactive
3389
days old
4 comments
2 participants
participants (2)
-
John Ferlan -
Martin Kletzander