Hello there,
I've just had to subscribe to the quagga dev mailing list to be able
to send you this bug report.
I tried to compile Redhat Fedora package quagga-0.98.4-2 with the
Intel C compiler version 8.1 with the flag -Wall.
The compiler said
1.
ospf_opaque.c(1993): remark #592: variable "ospf" is used before its value
is set
ospf_opaque.c(2072): remark #592: variable "ospf" is used before its value
is set
ospf_vty.c(6031): remark #592: variable "nexthop" is used before its value
is set
Suggest initialise local variables before first use.
2.
ospf6_asbr.c(456): warning #175: subscript out of range
ospf6_asbr.c(759): warning #175: subscript out of range
ospf6_asbr.c(762): warning #175: subscript out of range
ospf6_asbr.c(798): warning #175: subscript out of range
ospf6_asbr.c(803): warning #175: subscript out of range
ospf6_asbr.c(1208): warning #175: subscript out of range
There is a bug in the ZRUTE_NAME macro.
Regards
David Binderman
Use MSN Messenger to send music and pics to your friends
http://messenger.msn.co.uk
Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Sorry to be so slow to follow up on this.

Tue, Jul 26, 2005 at 11:07:08AM +0000, d binderman wrote:
The compiler said

1.

ospf_opaque.c(1993): remark #592: variable "ospf" is used before its value
is set
ospf_opaque.c(2072): remark #592: variable "ospf" is used before its value
is set

Are you referring to this code?

