[asterisk-dev] [asterisk-commits] mattf: trunk r150640 - in /trunk: channels/ configs/

[Matthew Fredrickson]

Matthew Fredrickson wrote:
> Kevin P. Fleming wrote:
>> SVN commits to the Asterisk project wrote:
>>
>>> +#ifdef PRI_SET_CHAN_MAPPING_LOGICAL
>>> +		pri_set_chan_mapping_logical(pri->dchans[i], pri->qsigchannelmapping == DAHDI_CHAN_MAPPING_LOGICAL);
>>> +#endif
>> Please use a configure script check to determine whether this
>> functionality is available or not, rather than adding more #define
>> statements to libpri.h. This should also be done for the
>> discardremotehold feature.
>>
>>>  #ifdef HAVE_PRI_INBANDDISCONNECT
>>> +			} else if (!strcasecmp(v->name, "qsigchannelmapping")) {
>>> +				if (!strcasecmp(v->value, "logical")) {
>>> +					confp->pri.qsigchannelmapping = DAHDI_CHAN_MAPPING_LOGICAL;
>>> +				} else if (!strcasecmp(v->value, "physical")) {
>>> +					confp->pri.qsigchannelmapping = DAHDI_CHAN_MAPPING_PHYSICAL;
>>> +				} else {
>>> +					confp->pri.qsigchannelmapping = DAHDI_CHAN_MAPPING_PHYSICAL;
>>> +				}
>>> +			} else if (!strcasecmp(v->name, "discardremoteholdretrieval")) {
>>> +				confp->pri.discardremoteholdretrieval = ast_true(v->value);
>> This code does not belong inside the HAVE_PRI_INBANDDISCONNECT
>> condition. Also, please report an error with LOG_ERROR if the value
>> supplied for qsigchannelmapping is invalid.
> 
> As long as we're backing these checks out, it would also make sense to 
> remove the old HAVE_PRI_INBANDDISCONNECT check as well...

Sorry, I think I misunderstood your thoughts.  We can certainly add 
additional checks for the new API calls, but (and I'm seeking some 
outside thoughts here), perhaps it might be interesting to think about 
mandating a minimum version of libpri here (1.4.x) for trunk/1.6.x 
version of Asterisk (whatever this version of trunk turns into).

The reason I ask is that some of these changes are going to require not 
just #if's but also doing the #else as well (since there's a new API to 
replace an old one).  To keep spaghettification to a minimum, it would 
be nice to not have to do this.  We could also get rid of a number of 
other #if HAVE_ and #ifdef PRI_... statements as well by requiring the 
minimum version number, leaving only the #if HAVE_ to whatever is the 
most recent API addition to libpri.  Any thoughts?

Matthew Fredrickson
Digium, Inc.