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

[Tilghman Lesher]

On Monday 02 January 2006 19:11, Luigi Rizzo wrote:
> 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.

Then you disagree with me as to what architectural review is.  To me,
that means that I agree with your approach in code.  A code review, on
the other hand, means the syntax and logic is correct.  

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

There are always limits on resources.  Given time resources, I could
fix a whole lot of actual bugs, or I could spend time on one
particular optimization/feature, getting it ready for inclusion.  Bugs
are clearly a priority.

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

The rule, for the present time, is that all patchsets get submitted
through bugs.digium.com.

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

The problem is that I don't know that.  I know that for many cases,
I will not have time to give a patch the attention that it deserves,
so it gets put off for another day.  That seems to be a parallel case
for other committers, as well.  We're all busy people.

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

Reduction of duplicate code doesn't increase maintainability?

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

It's not my intent to play such a card.  I simply have a limited
amount of time per day to work on Asterisk, as do most of the
developers, and patches which require my full uninterrupted attention
for long periods of time simply aren't going to get addressed most of
the time.  By breaking it down into smaller patches, each for a
particular purpose, you lessen the time that I have to spend examining
the patch, compiling, and testing, which means your patch ends up in
the repository more quickly -- that is your goal (and mine).

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

Trunk is for development, but we test patches before we commit, even
to trunk, because a broken trunk is useless.  If I don't have time to
test before I commit, what makes you think I'm going to have time
afterwards?

> > > "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:

If you're proposing a patch, the correct forum is bugs.digium.com.
In any case, if you're proposing putting inline assembly into the
code, you need to talk with Mark first -- that is not a change that
I am comfortable making.  The nice thing about a mutex is that for
any platform in which Asterisk runs, mutexes are portable.  The same
is not true for inline assembly.

-- 
Tilghman