void
(struct ospf_lsa *lsa0)
{
struct ospf *ospf = ospf;

and

void
ospf_opaque_lsa_flush_schedule (struct ospf_lsa *lsa0)
{
struct ospf *ospf = ospf;

I must admit that I cannot see any valid reason for doing
that. I also do not understand why gcc does not issue
a warning.

ospf_vty.c(6031): remark #592: variable "nexthop" is used before its value
is set

Is this referring to this code segment?

DEFUN (,
,
"no default-information originate",
NSTR
"Control distribution of default information\n"
"Distribute a default route\n")
{
struct ospf *ospf = vty->index;
struct prefix_ipv4 p;
struct in_addr nexthop;

p.family = AF_INET;
p.prefix.s_addr = 0;
p.prefixlen = 0;

ospf_external_lsa_flush (ospf, DEFAULT_RUTE, &p, 0, nexthop);

If so, this looks like a bug. the other hand, the
ospf_external_lsa_flush function does not seem to use the value
of the final argument (the only code that referenced it is
commented out). I'm not sure what the proper fix is
here. Given the existing code, it might make sense to drop
the final argument to the function

Suggest initialise local variables before first use.

2.

ospf6_asbr.c(456): warning #175: subscript out of range
ospf6_asbr.c(759): warning #175: subscript out of range
ospf6_asbr.c(762): warning #175: subscript out of range
ospf6_asbr.c(798): warning #175: subscript out of range
ospf6_asbr.c(803): warning #175: subscript out of range
ospf6_asbr.c(1208): warning #175: subscript out of range

There is a bug in the ZRUTE_NAME macro.

Perhaps I'm being dense, but I'm not seeing this.

In lib/zebra.h:

#define ZEBRA_RUTE_MAX 11

In ospf6d/ospf6_asbr.c:

const char *zroute_name[] =
{ "system", "kernel", "connected", "static",
"rip", "ripng", "ospf", "ospf6", "isis", "bgp", "hsls", "unknown" };

#define ZRUTE_NAME(x) \
(0 < (x) && (x) < ZEBRA_RUTE_MAX ? zroute_name[(x)] : \
zroute_name[ZEBRA_RUTE_MAX])

Note that zroute_name[] has 12 entries (subscripts 0 through 11).
So it looks like the subscript is in range to me, although the first
test for '0 < (x)' seems incorrect; shouldn't it be '0 <= (x)'?
But that still doesn't cause a subscript out of range.

Am I missing something?

Possible patch attached. But there don't seem to be any real
bugs here

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Fri, Sep 16, 2005 at 03:10:53PM +0000, d binderman wrote:
>I also do not understand why gcc does not issue a warning.

It probably will, if you turn up the warning level enough -Wall should do
it.

Actually, it doesn't:

sh-3.00$ gcc | head -1
gcc (GCC) 4.0.1 20050727 (Red Hat 4.0.1-5)
sh-3.00$ cat hello.c
#include <stdio.h>

int
main(int argc, char **argv)
{
int *x = x;
puts("Hello, world!");
return 0;
}
sh-3.00$ gcc -Wall hello.c
sh-3.00$

And the same with gcc 3.3.2. No warning at all. I guess the compiler
just treats it as a no-op.

I don't have the code easily available anymore, so I can't persuade you
further.

The macro seemed broken in the version I looked at. The Intel compiler
agreed with me. You might be looking at a later fixed version.

I guess this depends on which version of the code you were looking at.
There was a change on Jan 25, 2005 that may have fixed this problem

Anyway, I have attached a more robust patch to avoid future problems
of that nature.

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Fri, Sep 16, 2005 at 10:20:57AM +0300, Hasso Tepper wrote:
I don't see it either.

It seems to have been a problem in a previous version when "hsls"
was in the process of being added (hsls had been added to lib/zebra.h
but not yet to the array).

It reminds me one thing though. Every time new (routing) protocol is
added, bugs related to this will pop up. Every daemon has own zroute_name
array etc. This should be in lib IMH

Yes, this is a problem. I just sent a patch that would fix the
bug and cause new route types to appear as "unknown", but I agree
that the better approach might be to put it inside the library.
question: should this be packaged as function calls

extern const char *zebra_route_type(u_int);
extern const char *zebra_route_abbrev(u_int);

or as arrays:

extern const char *zebra_route_type[];
extern size_t num_zebra_route_type;
#define ZEBRA_RUTE_TYPE(X) \
((((X) >= 0) && ((X) < num_zebra_route_type)) ? \
zebra_route_type[X] : zebra_route_type[num_zebra_route_type])
extern const char *zebra_route_abbrev[];
extern size_t num_zebra_route_abbrev;
#define ZEBRA_RUTE_ABBREV(X) \
((((X) >= 0) && ((X) < num_zebra_route_abbrev)) ? \
zebra_route_abbrev[X] : zebra_route_abbrev[num_zebra_route_abbrev])

The function interface seems a bit cleaner

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Fri, Sep 16, 2005 at 07:39:12PM +0100, Paul Jakma wrote:
It's damn silly, get rid 'struct ospf *' altogether there. You can
find the correct struct ospf by navigating through struct ospf_lsa
(lsa0->area->ospf).

K, so you're saying that the call to ospf_lookup() can be replaced
by simply using lsa0->area->ospf? Does the attached patch make sense?
It seems a bit confusing to call a function with args
(lsa0->area->ospf, lsa). Are lsa->area->ospf and lsa0->area->ospf
guaranteed to be the same? There seem to be other places in the
code where lsa->area may be NULL (initialization issues?)
and ospf_lookup() is used as a fallback (e.g. register_opaque_info_per_type).
As you can tell, I don't fully understand what's going on with these
data structures, so I'm a bit hesitant about changing this

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Fri, Sep 16, 2005 at 10:20:57AM +0300, Hasso Tepper wrote:
Andrew J. Schorr wrote:
If so, this looks like a bug. the other hand, the
ospf_external_lsa_flush function does not seem to use the value
of the final argument (the only code that referenced it is
commented out). I'm not sure what the proper fix is
here. Given the existing code, it might make sense to drop
the final argument to the function

ACK. It's the old code.

So should the 5th argument be removed? The attached patch comments
it out (since nexthop is still used inside commented-out code
in the function). Does this make sense? should it be removed
entirely?

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Sat, Sep 17, 2005 at 06:20:21PM +0100, Paul Jakma wrote:
Sat, 17 Sep 2005, Andrew J. Schorr wrote:
Those functions are bust, imho. Why would you delete an LSA from an
area if it isn't in that area (impossible obviously)? from the
SPF domain, if it isn't in that SPF domain? Ie both the functions
are redundantly taking struct ospf_area * and struct ospf * arguments
respectively.

K. I guess this should be cleaned up at some point, but my goals are more
modest at the moment: simply to eliminate the brain-damaged code that says
"struct ospf *ospf = ospf;".

>and ospf_lookup() is used as a fallback (e.g.
>register_opaque_info_per_type).

Where's that?

example is in lookup_opaque_info_by_type where it says:

top = ospf_lookup ();
if ((area = lsa->area) != NULL && (top = area->ospf) == NULL)
{
zlog_warn ("Type-11 LSA: Reference to SPF is missing?");

>As you can tell, I don't fully understand what's going on with
>these data structures, so I'm a bit hesitant about changing this

Give it a go. ;)

K, I'll bang it in.

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Fri, Sep 16, 2005 at 07:31:19PM +0100, Paul Jakma wrote:
Fri, 16 Sep 2005, Andrew J. Schorr wrote:

question: should this be packaged as function calls

extern const char *zebra_route_type(u_int);
extern const char *zebra_route_abbrev(u_int);

>The function interface seems a bit cleaner

ACK. Most definitely. :)

K, will do. Does it make sense to put this in lib/log.c,
or should I create a new file lib/zebra.c (since the declarations
clearly belong in lib/zebra.h)? can you think of a better place?

At the moment, I'm thinking:

extern const char *zebra_route_string(u_int route_type);
extern char zebra_route_char(u_int route_type);

But this still leaves some questions. For example, in ospf6_asbr.c,
we have:

{ "X", "K", "C", "S", "R", "R", "", "", "I", "B", "H", "?" };

but in ospf6d/ospf6_zebra.c, we have:

{ "X", "K", "C", "S", "r", "R", "o", "", "I", "B" };

Is there a clear convention on whether rip and ospf should be lower-case
(as opposed to upper-case ripng and ospf6)?

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Sun, Sep 18, 2005 at 12:28:57PM +0300, Hasso Tepper wrote:
Andrew J. Schorr wrote:
But this still leaves some questions. For example, in ospf6_asbr.c,
we have:

{ "X", "K", "C", "S", "R", "R", "", "", "I", "B", "H", "?" };

but in ospf6d/ospf6_zebra.c, we have:

{ "X", "K", "C", "S", "r", "R", "o", "", "I", "B" };

Is there a clear convention on whether rip and ospf should be lower-case
(as opposed to upper-case ripng and ospf6)?

This is the only place where lowercase is used and it really doesn't even
matter here, ospf6d have no chance to see IPv4 routes (yet ;). So, go
uppercase.

K, so does something like the attached patch make sense? Note that
there is some disagreement in the existing code about how to represent
a system route. In , ZEBRA_RUTE_SYSTEM
maps to 'S', but in ripd/ripd.c, ripngd/ripngd.c, ospf6d/ospf6_zebra.c,
and ospf6d/ospf6_asbr.c, it maps to 'X'. Thoughts?

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Mon, Sep 19, 2005 at 10:31:48AM -0400, Andrew J. Schorr wrote:
K, so does something like the attached patch make sense? Note that
there is some disagreement in the existing code about how to represent
a system route. In , ZEBRA_RUTE_SYSTEM
maps to 'S', but in ripd/ripd.c, ripngd/ripngd.c, ospf6d/ospf6_zebra.c,
and ospf6d/ospf6_asbr.c, it maps to 'X'. Thoughts?

Also, there is disagreement on the right strings to use.
In , ripng is shown as "rip",
and ospf6 is shown as "ospf", but in other places "ripng" and "ospf6".
Can we agree on canonical strings and chars for each route type?

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Mon, Sep 19, 2005 at 08:12:04PM +0300, Hasso Tepper wrote:
Andrew J. Schorr wrote:
K, so does something like the attached patch make sense? Note that
there is some disagreement in the existing code about how to represent
a system route. In , ZEBRA_RUTE_SYSTEM
maps to 'S', but in ripd/ripd.c, ripngd/ripngd.c, ospf6d/ospf6_zebra.c,
and ospf6d/ospf6_asbr.c, it maps to 'X'. Thoughts?

Have you seen ZEBRA_RUTE_SYSTEM routes in practice? How? ;) And 'S' is
used for static. If for some mystic reason system routes will appear in
the rib, they will not cause confusion ;).

