[libvirt] [PATCH 2/2] network: Add another collision check into networkCheckRouteCollision
Martin Kletzander
mkletzan at redhat.com
Tue Jul 14 07:38:29 UTC 2015
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 at 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150714/21544da5/attachment-0001.sig>
More information about the libvir-list
mailing list