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

[Eric Blake]

On 12/20/2010 01:03 AM, Laine Stump wrote:
> virSocketPrefixToNetmask: Given a 'prefix', which is the number of 1
> bits in a netmask, fill in a virSocketAddr object with a netmask as an
> IP address (IPv6 or IPv4).
> 
> virSocketAddrMask: Mask off the host bits in one virSocketAddr
> according to the netmask in another virSocketAddr.
> 
> virSocketAddrMaskByPrefix, Mask off the host bits in a virSocketAddr
> according to a prefix (number of 1 bits in netmask).
> 
> VIR_SOCKET_FAMILY: return the family of a virSocketAddr

All sound quite useful.

> +    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];

Not portable.  Nothing standardizes the existence of s6_addr32[4], and
not all platforms provide this convenience alias.  Instead, you'll have
to iterate over s6_addr[16] bytes.

> +int
> +virSocketAddrMaskByPrefix(virSocketAddrPtr addr, int prefix)

s/int prefix/unsigned &/

> +{
> +    virSocketAddr netmask;
> +
> +    if (virSocketAddrPrefixToNetmask(prefix, &netmask,
> +                                     addr->data.stor.ss_family) < 0)
> +        return -1;

Avoid code duplication; use virSocketAddrMask to do the masking.

> +int virSocketAddrPrefixToNetmask(int prefix,

For consistency with the rest of the file, put a newline after the
return type and start virSocketAddrPrefixToNetmask in the first column.
 Again, use unsigned int for prefix.

> +    if (family == AF_INET) {
> +        int ip = 0;
> +
> +        if (prefix < 0 || prefix > 32)
> +            goto error;
> +
> +        while (prefix-- > 0) {
> +            ip >>= 1;
> +            ip |= 0x80000000;

Bit-wise loops are slow compared to direct computation.  Why not just:

if (prefix == 0)
    ip = 0;
else
    ip = ~((1 << (32 - prefix)) - 1);

> +    } else if (family == AF_INET6) {
> +        int ii = 0;
> +
> +        if (prefix > 128)

Technically, we could use (CHAR_BIT * sizeof
netmask->data.inet6.sin6_addr.s6_addr) instead of 128, and so forth, but
the magic numbers used in this function aren't too hard to see without
having to hide them behind named constants, so I'm fine with keeping 128.

> +            goto error;

Another argument to make prefix unsigned, since you didn't check for
negative values.

> +
> +        while (prefix >= 8) {
> +            /* do as much as possible an entire byte at a time */
> +            netmask->data.inet6.sin6_addr.s6_addr[ii++] = 0xff;
> +            prefix -= 8;
> +        }

okay.

> +        while (prefix-- > 0) {
> +            /* final partial byte one bit at a time */
> +            netmask->data.inet6.sin6_addr.s6_addr[ii] >>= 1;
> +            netmask->data.inet6.sin6_addr.s6_addr[ii] |= 0x80;
> +        }
> +        ii++;

Given your assumption that you are not starting from an initialized
value, this loop ends up putting garbage in the low half of the byte.
Fix that, and avoid the bitwise loop at the same time, by replacing
those six lines with:

if (prefix > 0) {
    netmask->data.inet6.sin6_addr.s6_addr[ii++] =
        ~((1 << (8 - prefix)) - 1);
}

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101220/0326b4b6/attachment-0001.sig>