Re: [libvirt] [PATCH 01/13] New virSocketAddr utility functions
On 12/20/2010 06:40 PM, Eric Blake wrote:
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.
Fixed.
> +int
> +virSocketAddrMaskByPrefix(virSocketAddrPtr addr, int prefix)
s/int prefix/unsigned&/
Okay, I've changed all of these (except for return values which are
prefix - those will remain int so that -1 can be returned for error).
> +{
> + virSocketAddr netmask;
> +
> + if (virSocketAddrPrefixToNetmask(prefix,&netmask,
> + addr->data.stor.ss_family)< 0)
> + return -1;
Avoid code duplication; use virSocketAddrMask to do the masking.
Right. Not sure why I didn't do that to begin with.
> +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.
Yep. I meant to put the return type on a separate line everywhere, but
this one slipped.
> + 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);
Or
ip = prefix ? ~((1 << (32 - prefix)) - 1) : 0;
:-)
> + } 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);
}
Done.
Thanks for the thorough review. v2 coming up!