[asterisk-dev] [Code Review]: avoid cppcheck warnings

[Mark Michelson]

> On Jan. 11, 2012, 2:45 a.m., wdoekes wrote:
> > trunk/apps/app_queue.c, lines 4866-4868
> > <https://reviewboard.asterisk.org/r/1651/diff/5/?file=23046#file23046line4866>
> >
> >     You *are* changing functionality:
> >     
> >     if MONITOR_EXEC == NULL but MONITOR_EXEC_ARGS isn't, then.
> >     
> >     Previously: which == qe->chan
> >     Now: which == peer
> >     
> >     Are you sure you want to do that? And if you are, then you should fix the inline if below, because then monexec is not NULL ever anymore.
> >     
> >     (And if you're not, one may need to reconsider the "if (!ast_strlen_zero(monexec)) {" below.. maybe it needs to be "if (!ast_strlen_zero(monexec) || !ast_strlen_zero(monargs)) {".)

Most of this code block doesn't make a lot of sense.

If qe->chan has MONITOR_EXEC set, then it makes good sense that once the call has completed, we should call ast_monitor_setjoinfiles() on qe->chan. With the existing code, if qe->chan does not have MONITOR_EXEC set but does have MONITOR_EXEC_ARGS set, then the result would be that we call ast_monitor_setjoinfiles() on the peer channel. The problem is, when the monitor stops, it's not actually going to use the MONITOR_EXEC or MONITOR_EXEC_ARGS from the qe->chan, it's going to use the MONITOR_EXEC and MONITOR_EXEC_ARGS on the peer channel, which may be different or (more likely) nonexistent.

It doesn't make sense to me that the presence of MONITOR_EXEC or MONITOR_EXEC_ARGS should determine which channel the monitor is applied to. It's overloading the meaning of what MONITOR_EXEC means. All the channel variable is supposed to do is give a command to be run once the monitor stops on the channel.

I suppose the one "nice" thing, and that's being generous, is that none of this behavior is documented anywhere. If we do change behavior, all we would need to do is make a note in CHANGES, and we wouldn't have to worry about being backwards compatible in the case of an upgrade.

In my opinion, the MONITOR_EXEC variable should be the only thing that is checked to determine if ast_monitor_setjoinfiles() should be called. res_monitor can determine what the args are for the command to execute. Now, as far as which channel to apply the monitor to...does it really matter? I mean, the result is the same either way. You get one channel in one recording and the other in another recording. The only difference is which channel is the "in" and which channel is the "out" recording. I think that in most cases, if people are setting MONITOR_EXEC or MONITOR_EXEC_ARGS they are either going to set it only on the incoming channel OR they're going to use __MONITOR_EXEC and __MONITOR_EXEC_ARGS so that the outbound channel has the same value as the incoming channel. So I think it makes the most sense to always monitor the incoming channel. If there's something I'm not taking into account let me know. The only thing I thought of that might make a difference is if a transfer occurs. But as far as I know, monitors get moved over during a masquerade, meaning that the result would still be the same no matter which channel the monitor was initially applied to.


- Mark


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


On Jan. 10, 2012, 8:49 p.m., junky wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1651/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2012, 8:49 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> By running cppcheck 1.52, i realized there was many errors/warnings.
> 
> This patch fixes many of those.
> 
> 
> Diffs
> -----
> 
>   trunk/addons/chan_mobile.c 349670 
>   trunk/addons/chan_ooh323.c 349670 
>   trunk/apps/app_alarmreceiver.c 349670 
>   trunk/apps/app_chanspy.c 349670 
>   trunk/apps/app_disa.c 349670 
>   trunk/apps/app_minivm.c 349670 
>   trunk/apps/app_osplookup.c 349670 
>   trunk/apps/app_queue.c 349670 
>   trunk/apps/app_voicemail.c 349670 
>   trunk/channels/chan_dahdi.c 349670 
>   trunk/channels/chan_iax2.c 349670 
>   trunk/channels/chan_misdn.c 349670 
>   trunk/channels/chan_skinny.c 349670 
>   trunk/channels/chan_usbradio.c 349670 
>   trunk/formats/format_h263.c 349670 
>   trunk/funcs/func_env.c 349670 
>   trunk/funcs/func_odbc.c 349670 
>   trunk/funcs/func_strings.c 349670 
>   trunk/main/acl.c 349670 
>   trunk/main/ast_expr2.c 349670 
>   trunk/main/ast_expr2f.c 349670 
>   trunk/main/pbx.c 349670 
>   trunk/main/udptl.c 349670 
>   trunk/res/res_phoneprov.c 349670 
>   trunk/utils/astman.c 349670 
> 
> Diff: https://reviewboard.asterisk.org/r/1651/diff
> 
> 
> Testing
> -------
> 
> still compile fine.
> Shouldn't have any impact on the code execution.
> 
> 
> Thanks,
> 
> junky
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120111/4eb5d342/attachment.htm>