[quagga-dev 7710] Re: [PATCH] zebra: consider all nexthops when looking fora gateway match

paul at jakma.org paul at jakma.org
Mon Feb 1 10:24:09 GMT 2010


On Mon, 1 Feb 2010, Alexis Rosen wrote:

> I'm completely in sympathy with the desire to clean up the code, 
> but refusing this patch won't make that happen any faster.

> Reviewing it and accepting it would probably take less time than 
> responding in the negative multiple times a year as the issue comes 
> up, which is in fact what's been going on.

Sigh.

There have not been negative responses to that patch. To the contrary 
in fact.

The problems are 2 fold, one relating to the RIB and one relating to 
"How are maintainers to know your patch is 'good'":

1 The RIB:

It's a bit of a mess at present. People poke at it to fix small bugs 
every now and then, but it's clearly much harder to understand than 
it ought to be. Joakim's own patch makes that comment; patches to the 
RIB (including the one that initiated this thread) often go through 
more than 1 revision even when the fix is conceptually simple.

Further, precisely because the code is a mess and hard to understand, 
it becomes hard for maintainers to apply patches. Which brings us 
to...


2. How can we maintainers judge whether a patch is 'good' or not?

We tend to go by:

a) Our ability to understand the fix
b) Hard empirical evidence (i.e. the output of test cases; even
    better: unit tests we can easily run ourselves)
c) Anecdotal evidence from feedback on the list

Now, the last one is an interesting one. Clearly, feedback is from a 
self-selected set - ones who have had the problem. From this, and 
somewhat confirmed by experience, it follows that feedback can not be 
relied upon to spot regressions.

So the regression risk can only really be evaluated using a and b.

-------

The patch in its current form was posted last november afaict (and is 
significantly expanded upon from the patch from last jan), and I 
think there have been a couple of emails since about this problem, 
which this patch might solve. The patch has never been rejected, it 
simply has not yet been integrated (see above).

So, ignoring the cleanup, I don't think this patch has been treated 
unfairly.

Regarding the RIB reform process. It is clearly the correct thing to 
do. The state of the current code is clearly a big obstacle to 
getting bugs fixed and patches integrated. Even if bug fixers do 
eventually manage to mentally untangle the code in their head, that 
knowledge doesn't transfer to reviewers and it doesn't transfer to 
the next time bugs are to be fixed (even by same fixer).

There are some big problems in the RIB that simply can NOT be solved 
without reforming it significantly. E.g. I'd be amazed if anyone gets 
the various recursion bugs fixed (architecturally it'll just be hard 
to do with duplicated nexthop information, I suspect).

Must bug contributors build on the clean-up, no. They can do what 
they want. Equally though, they need to have an appreciation of the 
difficulties of the maintainer to understand whether a patch is 
'good' or not.

Anyway..

regards,
-- 
Paul Jakma	paul at jakma.org	Key ID: 64A2FF6A
Fortune:
I *____knew* I had some reason for not logging you off... If I could just
remember what it was.



More information about the Quagga-dev mailing list