Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Quagga: Dev

q about prefix_same

 

 

Quagga dev RSS feed   Index | Next | Previous | View Threaded


sowmini.varadhan at sun

Jan 12, 2004, 11:21 AM

Post #1 of 16 (923 views)
Permalink
q about prefix_same

I have a question about the prefix_same function which has the code:

int
prefix_same (struct prefix *p1, struct prefix *p2)
{
if (p1->family == p2->family && p1->prefixlen == p2->prefixlen)
{
if (p1->family == AF_INET)
if (IPV4_ADDR_SAME (&p1->u.prefix, &p2->u.prefix))
return 1;
#ifdef HAVE_IPV6
if (p1->family == AF_INET6 )
if (IPV6_ADDR_SAME (&p1->u.prefix, &p2->u.prefix))
return 1;
#endif /* HAVE_IPV6 */
}
return 0;
}

It seems like this would return 0 if p1 was set (appropriately)
to 10.10.10.20/24, p2 was set to 10.10.10.30/24, even though
the 2 are really the same /24 prefix. This does not sound right.

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

--Sowmini


gdt at ir

Jan 12, 2004, 6:04 PM

Post #2 of 16 (916 views)
Permalink
Re: q about prefix_same [In reply to]

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 [at] ir>


gilad.arnold at terayon

Jan 13, 2004, 1:56 AM

Post #3 of 16 (915 views)
Permalink
Re: q about prefix_same [In reply to]

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


Vincent.Jardin at 6wind

Jan 13, 2004, 2:42 AM

Post #4 of 16 (917 views)
Permalink
Re: q about prefix_same [In reply to]

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


gdt at ir

Jan 13, 2004, 5:44 AM

Post #5 of 16 (913 views)
Permalink
Re: q about prefix_same [In reply to]

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.


gilad.arnold at terayon

Jan 13, 2004, 6:10 AM

Post #6 of 16 (913 views)
Permalink
Re: q about prefix_same [In reply to]

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


gdt at ir

Jan 13, 2004, 6:23 AM

Post #7 of 16 (910 views)
Permalink
Re: q about prefix_same [In reply to]

> 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)
{


sowmini.varadhan at Sun

Jan 13, 2004, 6:52 AM

Post #8 of 16 (911 views)
Permalink
Re: q about prefix_same [In reply to]

> 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


gilad.arnold at terayon

Jan 13, 2004, 7:46 AM

Post #9 of 16 (909 views)
Permalink
Re: q about prefix_same [In reply to]

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


gdt at ir

Jan 13, 2004, 7:51 AM

Post #10 of 16 (916 views)
Permalink
Re: q about prefix_same [In reply to]

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.


gilad.arnold at terayon

Jan 13, 2004, 7:52 AM

Post #11 of 16 (913 views)
Permalink
Re: q about prefix_same [In reply to]

sowmini.varadhan [at] Sun 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


gilad.arnold at terayon

Jan 13, 2004, 7:54 AM

Post #12 of 16 (913 views)
Permalink
Re: q about prefix_same [In reply to]

Greg Troxel wrote:

> Better?

Best of breed! ;->

Gilad


gdt at ir

Jan 13, 2004, 7:58 AM

Post #13 of 16 (913 views)
Permalink
Re: q about prefix_same [In reply to]

Thanks for the review - comments committed.


paul at clubi

Jan 14, 2004, 2:37 PM

Post #14 of 16 (913 views)
Permalink
Re: q about prefix_same [In reply to]

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 [at] clubi paul [at] jakma Key ID: 64A2FF6A
warning: do not ever send email to spam [at] dishone
Fortune:
Why do we want intelligent terminals when there are so many stupid users?


gdt at ir

Jan 14, 2004, 5:27 PM

Post #15 of 16 (918 views)
Permalink
Re: q about prefix_same [In reply to]

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 [at] clubi>

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.


paul at clubi

Jan 14, 2004, 6:47 PM

Post #16 of 16 (911 views)
Permalink
Re: q about prefix_same [In reply to]

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 [at] clubi paul [at] jakma Key ID: 64A2FF6A
warning: do not ever send email to spam [at] dishone
Fortune:
We can predict everything, except the future.

Quagga dev RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.