[PATCH] tevent_common_context_destructor aims to remove all node from list?

Jeremy Allison jra at samba.org
Sun Jan 11 18:42:56 GMT 2009


On Sat, Jan 10, 2009 at 01:54:41PM +0800, boyang wrote:
> Hi, Metze:
>      Does tevent_common_context_destructor aim to remove all nodes
> instead of just the first one? I think it is your intention to remove
> all instead of the first one. The current code just remove the first
> one, patch tries to fix it. Please review it.
>      Sorry that I have not compile it at present, because I don't have a
> build environment at my hand now. :-)

Arg. I know that patch is correct and metze just committed it,
but I *hate* that :

ptr = list_head
while(ptr) {
	do stuff...
	ptr = ptr->next;
}

idiom. This code was the cause of the bug in the
krb5 creds reinitialization code you back ported
that I missed when reviewing the back port (the

ptr = ptr->next;

part was missing). That's an easy bug to introduce
by mistake.

It's a *for* loop dammit :-). Please re-code as

for (ptr = list_head; ptr; ptr = ptr->next) {
	do stuff..
}

as that makes much more sense (and indeed was
the way it was originally written). The only time
you need to do the while() idiom is if you're
deleting elements of the list and can't use
ptr = ptr->next as the last part of the for
as the ptr element was freed.

Just a small rant, don't take it personally :-) :-).

Cheers,

Jeremy.


More information about the samba-technical mailing list