> 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. > > > 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. > > George Joseph wrote: > 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. > > >
Just for the record: I'm to blame for the !(format-list) for disallow. It was convenient for PJSIP_ENDPOINT - but if we can come up with a better (and not too painful) approach to representing the disallowed codecs, I'm all for that. Just for the history of 'why do we still have disallow': The 'disallow/allow' nomenclature of specifying codecs is less a function of the channel drivers than of the core, i.e., ast_parse_allow_disallow. While we could have gone with a different approach - requiring, for example, that you have to specify the allowed codecs and that the disallowed codecs are by default all of them - it really just didn't come up. What's a bit interesting is that sorcery/config_options does know the difference between the "disallow" and the "allow" options - the opt->flags passed into ast_parse_allow_disallow from codec_handler_fn in config_options specifies which is being handled. It's possible that the real problem here is codec_handler_fn in sorcery.c always assuming that what should be parsed out is the ast_codec_pref object on the configuration object. It could look at obj->flags to determine if what is being formatted is an allow or disallow. If an allow, the current code is fine; if a disallow, it could use ast_codec_pref_append_all to append all codecs to a new list, then remove the individual formats using ast_codec_pref_remove. That's kind of a pain, however, as you'd have to walk the preference list yourself and pass the format to ast_codec_pref_remove; it'd probably be easier to just add a new function to format_pref. All of this violates that "not too painful" thought I had earlier however, and I'm not sure that much work is useful for dumping out disallow. Maybe we just skip disallow entirely...? - Matt ----------------------------------------------------------- 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
