[libvirt] [PATCH 2/2] network: Add another collision check into networkCheckRouteCollision

[John Ferlan]

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 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>