Mailing List Archive

If family is the same, and prefixlen is the same, shouldn't we just
be comparing prefixlen bits, instead of calling IPV4_ADDR_SAME()?

Perhaps, but my preference would be to add a statement that does a
debug print (or maybe an assert while this is tracked down) if the
host bits are non-zero. To me, calling compare on two prefix data
structures when one or more has a non-zero host part is an indication
of a latent error in the code.
Perhaps not, but my inclination would be to see what's up with such
usage before changing the comparison.

--
Greg Troxel <gdt@ir.bbn.com>

Greg Troxel wrote:

> If family is the same, and prefixlen is the same, shouldn't we just
> be comparing prefixlen bits, instead of calling IPV4_ADDR_SAME()?

??

> Perhaps, but my preference would be to add a statement that does a
> debug print (or maybe an assert while this is tracked down) if the
> host bits are non-zero. To me, calling compare on two prefix data
> structures when one or more has a non-zero host part is an indication
> of a latent error in the code.

???

Seems to me that prefix_same() does exactly what the author destined it
to do, which is comparing all elements of two prefixes. To your
comments, I can say that (a) I believe that prefix_cmp() implements the
functionality you were referring to; (b) prefixes with non-zero lsbits
are perfectly valid, as a prefix doesn't necessarily represent a
destination network -- it could well stand for an interface address,
where you'd require the lsbits to be non-zero, isn't it?; and (c) there
are several usages of prefix_same() that rely on it's exact compare
semantics (e.g. see usage in zebra/connected.c/connected_check_ipv4()).

So, what's the point?

Gilad

Gil

>
> Seems to me that prefix_same() does exactly what the author destined
> it to do, which is comparing all elements of two prefixes. To your
> comments, I can say that (a) I believe that prefix_cmp() implements
> the functionality you were referring to; (b) prefixes with non-zero
> lsbits are perfectly valid, as a prefix doesn't necessarily represent
> a destination network -- it could well stand for an interface address,
> where you'd require the lsbits to be non-zero, isn't it?; and (c)
> there are several usages of prefix_same() that rely on it's exact
> compare semantics (e.g. see usage in
> zebra/connected.c/connected_check_ipv4()).
>
> So, what's the point?

I agree. It should not be changed !

Vincent

Please be more careful with your attributions. I did not say this
(but I did quote Sowmini who did say this):

> If family is the same, and prefixlen is the same, shouldn't we just
> be comparing prefixlen bits, instead of calling IPV4_ADDR_SAME()?

I did say:

> Perhaps, but my preference would be to add a statement that does a
> debug print (or maybe an assert while this is tracked down) if the
> host bits are non-zero. To me, calling compare on two prefix data
> structures when one or more has a non-zero host part is an indication
> of a latent error in the code.

My real point (not made entirely well) is that we should understand
what is going on and why before making a semantics change. To me, a
'prefix' should have zero host bits, but a 'address/netmask' might not
(same data structure in practice, but different rules).

So arguably nothing should change in the code, according to what you
report, but it would be nice to have comments that describe the
semantics as used in order to protect against future (incorrect given
the usage) semantics changes.

Greg Troxel wrote:

> Please be more careful with your attributions. I did not say this
> (but I did quote Sowmini who did say this):
>
> > If family is the same, and prefixlen is the same, shouldn't we just
> > be comparing prefixlen bits, instead of calling IPV4_ADDR_SAME()?

Obviously, I was relating to both your posts, it just happened to be in
a single reply (to both)... No offense, please! ;->

(PS: your not using a quote character does make it a bit harder to
follow your replies, or are my eyes getting older?...)


> My real point (not made entirely well) is that we should understand
> what is going on and why before making a semantics change. To me, a
> 'prefix' should have zero host bits, but a 'address/netmask' might not
> (same data structure in practice, but different rules).

Okay, however lib/prefix.c provides calls to manage the data structure
'struct prefix', and not the semantical object "network prefix" (or
whatever you name it). Clearly, one may use various calls, like
lib/prefix.c/apply_mask(), to enforce various semantics on specific
prefixes, but those aren't assumed to be a prerequisite for other calls
in the library.


> So arguably nothing should change in the code, according to what you
> report, but it would be nice to have comments that describe the
> semantics as used in order to protect against future (incorrect given
> the usage) semantics changes.

Sure, comments are important, but the code and a couple of usage
examples -- which couldn't be any clearer than in this case -- are an
immediate evidence to the semantics (at least IMO).

Gilad

