[libvirt] [PATCH 01/13] New virSocketAddr utility functions

[Paweł Krześniak]

On Mon, Dec 20, 2010 at 09:03, Laine Stump <laine at laine.org> wrote:
> diff --git a/src/util/network.c b/src/util/network.c
> index 1abe78b..e4791b9 100644
> --- a/src/util/network.c
> +++ b/src/util/network.c
> @@ -288,6 +288,73 @@ int virSocketAddrIsNetmask(virSocketAddrPtr netmask) {
>  }
>
>  /**
> + * virSocketAddrMask:
> + * @addr: address that needs to be masked
> + * @netmask: the netmask address
> + *
> + * Mask off the host bits of @addr according to @netmask, turning it
> + * into a network address.
> + *
> + * Returns 0 in case of success, or -1 on error.
> + */
> +int
> +virSocketAddrMask(virSocketAddrPtr addr, const virSocketAddrPtr netmask)
> +{
> +    if (addr->data.stor.ss_family != netmask->data.stor.ss_family)
> +        return -1;
> +
> +    if (addr->data.stor.ss_family == AF_INET) {
> +        addr->data.inet4.sin_addr.s_addr
> +            &= netmask->data.inet4.sin_addr.s_addr;
> +        return 0;
> +    }
> +    if (addr->data.stor.ss_family == AF_INET6) {
> +        int ii;
> +        for (ii = 0; ii < 4; ii++)
> +            addr->data.inet6.sin6_addr.s6_addr32[ii]
> +                &= netmask->data.inet6.sin6_addr.s6_addr32[ii];
> +        return 0;
> +    }
> +    return -1;
> +}
> +
> +/**
> + * virSocketAddrMaskByPrefix:
> + * @addr: address that needs to be masked
> + * @prefix: prefix (# of 1 bits) of netmask to apply
> + *
> + * Mask off the host bits of @addr according to @prefix, turning it
> + * into a network address.
> + *
> + * Returns 0 in case of success, or -1 on error.
> + */
> +int
> +virSocketAddrMaskByPrefix(virSocketAddrPtr addr, int prefix)
> +{
> +    virSocketAddr netmask;
> +
> +    if (virSocketAddrPrefixToNetmask(prefix, &netmask,
> +                                     addr->data.stor.ss_family) < 0)
> +        return -1;

why not replace following:

> +    if (addr->data.stor.ss_family == AF_INET) {
> +
> +        addr->data.inet4.sin_addr.s_addr
> +            &= netmask.data.inet4.sin_addr.s_addr;
> +        return 0;
> +    }
> +    if (addr->data.stor.ss_family == AF_INET6) {
> +        int ii;
> +
> +        for (ii = 0; ii < 4; ii++)
> +            addr->data.inet6.sin6_addr.s6_addr32[ii]
> +                &= netmask.data.inet6.sin6_addr.s6_addr32[ii];
> +        return 0;
> +    }
> +    return -1;
> +}

with simple:
return virSocketAddrMask(addr, netmask);

With cost of checking ss_family for addr and netmask we achieve clearer code.

> +/**
> + * virSocketPrefixToNetmask:
> + * @prefix: number of 1 bits to put in the netmask
> + * @netmask: address to fill in with the desired netmask
> + * @family: family of the address (AF_INET or AF_INET6 only)
> + *
> + * given @prefix and @family, fill in @netmask with a netmask
> + * (eg 255.255.255.0).
> + *
> + * Returns 0 on success or -1 on error.
> + */
> +
> +int virSocketAddrPrefixToNetmask(int prefix,
> +                                 virSocketAddrPtr netmask,
> +                                 int family)
> +{
> +    int result = -1;
> +
> +    netmask->data.stor.ss_family = AF_UNSPEC; /* assume failure */

You don't check if (prefix < 0) for AF_INET6. So my suggestion is to
check it here:
if (prefix < 0)
  goto error;

> +    if (family == AF_INET) {
> +        int ip = 0;
> +

and remove this check from here:

> +        if (prefix < 0 || prefix > 32)
> +            goto error;
> +

btw: does prefix really needs to be signed int?

-- 
Pawel