On Mon, Jan 20, 2014 at 9:09 PM, George Joseph <[email protected]>wrote:
> This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3136/ > > On January 17th, 2014, 4:46 p.m. MST, *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. :) > > > On January 20th, 2014, 8:40 a.m. MST, *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 ;-). > > On January 20th, 2014, 10:24 a.m. MST, *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. > > On January 20th, 2014, 11:38 a.m. MST, *Scott Griepentrog* wrote: > > 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. > > I think everyone agrees that chan_pjsip is a different animal from chan_sip. > Was there some opposition to removing disallow? Or is the feeling that now > that 12 is released there shouldn't be changes to the config spec? Might be > better to bite the bullet now and change allow/disallow to a single "codecs" > variable. > > Anyway, a fairly easy way to keep pjsip_cli clean would be to move "objset = > ast_sorcery_objectset_create(ast_sip_get_sorcery(),obj);" out of > ast_sip_cli_print_sorcery_objset and have each caller create the objset and > pass it in. This way endpoint could re-write the disallow value before > passing the objset and it would be no more than a 1 or 2 line change in each > of the other objects. The objset is allocated for each call so any changes > made to it just get tossed and the original sorcery object doesn't get > touched. > > > > > - George > > On January 17th, 2014, 3:46 p.m. MST, Scott Griepentrog wrote: > Review request for Asterisk Developers. > By Scott Griepentrog. > > *Updated Jan. 17, 2014, 3:46 p.m.* > *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) > > Testing > > Ran command and checked output. > > Diffs > > - /branches/12/res/res_pjsip/pjsip_cli.c (405882) > > View Diff <https://reviewboard.asterisk.org/r/3136/diff/> > > -- > _____________________________________________________________________ > -- 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 Just my two cents, but I don't have access to reviewboard so it's going on the list; hoping someone will see it. I don't like the disallow = !() syntax - it isn't obvious what's going on, if we keep disallow then this should list all the codecs that are disallowed individually, without it mattering if you specified disallow=all or not, However, codecs= makes sense in config and cli - I just don't like ! in disallow, it's not simple in understanding it, it should be as clear as it can be :) Dan
-- _____________________________________________________________________ -- 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
