[asterisk-dev] [Code Review]: avoid cppcheck warnings
junky
reviewboard at asterisk.org
Wed Jan 11 21:53:51 CST 2012
> On Jan. 11, 2012, 2:45 a.m., wdoekes wrote:
> > trunk/apps/app_osplookup.c, lines 1464-1469
> > <https://reviewboard.asterisk.org/r/1651/diff/5/?file=23045#file23045line1464>
> >
> > /me points some more at this code.
its "probably" the normal behavior, since its the same for OSP_CALLID_SIP too.
> On Jan. 11, 2012, 2:45 a.m., wdoekes wrote:
> > trunk/main/ast_expr2f.c, line 2596
> > <https://reviewboard.asterisk.org/r/1651/diff/5/?file=23059#file23059line2596>
> >
> > Since you're touching this line, you may as well fix the indentation (braces, correct spacing around the for-arguments).
fixed, with the comments below also.
> On Jan. 11, 2012, 2:45 a.m., wdoekes wrote:
> > trunk/apps/app_voicemail.c, lines 1952-1961
> > <https://reviewboard.asterisk.org/r/1651/diff/5/?file=23047#file23047line1952>
> >
> > You're changing an error message here:
> >
> > in init_mailstream, we have:
> >
> > if (vms->mailstream == NIL) {
> > return -1;
> > } else {
> > return 0;
> > }
> >
> > NIL happens to be 0 (which should be checked instead, but that's a different matter).
> >
> > (The only other time init_mailstream returns non-zero is when vms_p is NULL, but it isn't.)
> >
> > So now the error is "Unable to open ..." instead of "IMAP mailstream is ..." when !vms_p->mailstream.
> >
> > The rest of the code is inconsistent about checking against !vms_p->mailstream or vms_p->mailstream == NIL. I don't expect you to fix that.
> >
> > You could combine this into a single if, removing the second check, but using the second error.
> >
> > (P.S. A similar pattern of not checking init_mailstream return and only !somevar->mailstream happens later on as well on line 7456.)
>
> Mark Michelson wrote:
> More importantly, you are interpreting the return of init_mailstream incorrectly. It returns 0 on success, but you are treating it as if it is a failure here.
fixed, by combining the 2 conditions together.
> 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)) {".)
>
> Mark Michelson wrote:
> 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.
I restore the initial condition with monexec and monargs to avoid having MONITOR_EXEC, but not MONITOR_EXEC_ARGS.
I intentionally leave the ast_strlen_zero as-is as per Mark said.
In this way, we keep the exact same behavior and make sure everything is backward-compatible.
- junky
-----------------------------------------------------------
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/20120112/ecf7925b/attachment-0001.htm>
More information about the asterisk-dev
mailing list