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

[Matt Jordan]

> On May 17, 2012, 1:22 p.m., Neil Tallim 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.


> On May 17, 2012, 1:22 p.m., Neil Tallim wrote:
> > /trunk/apps/app_confbridge.c, lines 726-729
> > <https://reviewboard.asterisk.org/r/1869/diff/1/?file=27305#file27305line726>
> >
> >     While true, the decision only makes sense when the second participant joins. By the time a third has joined, nothing else in the logic of the confbridge app will alter the state in any meaningful way.
> >     
> >     Making it > 1 won't hurt anything, but it will result in unnecessary checks.

That's fine.


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

Regardless of other patches, you've added code that can be improved in this patch.  Please address the finding.


> On May 17, 2012, 1:22 p.m., Neil Tallim wrote:
> > /trunk/apps/app_confbridge.c, lines 732-735
> > <https://reviewboard.asterisk.org/r/1869/diff/1/?file=27305#file27305line732>
> >
> >     I was planning to submit a refactoring patch to do just that once my other patches (now eight in total) were approved or rejected. If I were to make that change now, I fear I might break other pending submissions.

Again, please address the findings in this patch.  Future patches should not dictate the quality of the patch currently under review.


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

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.


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

No, it needs to be cleaned up in this patch.


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

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.


- Matt


-----------------------------------------------------------
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/b856ffea/attachment-0001.htm>