<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jan 20, 2014 at 9:09 PM, George Joseph <span dir="ltr"><<a href="mailto:reviewboard@asterisk.org" target="_blank">reviewboard@asterisk.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<div style="font-family:Verdana,Arial,Helvetica,Sans-Serif"><div class="im">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border:1px #c9c399 solid">
<tbody><tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3136/" target="_blank">https://reviewboard.asterisk.org/r/3136/</a>
</td>
</tr>
</tbody></table>
<br>
</div><blockquote style="margin-left:1em;border-left:2px solid #d0d0d0;padding-left:10px"><div class="im">
<p style="margin-top:0">On January 17th, 2014, 4:46 p.m. MST, <b>George Joseph</b> wrote:</p>
<blockquote style="margin-left:1em;border-left:2px solid #d0d0d0;padding-left:10px">
<pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">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. :)
</pre>
</blockquote>
</div><div class="im"><p>On January 20th, 2014, 8:40 a.m. MST, <b>Scott Griepentrog</b> wrote:</p>
<blockquote style="margin-left:1em;border-left:2px solid #d0d0d0;padding-left:10px">
<pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">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 ;-).
</pre>
</blockquote>
</div><div class="im"><p>On January 20th, 2014, 10:24 a.m. MST, <b>George Joseph</b> wrote:</p>
<blockquote style="margin-left:1em;border-left:2px solid #d0d0d0;padding-left:10px">
<pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">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.
</pre>
</blockquote>
</div><div class="im"><p>On January 20th, 2014, 11:38 a.m. MST, <b>Scott Griepentrog</b> wrote:</p>
<blockquote style="margin-left:1em;border-left:2px solid #d0d0d0;padding-left:10px">
<pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">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. </pre>
</blockquote>
</div></blockquote>
<pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">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.
</pre>
<br>
<p>- George</p><div class="im">
<br>
<p>On January 17th, 2014, 3:46 p.m. MST, Scott Griepentrog wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image:url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png');background-repeat:repeat-x;border:1px black solid">
<tbody><tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Scott Griepentrog.</div>
<p style="color:grey"><i>Updated Jan. 17, 2014, 3:46 p.m.</i></p>
<div style="margin-top:1.5em">
<b style="color:#575012;font-size:10pt;margin-top:1.5em">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-23092" target="_blank">ASTERISK-23092</a>
</div>
<div style="margin-top:1.5em">
<b style="color:#575012;font-size:10pt">Repository: </b>
Asterisk
</div>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border:1px solid #b8b5a0">
<tbody><tr>
<td>
<pre style="margin:0;padding:0;white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">Insert a ! prefix in the display of endpoint disallow value. Result is:
disallow : !(ulaw|alaw)
</pre>
</td>
</tr>
</tbody></table>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border:1px solid #b8b5a0">
<tbody><tr>
<td>
<pre style="margin:0;padding:0;white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">Ran command and checked output.</pre>
</td>
</tr>
</tbody></table>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Diffs </h1>
<ul style="margin-left:3em;padding-left:0">
<li>/branches/12/res/res_pjsip/pjsip_cli.c <span style="color:grey">(405882)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3136/diff/" style="margin-left:3em" target="_blank">View Diff</a></p>
</td>
</tr>
</tbody></table>
</div></div>
</div>
<br>--<br>
_____________________________________________________________________<br>
-- Bandwidth and Colocation Provided by <a href="http://www.api-digital.com" target="_blank">http://www.api-digital.com</a> --<br>
<br>
asterisk-dev mailing list<br>
To UNSUBSCRIBE or update options visit:<br>
<a href="http://lists.digium.com/mailman/listinfo/asterisk-dev" target="_blank">http://lists.digium.com/mailman/listinfo/asterisk-dev</a></blockquote><div><br></div><div>Just my two cents, but I don't have access to reviewboard so it's going on the list; hoping someone will see it.</div>
<div><br></div><div>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,</div>
<div><br></div><div>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 :)</div><div><br></div><div>Dan </div></div>
<br></div></div>