> Okay, however lib/prefix.c provides calls to manage the data structure
> 'struct prefix', and not the semantical object "network prefix" (or
> whatever you name it). Clearly, one may use various calls, like
> lib/prefix.c/apply_mask(), to enforce various semantics on specific
> prefixes, but those aren't assumed to be a prerequisite for other calls
> in the library.

Good points.

> Sure, comments are important, but the code and a couple of usage
> examples -- which couldn't be any clearer than in this case -- are an
> immediate evidence to the semantics (at least IMO).

Perhaps, but are the following new comments accurate? If so, I'll
commit them.

Index: lib/prefix.h
===================================================================
RCS file: /var/cvsroot/quagga/lib/prefix.h,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 prefix.h
--- lib/prefix.h 13 Dec 2002 20:15:29 -0000 1.1.1.1
+++ lib/prefix.h 13 Jan 2004 13:21:04 -0000
@@ -23,6 +23,12 @@
#ifndef _ZEBRA_PREFIX_H
#define _ZEBRA_PREFIX_H

+/*
+ * A struct prefix can represent either a true 'network prefix', where
+ * the 'host bits' of the prefix are 0 (e.g. AF_INET:10.0.0.0/8), or
+ * an address and netmask (e.g. AF_INET:10.0.0.9/8).
+ */
+
/* IPv4 and IPv6 unified prefix structure. */
struct prefix
{
Index: lib/prefix.c
===================================================================
RCS file: /var/cvsroot/quagga/lib/prefix.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 prefix.c
--- lib/prefix.c 13 Dec 2002 20:15:29 -0000 1.1.1.1
+++ lib/prefix.c 13 Jan 2004 13:21:04 -0000
@@ -118,7 +118,13 @@
}
}

-/* If both prefix structure is same then return 1 else return 0. */
+/*
+ * Return 1 if the address/netmask contained in the prefix structure
+ * is the same, and else return 0. For this routine, 'same' requires
+ * that not only the prefix length and the network part be the same,
+ * but also the host part. Thus, 10.0.0.1/8 and 10.0.0.2/8 are not
+ * the same. This routine has the same return value sense as '=='.
+ */
int
prefix_same (struct prefix *p1, struct prefix *p2)
{
@@ -136,8 +142,16 @@
return 0;
}

