Re: [libvirt] [PATCH 01/13] New virSocketAddr utility functions
On 12/20/2010 05:45 PM, Paweł Krześniak wrote:
On Mon, Dec 20, 2010 at 09:03, Laine Stump<laine(a)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.
Right. Eric also suggested this, and I can't even tell you why I didn't
do it that way to begin with. :-P
> +/**
> + * 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?
No. I had it that way for consistency with prefix return values (which
are ints so we can return -1 for an error), but after your and Eric's
comments, I'm changing them to unsigned int (but leaving the return
values as ints).
v2 is coming up. Thanks for the comments!