[asterisk-dev] How to get peer review for patch to deprecated module (was: Re: [asterisk-users] Help debugging a possible SIP channel leak in 11.17.0, possible race condition)

[Matthew Jordan]

On Sat, Apr 11, 2015 at 4:31 PM, Alex Villacís Lasso
<a_villacis at palosanto.com> wrote:
> El 10/04/15 a las 14:16, Alex Villací­s Lasso escribió:
>
> El 08/04/15 a las 08:22, Vinicius Fontes escribió:
>
> Have you tried Asterisk 13? The bridging mechanism has been completely
> rewritten on Asterisk 12, so there's no longer channel masquerading and
> zombie channels. Might be worth a try.
>
> Sorry, this client is very hard to talk into stopping its operations long
> enough to install changes, let alone a major Asterisk version change. I
> already had trouble convincing him of the need to install a debugging
> version with DEBUG_THREADS enabled.
>
> After reviewing a "core show locks" output, I am very sure I have hit an
> Asterisk bug involving a deadlock in cdr_mysql . When doing a "core reload"
> on the CLI, cdr_mysql calls ast_cdr_unregister() with its internal lock held
> (mysql_lock). However, writing a CDR involves adquiring the cdr lock and
> then the internal lock. Therefore, deadlock. None of the other cdr modules
> does that.
>
> However, I know that cdr_mysql is currently deprecated for Asterisk 11. I
> have a patch to fix the issue mentioned above (attached), but I want to know
> how to get it reviewed, if only to see whether the fix is sane.
>

First, regardless of the module support state, if the module exists in
Asterisk, it can be patched. The module support state only reflects
the way in which support of a module is handled; if you write a patch
for a module and would like to contribute it, that's always welcomed.

Second, I suspect you are hitting this bug:

https://issues.asterisk.org/jira/browse/ASTERISK-20347

If you would like to have it re-opened, I'd be happy to do so.

Third, patches via e-mail are typically not viewed (although your
mileage may vary). Part of the CLA stipulates that, when submitting a
patch to Asterisk, you are verifying that you are either the author of
the patch, or have the permission of the authors to contribute the
patch back to Asterisk. Over e-mail, it is difficult to ascertain if
someone has (a) signed a CLA, and (b) wants to contribute a patch back
to Asterisk under the CLA. As such, you'll need to submit the patch
via one of the project mechanisms.

Normally, that would mean attaching the patch to an issue in JIRA
and/or submitting it to Review Board. Alas, this is a bit of an
awkward weekend, as the Asterisk project is in the middle of moving
from Subversion to Git. As a result, things are in a bit of a state of
flux.

I'd recommend doing the following:

* Re-open ASTERISK-20347 and attach the patch to the issue after
signing a CLA [1]. Regardless of the state of the source control for
Asterisk, your patch will at least be tracked in the project. Again,
I'll be happy to re-open the issue once you've confirmed you're
willing to sign a CLA and/or have already done so.
* If you'd like, after signing a CLA, you can submit the patch for
review on Gerrit [2]. Clone the 'asterisk' repo on Gerrit [3], apply
the patch to 'master', and submit it using 'git review'. Draft
instructions for using Gerrit can also be found on the wiki [4].

If you'd like immediate feedback, I'd highly suggest posting the patch
to Gerrit. Code reviews are a normal part of the Asterisk developer
workflow, and we typically look for patches on tools that support
that.

[1] https://issues.asterisk.org/jira/secure/DigiumLicense.jspa
[2] https://gerrit.asterisk.org/#/
[3] https://gerrit.asterisk.org/#/admin/projects/asterisk
[4] https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage

-- 
Matthew Jordan
Digium, Inc. | Director of Technology
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: http://digium.com & http://asterisk.org