[Asterisk-Dev] Race issue in channel.c involving uniqueint on Asterisk 1.2.1

[Luigi Rizzo]

On Mon, Jan 02, 2006 at 06:09:34PM -0600, Tilghman Lesher wrote:
...
> I commit the patches I'm comfortable with.  Usually mondo patches that
> consist of 100k worth of code are impossible to work with, because every

i am not talking of committing them - i am talking of giving
architectural review, which does not require to look at every single
line of the patch (which sometimes is there just as a detailed proof of
concept) but to read the description, ask questions to the submitter if
something is not clear, and express an opinion.

If resources to even review patches are limited, i fully understand
that -- we all have limited time, resources, and different interests.
But then, saying "patches are welcome" doesn't reflect reality, or
at least, only trivial patches are welcome, because for larger ones
the committers won't have the time or interest to deal with them.

Which - i repeat - is perfectly fine as long as it is acknowledged;
trying to hide this fact of life behind request to the submitters
to do more work, in my opinion, only serves to annoy the submitter.
On the contrary, making the "rules of the game" clear would
would help me to gain better insight on which things i should submit
to bugs.asterisk.com, and which ones i should just keep for myself
or share through different channels.

1: Note, i don't particularly like the idea of independent patchsets,
but in the end this is what made 386bsd become FreeBSD,
this is how OpenBSD was born, etc.. These things do happen.

> committer is nervous about patches that large.  And I _have_ committed
> several of your patches.  If you'd like to reply with a list of your
> patches, I'll happily submit a reason for each for why I have not
> committed each one.
>  
> Or, if you like, I certainly could close every patch of yours that I'm
> not comfortable applying, but I think you'd prefer that they remain
> open, so they can be eventually reviewed and applied (or rejected, for a
> little better reason than "I don't have time to review this patch right
> now.")

see above.  If you know that none of the committers will deal with
something, then closing it will save everybody time.

> applied is to break up each one into several smaller patches, to the
> tune of "this patch makes this one proposed revision."  For example,
> your patch in 5675 does several different things, radically changing
> the structure of say.c, as well as simplifying several legacy functions.
> If you were to split out the radical change to the structure from the
> simplification of several legacy functions, I could certainly see
> getting the second half of that patch committed.

yes, except that i think the second part gives us very little
in term of maintainability of the code, and so i see it as a waste
of time, expecially having already done the full work.

Anyways i know that you can play the card
"ok but since i am a committer and you are not, I decide".  goto 1; /* :) */

> Smaller patches are preferred, because when there's a bug, we start
> by tracking down which revision stopped working correctly and
> examine the code from there.  If a huge revision is committed, we
> have to look through a lot of code if and when there's a bug (and
> statistically, the larger the patch, the more likely it is that there's
> a bug).

this is a very sane policy, but one that is uses for a 'stable' branch.

There needs to be a development branch, or non-trivial
features and improvements never get in.

> > "i think luigi is wrong because ...." would be a much better
> > argument; even better would be "i think luigi is right", but one
> > can't always be right :)
> 
> I'm not interested enough to do it myself (read: build a patch, apply, 
> and test it), but if someone feels the need to do it, we'll certainly
> consider it.
> 
> However, I'll note that for the issue about which this thread was
> originally posted, these functions do not provide the necessary
> functionality of atomicity.  There would STILL be a race condition,

did you read the thread to the end ? imran pointed out how the
patch was buggy, and in a followup message i gave what i think is
a correct fix, which i repeat here:


	In fact in these cases one should use something like

		call_id = atomic_fetchadd_int(&uniqueid, 1);

	and then call_id is the unique value.
	On i386 and amd64 (from freebsd machine/atomic.h) we have

	/*
	 * Atomically add the value of v to the integer pointed to by p and return
	 * the previous value of *p.
	 */
	static __inline u_int
	atomic_fetchadd_int(volatile u_int *p, u_int v)
	{

		__asm __volatile (
		"       lock                    "
		"       xaddl   %0, %1 ;        "
		"# atomic_fetchadd_int"
		: "+r" (v),                     /* 0 (result) */   
		  "=m" (*p)                     /* 1 */
		: "m" (*p));                    /* 2 */

		return (v);
	}


> even if you used one of these functions for this situation.  You need to
> provide at least a few code examples of where this functionality would
> be helpful.

all the usecount management functions are extremely expensive because
of the mutex_lock/unlock around them which could be avoided by
using the atomic_add_int() or variants.

	cheers (and good night, given my timezone)
	luigi