Good point. Still, one must arbitrarily choose some character for
system routes. Do you prefer 'X' or 'S'?

I am attaching a possible patch. This turns out to be much more
troublesome than expected, as there were many places in the code
that mapped route types to strings or characters. I have tried
to find them all and replace them. But in some cases, messages
will now appear differently (debug messages, output from "show" commands,
etc.). For example, the zebra daemon currently displays rip and
ripng routes with the same string ("rip") (look in
). But with this patch, the
ripng routes will appear as "ripng" in the "Known via %s" messages.
I'm not sure whether this matters. It is arguably an improvement

Also, there is a question of whether we should canonicalize the
reverse mapping from text string to route type. There are a number
of places where this is done (typically in redistribute commands).
So, for example, But these
currently often match just a subset of route types, so I'm not sure
whether this should be changed.

Comments appreciated. This code has NT been tested, but it does compile. :-)

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Sat, Sep 24, 2005 at 08:43:11AM +0300, Hasso Tepper wrote:
Andrew J. Schorr wrote:
Mon, Sep 19, 2005 at 08:12:04PM +0300, Hasso Tepper wrote:
Have you seen ZEBRA_RUTE_SYSTEM routes in practice? How? ;) And 'S' is
used for static. If for some mystic reason system routes will appear in
the rib, they will not cause confusion ;).

Good point. Still, one must arbitrarily choose some character for
system routes. Do you prefer 'X' or 'S'?

I thought that already made my point - as 'S' is used for static, only
'X' can be used for system routes without potential confusion ;).

K, I just committed this patch.

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Fri, Sep 16, 2005 at 12:43:52PM -0400, Andrew J. Schorr wrote:
>I also do not understand why gcc does not issue a warning.
It probably will, if you turn up the warning level enough -Wall should do
it.
Actually, it doesn't:
[ ]
int
main(int argc, char **argv)
{
int *x = x;
puts("Hello, world!");
return 0;
}
sh-3.00$ gcc -Wall hello.c
sh-3.00$

And the same with gcc 3.3.2. No warning at all. I guess the compiler
just treats it as a no-op.

Sorry for following up late but:
Initializing a variable with itself is (or at least was) the GCC way to
_silence_ all ``used unintialized''-warnings for this variable.

regards Christian

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net

Thu, 13, 2005 at 11:30:18AM +0200, Christian Ehrhardt wrote:
Sorry for following up late but:
Initializing a variable with itself is (or at least was) the GCC way to
_silence_ all ``used unintialized''-warnings for this variable.

Interesting. This generally seems like a dangerous practice; I guess
one would only do this to get GCC to shut up when it was failing to
understand the initialization logic. But that was not the case here,
and I have already patched the code.

Regards,
Andy

Quagga-dev mailing list
Quagga-dev (AT) lists (DOT) quagga.net