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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list