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

[Russell Bryant]

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

Thanks,

--
Russell Bryant
Senior Software Engineer
Open Source Team Lead
Digium, Inc.