> On Jan. 17, 2014, 5:46 p.m., George Joseph wrote: > > Not sure this is a good idea. First, I wouldn't hardcode a test for > > "disallow" in the generic ast_sip_cli_print_sorcery_objset and given the > > meaning of "disallow", the "!" doesn't really make sense. It actually > > negates the disallow. :) > > > > > > Scott Griepentrog wrote: > This change is strictly for visual output of the endpoint object, and > only in the case where it is displayed in response to cli command "pjsip show > endpoint foobar". There is no change to the underlying sorcery code or the > meanings of the elements. > > The purpose is to change this: > > disallow : (ulaw|alaw) > allow : (ulaw|alaw) > > Into this: > > disallow : !(ulaw|alaw) > allow : (ulaw|alaw) > > The issue is that the interpretation of the disallow/allow can be > slightly more confusing by saying we're simultaneously disallowing and > allowing the same codecs, vs. saying that we are disallowing everything > except the listed codecs, and allowing those codecs. > > Since the list of codecs comes from the same data source underneath > sorcery, i.e. the pjsip endpoint element media.caps and/or media.prefs, it is > difficult although not impossible to recreate what was actually spelled out > on the pjsip.conf file for the disallow value. This is because both allow > and disallow get merged together (obviously disallow is inverted) in setting > media.caps/prefs. > > This is a small and easy change, affects CLI output only, avoids a > possible point of confusion, and also duplicates how the function > PJSIP_ENDPOINT() already works. > > My apologies, I should have spelled out the details of this in the > description a little better (was almost 5 on a Friday ;-). > > > George Joseph wrote: > This is going to be very confusing. Right now, allow and disallow both > show the exact same string, which granted is incorrect. Putting a ! in front > of the string for disallow isn't going to make it any less confusing. > > Why not change the pjsip config to match media.caps rather than trying to > perpetuate the allow/disallow construct? One config param named "codecs". > > If the ! really needs to go in, can you put it in pjsip_configuration > where the endpoint code is? I tried hard to refactor pjsip_cli so it doesn't > have any object specific code in it. Also if another object wants to use > "disallow" for something, they're going to get the same ! behavior. >
It would be more accurate to the way that the media.caps/prefs works to just eliminate the disallow mechanism entirely. It would be enough to say allow=!all,ulaw,alaw and then only output allow=(ulaw|alaw), or at least, less confusing. But in the interest of retaining the older allow/disallow convention, I think it's still a good idea to have it for input at least. A possibility would be to eliminate it from output on a sorcery level - as in mark disallow as a write-only value, there purely for the convenience of configuration files but otherwise hidden (which would then remove it from PJSIP_ENDPOINT() as well, maintaining consistency). There are possibly other ways of handling this as well, and while I agree with you that it's desirable to avoid this kind of ugly code, I'm not sure that another solution wouldn't be a bigger problem. But I'm very open to ideas on this. - Scott ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3136/#review10632 ----------------------------------------------------------- On Jan. 17, 2014, 4:46 p.m., Scott Griepentrog wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3136/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2014, 4:46 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-23092 > https://issues.asterisk.org/jira/browse/ASTERISK-23092 > > > Repository: Asterisk > > > Description > ------- > > Insert a ! prefix in the display of endpoint disallow value. Result is: > > disallow : !(ulaw|alaw) > > > Diffs > ----- > > /branches/12/res/res_pjsip/pjsip_cli.c 405882 > > Diff: https://reviewboard.asterisk.org/r/3136/diff/ > > > Testing > ------- > > Ran command and checked output. > > > Thanks, > > Scott Griepentrog > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
