[asterisk-dev] jpeeler: trunk r131868 - /trunk/channels/chan_dahdi.c

[Jeff Peeler]

----- "Russell Bryant" <russell at digium.com> wrote:

> On Jul 17, 2008, at 5:40 PM, SVN commits to the Digium repositories  
> wrote:
> 
> > Author: jpeeler
> > Date: Thu Jul 17 17:40:00 2008
> > New Revision: 131868
> >
> > URL: http://svn.digium.com/view/asterisk?view=rev&rev=131868
> > Log:
> > Add configuration option to chan_dahdi.conf to allow buffering  
> > policy and number of buffers to be configured per channel. Syntax:
> >
> > buffers=<num of buffers>,<policy>
> >
> > Where the number of buffers is some non-negative integer and the  
> > policy is either "full", "half", or "immediate".
> >
> > Modified:
> >    trunk/channels/chan_dahdi.c
> 
> <snip>
> 
> > @@ -13784,6 +13789,19 @@
> >  			iscrv = !strcasecmp(v->name, "crv");
> >  			if (build_channels(confp, iscrv, v->value, reload, v->lineno,  
> > &found_pseudo))
> >  					return -1;
> > +		} else if (!strcasecmp(v->name, "buffers")) {
> > +			char policy[8];
> > +			tempstr = v->value;
> > +			sscanf(tempstr, "%d,%s", &confp->chan.buf_no, policy);
> > +			if (confp->chan.buf_no < 0)
> > +				confp->chan.buf_no = numbufs;
> > +			if (!strcasecmp(policy, "full")) {
> > +				confp->chan.buf_policy = DAHDI_POLICY_WHEN_FULL;
> > +			} else if (!strcasecmp(policy, "half")) {
> > +				confp->chan.buf_policy = DAHDI_POLICY_IMMEDIATE /*HALF_FULL*/;
> > +			} else {
> > +				confp->chan.buf_policy = DAHDI_POLICY_IMMEDIATE;
> > +			}
> >  		} else if (!strcasecmp(v->name, "dahdichan")) {
> >  			ast_copy_string(dahdichan, v->value, sizeof(dahdichan));
> > 		} else if (!strcasecmp(v->name, "usedistinctiveringdetection")) {
> 
> I have a few suggestions for this option parsing:
> 
> 1) I would initialize the policy buffer to "".  If sscanf doesn't  
> actually write anything into it, then you're accessing an  
> uninitialized buffer with strcasecmp below.
> 
> 2) You should check the return value from sscanf for bogus input.   
> That way, if someone typos the option value, you can write a message 
> 
> to the Asterisk logger about it, and also leave the options at their 
> 
> default value.
> 
> 3) When checking the policy string, I would check for "immediate"  
> explicitly.  Again, this will allow you to alert the user if they  
> typoed the option as "hlaf", instead of "half", for example.
> 
> 4) Also, why is tempstr needed?  I know it was used in a couple of  
> other places, but it seems like a NoOp ...
> 

These are all valid criticisms and I'll fix them. I just noticed the "jitterbuffers" parameter that seems to already set the number of buffers. Should it be removed or should I just modify the newly added option to just be "bufferpolicy"? Most all the error checking then would no longer be necessary, but then again the options are closely related.

Jeff