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

[Matthew Fredrickson]

Tzafrir Cohen wrote:
> On Mon, Oct 20, 2008 at 12:23:18PM -0500, Matthew Fredrickson wrote:
>> 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?
> 
> This is a build time test. But at run time the libpri in question might
> not have the function in question.

That's a whole class of other problems that would need to be 
addressed.... (doing library version numbering properly, etc)

Matthew Fredrickson
Digium, Inc.