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

[Tilghman Lesher]

On Monday 02 January 2006 17:25, Luigi Rizzo wrote:
> On Mon, Jan 02, 2006 at 04:57:20PM -0600, Tilghman Lesher wrote:
> > On Monday 02 January 2006 02:31, Dinesh Nair wrote:
> > > On 12/30/05 23:02 Tilghman Lesher said the following:
> > > > On Friday 30 December 2005 07:12, Luigi Rizzo wrote:
> > > >>I think the proper course of action is to wrap the atomic ops
> > > >>into macros, and then let the common header implement the
> > > >>locking in the proper way, with fallback to the above
> > > >>sequence only for unknown architectures.
> > > >>
> > > >>FreeBSD (and i suppose linux as well) has example code
> > > >>for i386 and others in the machine/atomic.[ch] files
> > > >
> > > > This appears to be FreeBSD-specific (or BSD-specific).  There
> > > > are no such macros on Linux.
> > >
> > > but as luigi pointed out, these are architecture specific, so
> > > with linux also being used on the x86 class of processors,
> > > porting these freebsd constructs over shouldnt be too difficult.
> > > or so i think.
> >
> > Patches are welcome on http://bugs.digium.com
>
> This is an architectural decision. If it is not considered
> interesting, patches are useless, a waste of time on my side,
> and clutter on mantis.
>
> And speaking as one who, according to mantis, has submitted 67
> reports to date, 57 of which with patches; and has done the
> 'waste-of-time' exercise several times, with a number of
> architectural proposals, some of which requiring a lot of work, which
> have been sitting there, not even reviewed (e.g.  4377, 4584, 5675)
> and a number not even submitted because interfering with the pending
> ones, you will see that 'patches are welcome' is not exactly a good
> argument here...

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
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.")

However, my suggestion for your mondo patches that aren't getting
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.

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).

> "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,
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.

-- 
Tilghman