[quagga-dev 3112] Re: list loop cleanup/audit

Paul Jakma paul at clubi.ie
Tue Apr 5 15:32:00 BST 2005


On Tue, 5 Apr 2005, Andrew J. Schorr wrote:

> I have a few comments, perhaps repetitive of previous comments, sorry
> about that :-) :

No worries.

> 1. Is the assert in listgetdata really needed?

Yes it is. To ensure the listnode actually contains data, which it 
/always/ should do.

> I would guess that in most cases a SEGV would occur moments later 
> if the pointer is actually NULL.  And if there is a SEGV, we should 
> get a log message indicating the crash with a backtrace.  That is 
> pretty much the same result as an assertion failure, although not 
> quite as user-friendly...

The user might check data for NULL and subvert any such SEGV.

I'd like the assert to be there for a while at least, if you want we 
can play games with zassert so that it's only enabled for non-stable 
builds :).

> 2. It seems that you started using the new macros in hundreds of spots (I
> count 699 occurrences of ALL_LIST_ELEMENTS in the diff),

Yep.

> some of which may be bug fixes.

There should be a few fixes, yes. The reason I'm reposting is 
precisely because Hasso recently debugged an ospf6d segv which was 
due to a delete of listnode inside a list loop - which seems to have 
changed his opinion slightly on the goodness of this proposed patch 
:).

> But others may be changes to working non-buggy code.

Likely. *But* the point is to introduce safe by default list loop 
construct - precisely because the existing construct is (time and 
time again) used dangerously. We hit these 'listnode deleted inside 
list loop' bugs every now and then.

So yes, a lot of those changes are churn to non-buggy code. The point 
however is to convert the existing list-loop constructs to more 
robust constructs.

> This leads to the issue that Hasso has raised: does it make sense 
> to change non-buggy code, thereby decreasing source code 
> compatibility with zebra? I have no strong opinion on this, but we 
> should come to a consensus. I presume that the 325 instances of 
> ALL_LIST_ELEMENTS_RO are not actually fixing bugs; is that safe to 
> say?

Indeed.

But it sends a clear message as to intent, where LIST_LOOP does not. 
If you see a _RO loop call that isnt obviously safe - its a bug. The 
non _RO variant is always safe.

> 3. Regarding the QUAGGA_DEPRECATED stuff: isn't the net result going to
> be a warning message every time a source file is compiled?

No, cause Quagga won't define it. It's there to allow 3rd party code 
to continue to compile - one of hasso's other objections from 
previously.

Actually, it's probably wrong way around, we should have a 
QUAGGA_NO_DEPRECATED_INTERFACES define, and /exclude/ the deprecated 
interfaces if its defined. We can then set it in zebra.h.

> It would be quite useful if the message only occurred when a 
> deprecated macro is actually used, but it's not clear to me that 
> it's so useful to get the message for every single compilation. 
> Is this just a club to enforce removing those old macros 
> eventually?  And this ties in with issue #2: is that really 
> desirable?

Yes :)

If it's right to deprecate those macros, it's right, full stop. And 
if so, we will one day expunge them (we cant accumulate deprecated 
detritus forever). So the warning will give others that may be using 
it fair notice.

> Certainly, if we were starting from scratch, I would vastly prefer 
> your macros to the existing ones, but does it make sense to convert 
> everything at this point?  Or just to convert as needed to fix 
> bugs?

Hmmm.. enjoy spending time each time debugging 'listnode deleted that 
list loop is going to reference' bugs. :)

Personally, I think squashing bugs is far more important than trying 
to stay as compatible as possible for ease of merging ever more 
hypothethical GNU Zebra changes..

> Regards,
> Andy

regards,
-- 
Paul Jakma	paul at clubi.ie	paul at jakma.org	Key ID: 64A2FF6A
Fortune:
It's amazing how much "mature wisdom" resembles being too tired.



More information about the Quagga-dev mailing list