[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