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

[Tzafrir Cohen]

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.

-- 
               Tzafrir Cohen
icq#16849755              jabber:tzafrir.cohen at xorcom.com
+972-50-7952406           mailto:tzafrir.cohen at xorcom.com
http://www.xorcom.com  iax:guest at local.xorcom.com/tzafrir