[asterisk-dev] [Code Review]: [confbridge] Behavioural correction for hold-music state when users join/part conferences in varying combinations

Wed Jun 13 15:23:04 CDT 2012

> On May 17, 2012, 1:22 p.m., Neil Tallim wrote:
> >
> 
> Matt Jordan wrote:
>     When replying to a review, please reply to the individual findings, as opposed to creating a new review.  It makes it very difficult to track responses back to findings.

Sorry about that. I don't know how I missed the "Add comment" links, but remember searching for something like that before writing my response, thinking I was doing it wrong. I'm pretty sure I was on a phone or a tablet at the time, but that's an excuse, not a constructive statement.

> On May 17, 2012, 1:22 p.m., Neil Tallim wrote:
> > /trunk/apps/app_confbridge.c, lines 861-862
> > <https://reviewboard.asterisk.org/r/1869/diff/1/?file=27305#file27305line861>
> >
> >     While the Asterisk styleguide does say these should not be mixed, the published confbridge code did it in many places. In keeping with the style of the module, I followed suit. I have no objection to restructuring to be compliant, but another patch will be required to enforce full-file consistency, so I would like to leave that for a refactoring patch to be submitted after the other patches I have pending have been reviewed, to avoid diff-fuzz.
> >     
> >     
> >     As for the number of participants, I disagree: the test needs to be whether we have any other participants who don't require a marked user present in order to speak, or else some people who should be able to converse will be incorrectly set to listening to MoH (and that would cause another problem addressed by the patch I submitted regarding MoH not being consistent with mute).
> >     
> >     That the number is counted instead of just setting a value to 1 like a fake-boolean may be misleading, however, since if even one exists, the condition is satisfied.
> 
> Matt Jordan wrote:
>     The existence of coding style guideline infractions within checked in code does not excuse further infractions.  If you find something that needs to be improved to bring it into compliance, then please do so.
>     
>     The finding here is still valid: if all you're doing is tracking whether or not *any* other participant is in the conference, then you simply need to set nonwaiting_participants to 1 and break.  You don't need to 'count' them all - doing so requires more iterations across the conference bridge users then is necessary.  The fact that the current code 'works' but is inefficient and - because it counts the users as opposed to merely detecting that other users are in the conference - implies conditions that are not true.  Please address the finding.

I apologise for the confusion here.

The current code does not work as expected for the described purpose (hence the logic change), but I meant to close with a statement that agreed with using a boolean-like representation, breaking as soon as the condition was satisfied.

> On May 17, 2012, 1:22 p.m., Neil Tallim wrote:
> > /trunk/apps/app_confbridge.c, lines 727-731
> > <https://reviewboard.asterisk.org/r/1869/diff/1/?file=27305#file27305line727>
> >
> >     They can right now, yes, but another change I was working on in parallel (explicit toggling of MoH via AMI/CLI) requires them to be separate because it introduces another variable that needs to be checked.
> >     
> >     That patch was not combined with this one because it's a feature, but a consistency correction.
> 
> Matt Jordan wrote:
>     Regardless of other patches, you've added code that can be improved in this patch.  Please address the finding.

This would cause the other patch, which I have already submitted, to not apply cleanly against this one, so I'm reluctant to do that unless it is a condition for refusing acceptance. (If it is, then I have no choice, but will probably not have time to rebuild the patches until next week)

I'm used to submitting patches upstream against much smaller projects (or small, highly active sections of a larger project), where they are often accepted or rejected rapidly, allowing me to submit follow-up patches in response to established decisions, rather than trying to predict what will happen.

Given the naivity that this implies on my part, and which I've demonstrated throughout this process, especially since people have already started looking at later submissions, which depend on previous changes, would it be helpful for me to submit more unified patches, collecting all of the changes I've made that have dependencies on each other (I think that would reduce the count from eight to three, each self-contained), and presenting those for review as mutable fragments, even though they span more ideas and intents?