-/* When both prefix structure is not same, but will be same after
- applying mask, return 0. otherwise, return 1 */
+/*
+ * Return 0 if the network prefixes represented by the struct prefix
+ * arguments are the same prefix, and 1 otherwise. Network prefixes
+ * are considered the same if the prefix lengths are equal and the
+ * network parts are the same. Host bits (which are considered masked
+ * by the prefix length) are not significant. Thus, 10.0.0.1/8 and
+ * 10.0.0.2/8 are considered equivalent by this routine. Note that
+ * this routine has the same return sense as strcmp (which is different
+ * from prefix_same).
+ */
int
prefix_cmp (struct prefix *p1, struct prefix *p2)
{

> semantics (e.g. see usage in zebra/connected.c/connected_check_ipv4()).

yes, after posting my question, I saw that. I have some comments on
that too, which I shall post in reply to the ripd related problems.

> So, what's the point?

Good question... is there a reason why prefix_same can't just do
a single memcmp() instead of painfully first comparing family and
len? Is there any padding in that structure?

--Sowmini

Greg Troxel wrote:

> Perhaps, but are the following new comments accurate? If so, I'll
> commit them.

See inline.

> +/*
> + * A struct prefix can represent either a true 'network prefix', where
> + * the 'host bits' of the prefix are 0 (e.g. AF_INET:10.0.0.0/8), or
> + * an address and netmask (e.g. AF_INET:10.0.0.9/8).
> + */

You're assuming apriori knowledge of the term "network prefix", while
'struct prefix' simply stands for a pair of address/mask represented in
some compact way. Given that, you may denote a general-purpose "IPv4/6
prefix" can be used as a "network prefix" (zero lsbits), "address
prefix" (non-zero lsbits), etc. Makes better sense to me, what do you think?


> -/* If both prefix structure is same then return 1 else return 0. */
> +/*
> + * Return 1 if the address/netmask contained in the prefix structure
> + * is the same, and else return 0. For this routine, 'same' requires
> + * that not only the prefix length and the network part be the same,
> + * but also the host part. Thus, 10.0.0.1/8 and 10.0.0.2/8 are not
> + * the same. This routine has the same return value sense as '=='.
> + */

Okay.


> -/* When both prefix structure is not same, but will be same after
> - applying mask, return 0. otherwise, return 1 */
> +/*
> + * Return 0 if the network prefixes represented by the struct prefix
> + * arguments are the same prefix, and 1 otherwise. Network prefixes
> + * are considered the same if the prefix lengths are equal and the
> + * network parts are the same. Host bits (which are considered masked
> + * by the prefix length) are not significant. Thus, 10.0.0.1/8 and
> + * 10.0.0.2/8 are considered equivalent by this routine. Note that
> + * this routine has the same return sense as strcmp (which is different
> + * from prefix_same).
> + */

Fine. Just one suggestion: the two comments should cross-reference each
other, so as to stress the difference.

Gilad

You're assuming apriori knowledge of the term "network prefix", while

well yes, but understanding CIDR prefixes is a prereq for working on
routing daemons.

/*
* A struct prefix contains an address family, a prefix length, and an
* address. This can represent either a 'network prefix' as defined
* by CIDR, where the 'host bits' of the prefix are 0
* (e.g. AF_INET:10.0.0.0/8), or an address and netmask
* (e.g. AF_INET:10.0.0.9/8), such as might be configured on an
* interface.
*/

Better?

I added the second xref pointer to the comparison pair, too; thanks
for the suggestion.

sowmini.varadhan@Sun.COM wrote:

> Good question... is there a reason why prefix_same can't just do
> a single memcmp() instead of painfully first comparing family and
> len? Is there any padding in that structure?

Of course it can't, because (a) you want to be padding independant, and
this isn't a sequential type of data structure; (b) in this case, it's
obvious that the union member (prefix.u...) imposes trailing pad-bytes,
since sizeof(struct in_addr) < sizeof(struct in6_addr). Assuming some
non-virgin IPv6 prefix is assigned an IPv4 address/mask, you can't
guarantee the excess bytes are of any particular value, can you?

(PS: what's wrong with particle comparison anyway?)

Gilad

Greg Troxel wrote:

> Better?

Best of breed! ;->

Gilad

Thanks for the review - comments committed.

On Tue, 13 Jan 2004, Gilad Arnold wrote:

> > + * A struct prefix can represent either a true 'network prefix', where
> > + * the 'host bits' of the prefix are 0 (e.g. AF_INET:10.0.0.0/8), or
> > + * an address and netmask (e.g. AF_INET:10.0.0.9/8).
> > + */

I fail to see the difference. (at least in this context, ie whether
the host bits matter depends entirely on the context and in this
context it does not.)

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Why do we want intelligent terminals when there are so many stupid users?

From lib/prefix.h:

/*
* A struct prefix contains an address family, a prefix length, and an
* address. This can represent either a 'network prefix' as defined
* by CIDR, where the 'host bits' of the prefix are 0
* (e.g. AF_INET:10.0.0.0/8), or an address and netmask
* (e.g. AF_INET:10.0.0.9/8), such as might be configured on an
* interface.
*/

From: Paul Jakma <paul@clubi.ie>

I fail to see the difference. (at least in this context, ie whether
the host bits matter depends entirely on the context and in this
context it does not.)

My point was that there are two semantic objects, and the
representation invariant and the range of the abstraction function
differ.
Essentially, if one is dealing with what should be a CIDR prefix, then
certain values of struct prefix are not allowed. Further, for things
know to be prefixes, certain kinds of comparisons make sense.

For things known to be addresses/masks, the allowable values are
much less constrained, and it makes sense to ask both 'same prefix'
(where host bits are ignored) and 'same address/mask', where they
aren't.

I was really trying to point out that this structure can be used for
two purposes, and that one should not assume either just because the
structure was used.

This is particularly important since the routine prefix_same has an
unfortunate name; it checks whether two address/masks are equal, and
someone could reasonably expect this to behave like prefix_cmp.

On Wed, 14 Jan 2004, Greg Troxel wrote:

> My point was that there are two semantic objects, and the
> representation invariant and the range of the abstraction function
> differ. Essentially, if one is dealing with what should be a CIDR
> prefix, then certain values of struct prefix are not allowed.
> Further, for things know to be prefixes, certain kinds of
> comparisons make sense.

Hmm.. I guess its a way of thinking. To my mind 10.9/8 is a prefix
regardless. whether i care about the host part or not depends on what
i'm thinking of. :)

> For things known to be addresses/masks, the allowable values are
> much less constrained, and it makes sense to ask both 'same prefix'
> (where host bits are ignored) and 'same address/mask', where they
> aren't.

Hmm.. yes, maybe. :)

> This is particularly important since the routine prefix_same has an
> unfortunate name; it checks whether two address/masks are equal,
> and someone could reasonably expect this to behave like prefix_cmp.

Ok yes. Its a good a place to comment it as any.

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
We can predict everything, except the future.