[quagga-dev 3112] Re: list loop cleanup/audit
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 :-) :
> 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
> 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),
> 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
> 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
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
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
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
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..
Paul Jakma paul at clubi.ie paul at jakma.org Key ID: 64A2FF6A
It's amazing how much "mature wisdom" resembles being too tired.
More information about the Quagga-dev