> On May 17, 2012, 1:22 p.m., Neil Tallim wrote:
> > /trunk/apps/app_confbridge.c, lines 1169-1177
> > <https://reviewboard.asterisk.org/r/1869/diff/1/?file=27305#file27305line1169>
> >
> >     1. As with the previous instance, these serve as a hook for another patch that adds a feature.
> >     
> >     2. The value differentiates between users with WAITMARKED and those without. According to the config file and ConfBridge wiki entry by Malcolm Davenport, which I'm considering as official documentation, users without that flag set do NOT need to wait for a marked user to begin speaking. The changes in this patch make it such that, if one such user is in a conference with no eligible speech partners, they hear MoH as a comfort-noise thing, but as soon as someone they can speak to, be it a marked user or another non-WAITMARKED user, joins, they're able to converse freely.
> >     
> >     3. I've personally always found "continue" statements surprising when reading code. I always feel I need to do a double-take to understand why the loop is progressing, hence why they're commented. I have no problem with removing them if you believe the logic leading up to them is clear, however.
> 
> Matt Jordan wrote:
>     1. Do not incorporate future enhancements with a bug fix.
>     
>     2. I'm not sure what you mean by "official documentation" - documentation provides user assistance with the behavior of Asterisk, not the other way around.  That being said, allowing them to speak and starting MOH are two different things.  The wait_marked flag should *only* control whether or not the user is allowed into the conference based on the presence of a marked user in the conference - it shouldn't control MOH behavior or - as you pointed out - their ability to talk into the 'waiting area'.  If its desireable to have MOH be stopped when two or more users are 'waiting' for a conference, regardless of their user profile settings, then that would be a new feature on the bridge profile.
>     
>     3. The comments don't clarify the intent of the code.  Its clear that we are skipping further actions if these conditions are met; that's a very common pattern throughout Asterisk.

2. The implementation of keeping a user "outside" of a conference is currently to play MoH to them so they can't hear the bridge itself, though they can sometimes speak into it (addressed in another patch, currently under review, but facing resistence because it changes perceived behaviour). I do not seek to alter that behaviour, only to ensure that they hear MoH when it is necessary.

I am also not suggesting that, just because there are two users, they should be able to speak to each other, because that would be a major behavioural change.

What is intended here, and what I believe I have implemented with consistency, is that, if a user has wait_marked set, they are forced to remain excluded from the bridge via MoH, just as they are now, until a marked user is present. If two or more users are present who do *not* have wait_marked set, the fact that they are implicitly allowed "inside" the bridge means they should not hear MoH, since that's the only qualifiable difference between the states.

What this does is ensure that, when non-wait_marked participants have nobody to speak with, they will hear MoH, as a comfort-noise factor (this already happens, but has quirks based on unchecked assumptions, addressed elsewhere in this patch), and the music will end whenever they have a qualified peer (another non-wait_marked user or a marked user), not whenever there's another user with whom they cannot communicate (the case now). This is important because doing otherwise gives the misleading impression that a non-wait_marked user is not alone while only wait_marked users are present, when, effectively, they are.

> On May 17, 2012, 1:22 p.m., Neil Tallim wrote:
> > /trunk/apps/app_confbridge.c, lines 862-882
> > <https://reviewboard.asterisk.org/r/1869/diff/1/?file=27305#file27305line862>
> >
> >     This is because I was working across multiple systems and, in some of them, gEdit wasn't set up with consistent tab/space replacement. I probably should have just determined what the file's normal indentation rules were and run astyle over it before generating the diff or something. I'll do that with any future patches.
> 
> Matt Jordan wrote:
>     No, it needs to be cleaned up in this patch.

I meant that I will ensure that any future patches will be cleaned up prior to submission, not that indentation inconsistencies will be ignored in this case. Upon applying other corrections, this will be addressed, too.

- Neil

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1869/#review6244
-----------------------------------------------------------

On May 9, 2012, 4:35 p.m., Neil Tallim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1869/
> -----------------------------------------------------------
> 
> (Updated May 9, 2012, 4:35 p.m.)
> 
> 
> Review request for Asterisk Developers, David Vossel and jcolp.
> 
> 
> Summary
> -------
> 
> This is the "everything" patch attached to issue https://issues.asterisk.org/jira/browse/ASTERISK-19562
> 
> Details about what it does may be found in the issue's description.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_confbridge.c 359977 
> 
> Diff: https://reviewboard.asterisk.org/r/1869/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neil
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120613/a54c438e/attachment-0001.htm>