Re: [asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel

[Russell Bryant]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/
---

(Updated Jan. 18, 2014, 12:58 a.m.)


Review request for Asterisk Developers and leifmadsen.


Changes
---

Addressed Richard's comments.

Thanks for the review!


Repository: Asterisk


Description
---

The ast_filestream object gets tacked on to a channel via
chan->timingdata.  It's a reference counted object, but the reference
count isn't used when putting it on a channel.  It's theoretically
possible for another thread to interfere with the channel while it's
unlocked and cause the filestream to get destroyed.

Use the astobj2 reference count to make sure that as long as this code
path is holding on the ast_filestream and passing it into the file.c
playback code, that it knows it's valid.

-

More detail:

A crash occurs in voicemail.  Here is the backtrace:

#0  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
file.c:779
#1  0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
#2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
channel.c:3865
#3  0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
#4  0x004d5231 in waitstream_core (c=0x7f2644c49460, 
breakon=0x7f262ed56f00 "#*", forward=0x64c4a2 "", reverse=0x64c4a2 "", 
skip_ms=0, audiofd=-1, cmdfd=-1, context=0x0) at file.c:1253
#5  0x004d5783 in ast_waitstream (c=0x7f2644c49460, 
breakon=0x7f262ed56f00 "#*") at file.c:1344
#6  0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460, 
ext=0x7f2609908c20 "", options=0x7f262ed56ff0) at app_voicemail.c:5773
#7  0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 
"") at app_voicemail.c:10722


(gdb)  frame 0
#0  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
file.c:779
779 if (s->owner->timingfd > -1) {
(gdb) p s
$6 = (struct ast_filestream *) 0x7f260856f580

The crash occurs here because the contents of the ast_filestream are garbage.

(gdb) p *s
$7 = {fmt = 0x6372756f530a0d74, flags = 924858981, mode = 875902769, 
open_filename = 0x440a0d3330393734 , 
filename = 0x6974616e69747365 , 
  realfilename = 0x440a0d73203a6e6f , 
vfs = 0x6974616e69747365, trans = 0x7865746e6f436e6f, tr = 0x2d7473786e203a74, 
lastwriteformat = 762475363, lasttimeout = 1969583473, 
  owner = 0x656c6c61430a0d65, f = 0x313722203a444972, fr = {frametype = 
875836727, subclass = {integer = 926687266, codec = 3761973748257529890}, 
datalen = 809056052, samples = 168640051, mallocd = 1851877443, 
mallocd_hdr_len = 7881689877736674080, offset = 762148397, src = 
0xd61633832313030 , data = {ptr = 
0x616e69747365440a, uint32 = 1936016394, pad = "\nDestina"}, delivery = {
  tv_sec = 7953753055737899380, tv_usec = 5785246594218354030}, frame_list 
= {next = 0x36702d5453584e2f}, flags = 758134073, ts = 7010989772577650738, len 
= 7163375912484959347, seqno = 1869182049}, 
  buf = 0x614c0a0d65756575 , _private 
= 0x203a617461447473, orig_chan_name = 0x7273632d7473786e , 
  write_buffer = 0x38312c2c2c2c712d }
(gdb) p s->owner
$8 = (struct ast_channel *) 0x656c6c61430a0d65
(gdb) p *s->owner
Cannot access memory at address 0x656c6c61430a0d65

s->owner should have been 0x7f2644c49460, from higher up in the backtrace.


Here is where things get quite interesting ...


(gdb) frame 2
#2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
channel.c:3865
3865func(data);
(gdb) list
3860/* save a copy of func/data before 
unlocking the channel */
3861int (*func)(const void *) = 
chan->timingfunc;
3862void *data = chan->timingdata;
3863chan->fdno = -1;
3864ast_channel_unlock(chan);
3865func(data);
3866} else {
3867ast_timer_set_rate(chan->timer, 0);
3868chan->fdno = -1;
3869ast_channel_unlock(chan);
(gdb) p chan->timingfunc
$9 = (int (*)(const void *)) 0
(gdb) p chan->timingdata
$10 = (void *) 0x0
(gdb) p func
$11 = (int (*)(const void *)) 0x4d3b6a 
(gdb) p data
$12 = (void *) 0x7f260856f580


This is the code inside of ast_read() that executes the callback 
ast_fsread_audio() from main/file.c to play the next bits of the audio file to 
the channel.  We can see that chan->timingfunc and chan->timingdata have now 
been reset since this code ran.  That probably also means that the 
ast_filestream got destroyed before the code in ast_readaudio_callback was done 
using it.

The only way I can think of this happening is via something in another thread.  
S

Re: [asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel

[Russell Bryant]

> On Jan. 17, 2014, 11:50 p.m., rmudgett wrote:
> > /branches/1.8/main/channel.c, lines 3576-3579
> > 
> >
> > This should not be done at all.
> > 
> > You are dropping a reference when timingdata doesn't really hold the 
> > reference.  In the only case where timingdata is an identified ao2 object, 
> > the timingdata pointer is sharing the reference with c->stream.  The 
> > reference is dropped by ast_closestream() after clearing the timingdata 
> > with its own call to ast_settimeout().  Otherwise, you will need to give 
> > timingdata a reference when setting an identified ao2 object.

I intended to have a corresponding ao2_ref(obj, 1) in ast_settimeout() when it 
gets stored on the channel.  I'll fix that.


- Russell


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/#review10631
---


On Jan. 17, 2014, 10:18 p.m., Russell Bryant wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3135/
> ---
> 
> (Updated Jan. 17, 2014, 10:18 p.m.)
> 
> 
> Review request for Asterisk Developers and leifmadsen.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> The ast_filestream object gets tacked on to a channel via
> chan->timingdata.  It's a reference counted object, but the reference
> count isn't used when putting it on a channel.  It's theoretically
> possible for another thread to interfere with the channel while it's
> unlocked and cause the filestream to get destroyed.
> 
> Use the astobj2 reference count to make sure that as long as this code
> path is holding on the ast_filestream and passing it into the file.c
> playback code, that it knows it's valid.
> 
> -
> 
> More detail:
> 
> A crash occurs in voicemail.  Here is the backtrace:
> 
> #0  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
> file.c:779
> #1  0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
> #2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
> channel.c:3865
> #3  0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
> #4  0x004d5231 in waitstream_core (c=0x7f2644c49460, 
> breakon=0x7f262ed56f00 "#*", forward=0x64c4a2 "", reverse=0x64c4a2 "", 
> skip_ms=0, audiofd=-1, cmdfd=-1, context=0x0) at file.c:1253
> #5  0x004d5783 in ast_waitstream (c=0x7f2644c49460, 
> breakon=0x7f262ed56f00 "#*") at file.c:1344
> #6  0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460, 
> ext=0x7f2609908c20 "", options=0x7f262ed56ff0) at 
> app_voicemail.c:5773
> #7  0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 
> "") at app_voicemail.c:10722
> 
> 
> (gdb)  frame 0
> #0  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
> file.c:779
> 779 if (s->owner->timingfd > -1) {
> (gdb) p s
> $6 = (struct ast_filestream *) 0x7f260856f580
> 
> The crash occurs here because the contents of the ast_filestream are garbage.
> 
> (gdb) p *s
> $7 = {fmt = 0x6372756f530a0d74, flags = 924858981, mode = 875902769, 
> open_filename = 0x440a0d3330393734  bounds>, filename = 0x6974616e69747365  bounds>, 
>   realfilename = 0x440a0d73203a6e6f  bounds>, vfs = 0x6974616e69747365, trans = 0x7865746e6f436e6f, tr = 
> 0x2d7473786e203a74, lastwriteformat = 762475363, lasttimeout = 1969583473, 
>   owner = 0x656c6c61430a0d65, f = 0x313722203a444972, fr = {frametype = 
> 875836727, subclass = {integer = 926687266, codec = 3761973748257529890}, 
> datalen = 809056052, samples = 168640051, mallocd = 1851877443, 
> mallocd_hdr_len = 7881689877736674080, offset = 762148397, src = 
> 0xd61633832313030 , data = {ptr = 
> 0x616e69747365440a, uint32 = 1936016394, pad = "\nDestina"}, delivery = {
>   tv_sec = 7953753055737899380, tv_usec = 5785246594218354030}, 
> frame_list = {next = 0x36702d5453584e2f}, flags = 758134073, ts = 
> 7010989772577650738, len = 7163375912484959347, seqno = 1869182049}, 
>   buf = 0x614c0a0d65756575 , 
> _private = 0x203a617461447473, orig_chan_name = 0x7273632d7473786e  0x7273632d7473786e out of bounds>, 
>   write_buffer = 0x38312c2c2c2c712d  bounds>}
> (gdb) p s->owner
> $8 = (struct ast_channel *) 0x656c6c61430a0d65
> (gdb) p *s->owner
> Cannot access memory at address 0x656c6c61430a0d65
> 
> s->owner should have been 0x7f2644c49460, from higher up in the backtrace.
> 
> 
> Here is where things get quite interesting ...
> 
> 
> (gdb) frame 2
> #2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
> channel.c:3865
> 3865func(data);
> (gdb) list
> 3860/

Re: [asterisk-dev] [Code Review] 3134: pbx.c: avoid segfault on uninitialized time values

[rmudgett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3134/#review10633
---


Only the corryfarrel patch is needed.  The other patch is not complete and is 
supurfluous.

- rmudgett


On Jan. 17, 2014, 12:51 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3134/
> ---
> 
> (Updated Jan. 17, 2014, 12:51 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22861
> https://issues.asterisk.org/jira/browse/ASTERISK-22861
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Merge of two patches - one from sebmurray and another from coreyfarrell - 
> related to eliminating referencing uninitialized structures in pbx.c.  Avoids 
> destroying timing unless it was successfully built, and leaking an 
> uninitialized timezone.
> 
> 
> Diffs
> -
> 
>   /branches/1.8/utils/extconf.c 405842 
>   /branches/1.8/main/pbx.c 405842 
> 
> Diff: https://reviewboard.asterisk.org/r/3134/diff/
> 
> 
> Testing
> ---
> 
> 
> 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

Re: [asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel

[rmudgett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/#review10631
---



/branches/1.8/main/channel.c


This should not be done at all.

You are dropping a reference when timingdata doesn't really hold the 
reference.  In the only case where timingdata is an identified ao2 object, the 
timingdata pointer is sharing the reference with c->stream.  The reference is 
dropped by ast_closestream() after clearing the timingdata with its own call to 
ast_settimeout().  Otherwise, you will need to give timingdata a reference when 
setting an identified ao2 object.



/branches/1.8/main/channel.c


Setting timingdata to NULL here is unnecessary since it is set with a new 
value right after.  It is the purpose of the function.


- rmudgett


On Jan. 17, 2014, 4:18 p.m., Russell Bryant wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3135/
> ---
> 
> (Updated Jan. 17, 2014, 4:18 p.m.)
> 
> 
> Review request for Asterisk Developers and leifmadsen.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> The ast_filestream object gets tacked on to a channel via
> chan->timingdata.  It's a reference counted object, but the reference
> count isn't used when putting it on a channel.  It's theoretically
> possible for another thread to interfere with the channel while it's
> unlocked and cause the filestream to get destroyed.
> 
> Use the astobj2 reference count to make sure that as long as this code
> path is holding on the ast_filestream and passing it into the file.c
> playback code, that it knows it's valid.
> 
> -
> 
> More detail:
> 
> A crash occurs in voicemail.  Here is the backtrace:
> 
> #0  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
> file.c:779
> #1  0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
> #2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
> channel.c:3865
> #3  0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
> #4  0x004d5231 in waitstream_core (c=0x7f2644c49460, 
> breakon=0x7f262ed56f00 "#*", forward=0x64c4a2 "", reverse=0x64c4a2 "", 
> skip_ms=0, audiofd=-1, cmdfd=-1, context=0x0) at file.c:1253
> #5  0x004d5783 in ast_waitstream (c=0x7f2644c49460, 
> breakon=0x7f262ed56f00 "#*") at file.c:1344
> #6  0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460, 
> ext=0x7f2609908c20 "", options=0x7f262ed56ff0) at 
> app_voicemail.c:5773
> #7  0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 
> "") at app_voicemail.c:10722
> 
> 
> (gdb)  frame 0
> #0  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
> file.c:779
> 779 if (s->owner->timingfd > -1) {
> (gdb) p s
> $6 = (struct ast_filestream *) 0x7f260856f580
> 
> The crash occurs here because the contents of the ast_filestream are garbage.
> 
> (gdb) p *s
> $7 = {fmt = 0x6372756f530a0d74, flags = 924858981, mode = 875902769, 
> open_filename = 0x440a0d3330393734  bounds>, filename = 0x6974616e69747365  bounds>, 
>   realfilename = 0x440a0d73203a6e6f  bounds>, vfs = 0x6974616e69747365, trans = 0x7865746e6f436e6f, tr = 
> 0x2d7473786e203a74, lastwriteformat = 762475363, lasttimeout = 1969583473, 
>   owner = 0x656c6c61430a0d65, f = 0x313722203a444972, fr = {frametype = 
> 875836727, subclass = {integer = 926687266, codec = 3761973748257529890}, 
> datalen = 809056052, samples = 168640051, mallocd = 1851877443, 
> mallocd_hdr_len = 7881689877736674080, offset = 762148397, src = 
> 0xd61633832313030 , data = {ptr = 
> 0x616e69747365440a, uint32 = 1936016394, pad = "\nDestina"}, delivery = {
>   tv_sec = 7953753055737899380, tv_usec = 5785246594218354030}, 
> frame_list = {next = 0x36702d5453584e2f}, flags = 758134073, ts = 
> 7010989772577650738, len = 7163375912484959347, seqno = 1869182049}, 
>   buf = 0x614c0a0d65756575 , 
> _private = 0x203a617461447473, orig_chan_name = 0x7273632d7473786e  0x7273632d7473786e out of bounds>, 
>   write_buffer = 0x38312c2c2c2c712d  bounds>}
> (gdb) p s->owner
> $8 = (struct ast_channel *) 0x656c6c61430a0d65
> (gdb) p *s->owner
> Cannot access memory at address 0x656c6c61430a0d65
> 
> s->owner should have been 0x7f2644c49460, from higher up in the backtrace.
> 
> 
> Here is where things get quite interesting ...
> 
> 
> (gdb) frame 2
> #2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
> channel.c:3865
> 3865func(data);
> (gdb) list
> 3860/* save a copy of func/data before 

Re: [asterisk-dev] [Code Review] 3136: cli: pjsip show endpoint shows allow/disallow codecs the same

[George Joseph]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3136/#review10632
---


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. :)



- George Joseph


On Jan. 17, 2014, 3: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, 3: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

[asterisk-dev] res_pjsip: MWI subscription missing name

[Kevin Harwell]

Greetings,

When a subscription to MWI comes into the res_pjsip_mwi module it will
attempt to extract an "aor_name" from the URI on the request.  Currently
a warning message is logged and the request is rejected if a suitable
AoR cannot be found by the given name.

But what should happen if the request URI doesn't contain the name to
begin with, but just the address?

Currently the best solution that was thought of (by Mark Michelson) is
(1) to just subscribe to all AoRs for the endpoint.  I like this
solution because a) it will work, b) it seems intuitive so easy for
users to understand c) it is the least ugly as far as any configuration
goes and least invasive as far as code changes go.

Other options potentially include:

(2) Use the contact - but then if a contact is part of multiple AoRs
then what?

(3) Attempt some sort of intersection between all AoRs on the endpoint
and those associated with the contact and use a best guess.  This isn't
bad and may narrow things down a bit more, but the results may be
unexpected (at least from a user perspective).

(4) Just use the mailboxes on the endpoint.  But what if they don't want
unsolicited MWI?

(5) Some other idea?

I'll probably go with option one barring any other ideas.  I can also
add a configuration option on the endpoint to turn this on/off if people
feel that is warranted.

Thanks!

-- 
Kevin Harwell
Digium, Inc. | Software Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: http://digium.com & http://asterisk.org


-- 
_
-- 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

[asterisk-dev] [Code Review] 3136: cli: pjsip show endpoint shows allow/disallow codecs the same

[Scott Griepentrog]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3136/
---

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

Re: [asterisk-dev] [Code Review] 2723: Add pass through support for both VP8 and Opus

[Sean Bright]

> On Jan. 16, 2014, 5:16 p.m., Sean Bright wrote:
> > /trunk/channels/chan_sip.c, lines 13419-13421
> > 
> >
> > Shouldn't this be &a_audio instead of &a_text, or does it matter?
> 
> Matt Jordan wrote:
> Good catch - feel free to patch away

Committed in r405877 (12)/r405878 (trunk)


- Sean


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2723/#review10605
---


On Aug. 23, 2013, 3:42 p.m., Matt Jordan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2723/
> ---
> 
> (Updated Aug. 23, 2013, 3:42 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp and Mark Michelson.
> 
> 
> Bugs: ASTERISK-21981
> https://issues.asterisk.org/jira/browse/ASTERISK-21981
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Note: This patch was written by Lorenzo Miniero. I know he's at the IETF this 
> week, but I figured we could get the formal code review going for him :-)
> 
> This patch adds pass through support for Opus and VP8. That includes:
> * Format attribute negotiation for Opus. Note that unlike some other codecs, 
> the draft RFC specifies having spaces delimiting the attributes in addition 
> to ';', so you have "attra=X; attrb=Y". This broke the attribute parsing in 
> chan_sip, so a small tweak was also included in this patch for that.
> * A format attribute negotiation module for Opus
> * Fast picture update for VP8. Since VP8 uses a different RTCP packet number 
> than FIR, this really is specific to VP8 at this time. Ideally this would be 
> more generic and flexible for user preferences and other video codecs, but 
> that could be done at a latter date.
> 
> The only part of this work that I did was port over the fast picture update 
> code to chan_pjsip. I *think* that chan_pjsip will still suck out the 
> attributes in res_pjsip_sdp_rtp, but I could be mistaken (Josh?)
> 
> 
> Diffs
> -
> 
>   /trunk/res/res_rtp_asterisk.c 397424 
>   /trunk/res/res_pjsip_sdp_rtp.c 397424 
>   /trunk/res/res_format_attr_opus.c PRE-CREATION 
>   /trunk/main/rtp_engine.c 397424 
>   /trunk/main/frame.c 397424 
>   /trunk/main/format.c 397424 
>   /trunk/main/channel.c 397424 
>   /trunk/include/asterisk/opus.h PRE-CREATION 
>   /trunk/include/asterisk/format.h 397424 
>   /trunk/channels/chan_sip.c 397424 
>   /trunk/channels/chan_pjsip.c 397424 
> 
> Diff: https://reviewboard.asterisk.org/r/2723/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-- 
_
-- 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

[asterisk-dev] [Code Review] 3135: Protect ast_filestream object when on a channel

[Russell Bryant]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3135/
---

Review request for Asterisk Developers and leifmadsen.


Repository: Asterisk


Description
---

The ast_filestream object gets tacked on to a channel via
chan->timingdata.  It's a reference counted object, but the reference
count isn't used when putting it on a channel.  It's theoretically
possible for another thread to interfere with the channel while it's
unlocked and cause the filestream to get destroyed.

Use the astobj2 reference count to make sure that as long as this code
path is holding on the ast_filestream and passing it into the file.c
playback code, that it knows it's valid.

-

More detail:

A crash occurs in voicemail.  Here is the backtrace:

#0  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
file.c:779
#1  0x004d3b8a in ast_fsread_audio (data=0x7f260856f580) at file.c:806
#2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
channel.c:3865
#3  0x00477d81 in ast_read (chan=0x7f2644c49460) at channel.c:4325
#4  0x004d5231 in waitstream_core (c=0x7f2644c49460, 
breakon=0x7f262ed56f00 "#*", forward=0x64c4a2 "", reverse=0x64c4a2 "", 
skip_ms=0, audiofd=-1, cmdfd=-1, context=0x0) at file.c:1253
#5  0x004d5783 in ast_waitstream (c=0x7f2644c49460, 
breakon=0x7f262ed56f00 "#*") at file.c:1344
#6  0x7f266d34c22b in leave_voicemail (chan=0x7f2644c49460, 
ext=0x7f2609908c20 "", options=0x7f262ed56ff0) at app_voicemail.c:5773
#7  0x7f266d35eb71 in vm_exec (chan=0x7f2644c49460, data=0x7f262ed59720 
"") at app_voicemail.c:10722


(gdb)  frame 0
#0  0x004d396a in ast_readaudio_callback (s=0x7f260856f580) at 
file.c:779
779 if (s->owner->timingfd > -1) {
(gdb) p s
$6 = (struct ast_filestream *) 0x7f260856f580

The crash occurs here because the contents of the ast_filestream are garbage.

(gdb) p *s
$7 = {fmt = 0x6372756f530a0d74, flags = 924858981, mode = 875902769, 
open_filename = 0x440a0d3330393734 , 
filename = 0x6974616e69747365 , 
  realfilename = 0x440a0d73203a6e6f , 
vfs = 0x6974616e69747365, trans = 0x7865746e6f436e6f, tr = 0x2d7473786e203a74, 
lastwriteformat = 762475363, lasttimeout = 1969583473, 
  owner = 0x656c6c61430a0d65, f = 0x313722203a444972, fr = {frametype = 
875836727, subclass = {integer = 926687266, codec = 3761973748257529890}, 
datalen = 809056052, samples = 168640051, mallocd = 1851877443, 
mallocd_hdr_len = 7881689877736674080, offset = 762148397, src = 
0xd61633832313030 , data = {ptr = 
0x616e69747365440a, uint32 = 1936016394, pad = "\nDestina"}, delivery = {
  tv_sec = 7953753055737899380, tv_usec = 5785246594218354030}, frame_list 
= {next = 0x36702d5453584e2f}, flags = 758134073, ts = 7010989772577650738, len 
= 7163375912484959347, seqno = 1869182049}, 
  buf = 0x614c0a0d65756575 , _private 
= 0x203a617461447473, orig_chan_name = 0x7273632d7473786e , 
  write_buffer = 0x38312c2c2c2c712d }
(gdb) p s->owner
$8 = (struct ast_channel *) 0x656c6c61430a0d65
(gdb) p *s->owner
Cannot access memory at address 0x656c6c61430a0d65

s->owner should have been 0x7f2644c49460, from higher up in the backtrace.


Here is where things get quite interesting ...


(gdb) frame 2
#2  0x0047599d in __ast_read (chan=0x7f2644c49460, dropaudio=0) at 
channel.c:3865
3865func(data);
(gdb) list
3860/* save a copy of func/data before 
unlocking the channel */
3861int (*func)(const void *) = 
chan->timingfunc;
3862void *data = chan->timingdata;
3863chan->fdno = -1;
3864ast_channel_unlock(chan);
3865func(data);
3866} else {
3867ast_timer_set_rate(chan->timer, 0);
3868chan->fdno = -1;
3869ast_channel_unlock(chan);
(gdb) p chan->timingfunc
$9 = (int (*)(const void *)) 0
(gdb) p chan->timingdata
$10 = (void *) 0x0
(gdb) p func
$11 = (int (*)(const void *)) 0x4d3b6a 
(gdb) p data
$12 = (void *) 0x7f260856f580


This is the code inside of ast_read() that executes the callback 
ast_fsread_audio() from main/file.c to play the next bits of the audio file to 
the channel.  We can see that chan->timingfunc and chan->timingdata have now 
been reset since this code ran.  That probably also means that the 
ast_filestream got destroyed before the code in ast_readaudio_callback was done 
using it.

The only way I can think of this happening is via something in another thread.  
Something like AMI running on a channel doing something that affects audio.  
I'm basically speculating at this 

Re: [asterisk-dev] [Code Review] 2723: Add pass through support for both VP8 and Opus

[Matt Jordan]

> On Jan. 16, 2014, 11:16 a.m., Sean Bright wrote:
> > /trunk/channels/chan_sip.c, lines 13419-13421
> > 
> >
> > Shouldn't this be &a_audio instead of &a_text, or does it matter?

Good catch - feel free to patch away


- Matt


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2723/#review10605
---


On Aug. 23, 2013, 10:42 a.m., Matt Jordan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2723/
> ---
> 
> (Updated Aug. 23, 2013, 10:42 a.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp and Mark Michelson.
> 
> 
> Bugs: ASTERISK-21981
> https://issues.asterisk.org/jira/browse/ASTERISK-21981
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Note: This patch was written by Lorenzo Miniero. I know he's at the IETF this 
> week, but I figured we could get the formal code review going for him :-)
> 
> This patch adds pass through support for Opus and VP8. That includes:
> * Format attribute negotiation for Opus. Note that unlike some other codecs, 
> the draft RFC specifies having spaces delimiting the attributes in addition 
> to ';', so you have "attra=X; attrb=Y". This broke the attribute parsing in 
> chan_sip, so a small tweak was also included in this patch for that.
> * A format attribute negotiation module for Opus
> * Fast picture update for VP8. Since VP8 uses a different RTCP packet number 
> than FIR, this really is specific to VP8 at this time. Ideally this would be 
> more generic and flexible for user preferences and other video codecs, but 
> that could be done at a latter date.
> 
> The only part of this work that I did was port over the fast picture update 
> code to chan_pjsip. I *think* that chan_pjsip will still suck out the 
> attributes in res_pjsip_sdp_rtp, but I could be mistaken (Josh?)
> 
> 
> Diffs
> -
> 
>   /trunk/res/res_rtp_asterisk.c 397424 
>   /trunk/res/res_pjsip_sdp_rtp.c 397424 
>   /trunk/res/res_format_attr_opus.c PRE-CREATION 
>   /trunk/main/rtp_engine.c 397424 
>   /trunk/main/frame.c 397424 
>   /trunk/main/format.c 397424 
>   /trunk/main/channel.c 397424 
>   /trunk/include/asterisk/opus.h PRE-CREATION 
>   /trunk/include/asterisk/format.h 397424 
>   /trunk/channels/chan_sip.c 397424 
>   /trunk/channels/chan_pjsip.c 397424 
> 
> Diff: https://reviewboard.asterisk.org/r/2723/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3131: PJSIP: support 'allow=all'

[Scott Griepentrog]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3131/
---

(Updated Jan. 17, 2014, 3:53 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Bugs: ASTERISK-23018
https://issues.asterisk.org/jira/browse/ASTERISK-23018


Repository: Asterisk


Description
---

Changes to support allow=all in PJSIP.conf: 

* added ast_codec_pref_append_all() that intelligently adds all yet unspecified 
codecs to the preference list

* Sorcery function ast_parse_allow_disallow() now uses codec_pref_append_all on 
pref list when all is supplied

* in create_outgoing_sdp_stream(), the loop checking codecs to add will now 
skip a codec that doesn't have a payload code instead of bailing (fixes issue 
with deferred sdp bombing on testlaw)

* switched display of codecs (reverse codec_handler_fn in sorcery.c) from caps 
to prefs, so that cli 'pjsip show endpoint x' shows allowed codec list in 
preference order matching the configuration

Note that it is now possible to put all at the end of an allow list, such as 
allow=ulaw,all which will set ulaw as preferred (first in list) but still have 
every other available codec listed.  Also possible is to remove specific codecs 
after all, such as allow=ulaw,all,!g729 which puts ulaw first, then every other 
codec, but removes g729.

The codec order otherwise is currently set by the internal codec table list 
order, which is arbitrary but functional.   


Diffs
-

  /branches/12/res/res_pjsip_sdp_rtp.c 405637 
  /branches/12/main/sorcery.c 405637 
  /branches/12/main/frame.c 405637 
  /branches/12/main/format_pref.c 405637 
  /branches/12/include/asterisk/format_pref.h 405637 

Diff: https://reviewboard.asterisk.org/r/3131/diff/


Testing
---

Tested with new test allow_all_sdp (https://reviewboard.asterisk.org/r/3132/) 
with success.  Also ran pjsip basic_calls. 


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

Re: [asterisk-dev] [Code Review] 3122: ARI: Add support for specifying channel variables during originate

[opticron]

> On Jan. 17, 2014, 12:38 p.m., Kevin Harwell wrote:
> > branches/12/res/ari/resource_channels.c, lines 753-756
> > 
> >
> > I'm sure you looked into this, but is there no way to have this block 
> > of code in the auto generated file res_ari_channels.c?  I'm guessing the 
> > answer is no as this is probably the crux of the problem, but figured I'd 
> > ask :-)

This block of code could easily be in the auto-generated code, but having this 
behavior be the default would break the model that Swagger operates on and 
would prevent the body parameter from being used in other ways. A body 
parameter is ALWAYS expected to consume the entirety of the body's JSON; this 
one just so happens to parse it as if there were no body parameter and only 
uses one element in the body.


> On Jan. 17, 2014, 12:38 p.m., Kevin Harwell wrote:
> > branches/12/res/res_ari_channels.c, line 209
> > 
> >
> > Is this line auto generated or did you have to manually enter it?  I 
> > know the file is and it would be a shame to have to manually edit that file 
> > from here on out.

This line lives in res_ari_channels.c and so was auto-generated.


- opticron


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3122/#review10625
---


On Jan. 13, 2014, 11:09 a.m., opticron wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3122/
> ---
> 
> (Updated Jan. 13, 2014, 11:09 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23051
> https://issues.asterisk.org/jira/browse/ASTERISK-23051
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This adds back in support for specifying channel variables during an 
> originate without compromising the ability to specify query parameters in the 
> JSON body. This was accomplished by generating the body-parsing code in a 
> separate function instead of being integrated with the URI query parameter 
> parsing code such that it could be called by paths with body parameters. This 
> is transparent to the user of the API and prevents manual duplication of code 
> or data structures.
> 
> 
> Diffs
> -
> 
>   branches/12/rest-api/api-docs/channels.json 405252 
>   branches/12/rest-api-templates/res_ari_resource.c.mustache 405252 
>   branches/12/rest-api-templates/param_parsing.mustache 405252 
>   branches/12/rest-api-templates/body_parsing.mustache PRE-CREATION 
>   branches/12/rest-api-templates/asterisk_processor.py 405252 
>   branches/12/rest-api-templates/ari_resource.h.mustache 405252 
>   branches/12/res/res_ari_sounds.c 405252 
>   branches/12/res/res_ari_playbacks.c 405252 
>   branches/12/res/res_ari_device_states.c 405252 
>   branches/12/res/res_ari_channels.c 405252 
>   branches/12/res/res_ari_bridges.c 405252 
>   branches/12/res/res_ari_asterisk.c 405252 
>   branches/12/res/res_ari_applications.c 405252 
>   branches/12/res/ari/resource_sounds.h 405252 
>   branches/12/res/ari/resource_playbacks.h 405252 
>   branches/12/res/ari/resource_device_states.h 405252 
>   branches/12/res/ari/resource_channels.c 405252 
>   branches/12/res/ari/resource_channels.h 405252 
>   branches/12/res/ari/resource_bridges.h 405252 
>   branches/12/res/ari/resource_asterisk.h 405252 
>   branches/12/res/ari/resource_applications.h 405252 
> 
> Diff: https://reviewboard.asterisk.org/r/3122/diff/
> 
> 
> Testing
> ---
> 
> Tested with a slightly modified originate_with_vars test created for the 
> original functionality by Kevin Harwell.
> 
> 
> Thanks,
> 
> opticron
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3126: testsuite: check chunked Transfer-Encoding operation

[Scott Griepentrog]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3126/
---

(Updated Jan. 17, 2014, 2:58 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers and Matt Jordan.


Bugs: ASTERISK-23068
https://issues.asterisk.org/jira/browse/ASTERISK-23068


Repository: testsuite


Description
---

Test checks operation of http chunked transfer encoding in review 
https://reviewboard.asterisk.org/r/3125/.  Uses requests library to initiate 
HTTP request, which uses chunked mode when a generator function supplies the 
data.  Writes a global variable using a chunked request, and then reads it back 
with a regular request and compares the value to the original. 


Diffs
-

  /asterisk/trunk/tests/rest_api/tests.yaml 4550 
  /asterisk/trunk/tests/rest_api/chunked-transfer/test-config.yaml PRE-CREATION 
  /asterisk/trunk/tests/rest_api/chunked-transfer/run-test PRE-CREATION 

Diff: https://reviewboard.asterisk.org/r/3126/diff/


Testing
---


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

Re: [asterisk-dev] [Code Review] 3125: http: support chunked Transfer-Encoding

[Scott Griepentrog]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3125/
---

(Updated Jan. 17, 2014, 2:51 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers and Matt Jordan.


Bugs: ASTERISK-23068
https://issues.asterisk.org/jira/browse/ASTERISK-23068


Repository: Asterisk


Description
---

Implements support for HTTP chunked mode Transfer-Encoding in JSON and Form 
(post vars) body content.  Relocated code from ast_http_get_json() and 
ast_http_get_post_vars() that reads content from stream into new function 
ast_http_get_contents(), and added support for reading and decoding chunked 
mode transfers.


Diffs
-

  /branches/12/main/http.c 405282 

Diff: https://reviewboard.asterisk.org/r/3125/diff/


Testing
---

Testsuite test rest_api/chunked_transfer added (see 
https://reviewboard.asterisk.org/r/3126/) to insure correct operation of 
chunked and regular mode.


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

Re: [asterisk-dev] [Code Review] 3125: http: support chunked Transfer-Encoding

[opticron]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3125/#review10627
---

Ship it!


The chunked decode looks good according to the RFC!

- opticron


On Jan. 13, 2014, 5:26 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3125/
> ---
> 
> (Updated Jan. 13, 2014, 5:26 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Bugs: ASTERISK-23068
> https://issues.asterisk.org/jira/browse/ASTERISK-23068
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Implements support for HTTP chunked mode Transfer-Encoding in JSON and Form 
> (post vars) body content.  Relocated code from ast_http_get_json() and 
> ast_http_get_post_vars() that reads content from stream into new function 
> ast_http_get_contents(), and added support for reading and decoding chunked 
> mode transfers.
> 
> 
> Diffs
> -
> 
>   /branches/12/main/http.c 405282 
> 
> Diff: https://reviewboard.asterisk.org/r/3125/diff/
> 
> 
> Testing
> ---
> 
> Testsuite test rest_api/chunked_transfer added (see 
> https://reviewboard.asterisk.org/r/3126/) to insure correct operation of 
> chunked and regular mode.
> 
> 
> 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

Re: [asterisk-dev] [Code Review] 3130: testsuite: Update hold test to monitor device state of PJSIP channels

[Mark Michelson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3130/#review10626
---

Ship it!


Ship It!

- Mark Michelson


On Jan. 16, 2014, 6:33 p.m., Jonathan Rose wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3130/
> ---
> 
> (Updated Jan. 16, 2014, 6:33 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
> 
> 
> Repository: testsuite
> 
> 
> Description
> ---
> 
> Tests that this is working: https://reviewboard.asterisk.org/r/3129/
> 
> Adds a hint monitoring the phone_A device so that device state changes to 
> phone_A will issue ExtensionStatus events.  These events are then evaluated 
> to check for inuse, notinuse, and hold statuses.
> 
> 
> Diffs
> -
> 
>   /asterisk/trunk/tests/channels/pjsip/hold/run-test 4559 
>   /asterisk/trunk/tests/channels/pjsip/hold/configs/ast1/extensions.conf 4559 
> 
> Diff: https://reviewboard.asterisk.org/r/3130/diff/
> 
> 
> Testing
> ---
> 
> Ran test against unpatched Asterisk 
> (https://reviewboard.asterisk.org/r/3129/) - Failed
> Ran test against patched Asterisk - Success
> Verified that the cause of the failures in each case was the lack of hold and 
> subsequent inuse status changes.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

-- 
_
-- 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

[asterisk-dev] [Code Review] 3134: pbx.c: avoid segfault on uninitialized time values

[Scott Griepentrog]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3134/
---

Review request for Asterisk Developers.


Bugs: ASTERISK-22861
https://issues.asterisk.org/jira/browse/ASTERISK-22861


Repository: Asterisk


Description
---

Merge of two patches - one from sebmurray and another from coreyfarrell - 
related to eliminating referencing uninitialized structures in pbx.c.  Avoids 
destroying timing unless it was successfully built, and leaking an 
uninitialized timezone.


Diffs
-

  /branches/1.8/utils/extconf.c 405842 
  /branches/1.8/main/pbx.c 405842 

Diff: https://reviewboard.asterisk.org/r/3134/diff/


Testing
---


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

Re: [asterisk-dev] [Code Review] 3122: ARI: Add support for specifying channel variables during originate

[Kevin Harwell]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3122/#review10625
---



branches/12/res/ari/resource_channels.c


I'm sure you looked into this, but is there no way to have this block of 
code in the auto generated file res_ari_channels.c?  I'm guessing the answer is 
no as this is probably the crux of the problem, but figured I'd ask :-)



branches/12/res/res_ari_channels.c


Is this line auto generated or did you have to manually enter it?  I know 
the file is and it would be a shame to have to manually edit that file from 
here on out.


- Kevin Harwell


On Jan. 13, 2014, 11:09 a.m., opticron wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3122/
> ---
> 
> (Updated Jan. 13, 2014, 11:09 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23051
> https://issues.asterisk.org/jira/browse/ASTERISK-23051
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This adds back in support for specifying channel variables during an 
> originate without compromising the ability to specify query parameters in the 
> JSON body. This was accomplished by generating the body-parsing code in a 
> separate function instead of being integrated with the URI query parameter 
> parsing code such that it could be called by paths with body parameters. This 
> is transparent to the user of the API and prevents manual duplication of code 
> or data structures.
> 
> 
> Diffs
> -
> 
>   branches/12/rest-api/api-docs/channels.json 405252 
>   branches/12/rest-api-templates/res_ari_resource.c.mustache 405252 
>   branches/12/rest-api-templates/param_parsing.mustache 405252 
>   branches/12/rest-api-templates/body_parsing.mustache PRE-CREATION 
>   branches/12/rest-api-templates/asterisk_processor.py 405252 
>   branches/12/rest-api-templates/ari_resource.h.mustache 405252 
>   branches/12/res/res_ari_sounds.c 405252 
>   branches/12/res/res_ari_playbacks.c 405252 
>   branches/12/res/res_ari_device_states.c 405252 
>   branches/12/res/res_ari_channels.c 405252 
>   branches/12/res/res_ari_bridges.c 405252 
>   branches/12/res/res_ari_asterisk.c 405252 
>   branches/12/res/res_ari_applications.c 405252 
>   branches/12/res/ari/resource_sounds.h 405252 
>   branches/12/res/ari/resource_playbacks.h 405252 
>   branches/12/res/ari/resource_device_states.h 405252 
>   branches/12/res/ari/resource_channels.c 405252 
>   branches/12/res/ari/resource_channels.h 405252 
>   branches/12/res/ari/resource_bridges.h 405252 
>   branches/12/res/ari/resource_asterisk.h 405252 
>   branches/12/res/ari/resource_applications.h 405252 
> 
> Diff: https://reviewboard.asterisk.org/r/3122/diff/
> 
> 
> Testing
> ---
> 
> Tested with a slightly modified originate_with_vars test created for the 
> original functionality by Kevin Harwell.
> 
> 
> Thanks,
> 
> opticron
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3118: Testsuite: Add tests for ARI control of external MWI (ARI mailboxes resource)

[Mark Michelson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3118/#review10624
---

Ship it!


Ship It!

- Mark Michelson


On Jan. 16, 2014, 8:26 p.m., Jonathan Rose wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3118/
> ---
> 
> (Updated Jan. 16, 2014, 8:26 p.m.)
> 
> 
> Review request for Asterisk Developers, Kevin Harwell, Matt Jordan, and Mark 
> Michelson.
> 
> 
> Repository: testsuite
> 
> 
> Description
> ---
> 
> Somewhat similar to kharwell's device state tests.
> 
> ARI is used to:
> * create a couple mailboxes
> * confirm that GET can be used to get one mailbox and that it matches 
> expectations
> * confirm that GET can be used to get all the mailboxes and that they match 
> expectations
> * modify one mailbox
> * delete another mailbox
> * verify that the mailbox that was deleted is no longer obtained by get
> * verify that the mailbox that was modified has the modified state.
> 
> The nature of these operations is simple enough that this seemed appropriate 
> to be a single test.
> 
> 
> Diffs
> -
> 
>   /asterisk/trunk/tests/rest_api/tests.yaml 4545 
>   /asterisk/trunk/tests/rest_api/mailbox/baseline/test-config.yaml 
> PRE-CREATION 
>   /asterisk/trunk/tests/rest_api/mailbox/baseline/mailbox_baseline.py 
> PRE-CREATION 
>   
> /asterisk/trunk/tests/rest_api/mailbox/baseline/configs/ast1/extensions.conf 
> PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/3118/diff/
> 
> 
> Testing
> ---
> 
> Ran the test. Varied the expectations from the results to cause failures for 
> each substep.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3131: PJSIP: support 'allow=all'

[Mark Michelson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3131/#review10623
---

Ship it!


Because my comments are so minor, I'm going ahead with a "ship it!" on the 
review.


/branches/12/main/format_pref.c


And the award for most nit-picky critique on a code review goes to: 


Mark Michelson! For his comment: "The i.e. here should really be an e.g. 
since you are providing an example of an entry that could result in duplicates".



/branches/12/main/format_pref.c


Coding guidelines: add spaces around the plus sign.



/branches/12/main/format_pref.c


Coding guidelines: add spaces around the plus sign.


- Mark Michelson


On Jan. 16, 2014, 10:13 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3131/
> ---
> 
> (Updated Jan. 16, 2014, 10:13 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23018
> https://issues.asterisk.org/jira/browse/ASTERISK-23018
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Changes to support allow=all in PJSIP.conf: 
> 
> * added ast_codec_pref_append_all() that intelligently adds all yet 
> unspecified codecs to the preference list
> 
> * Sorcery function ast_parse_allow_disallow() now uses codec_pref_append_all 
> on pref list when all is supplied
> 
> * in create_outgoing_sdp_stream(), the loop checking codecs to add will now 
> skip a codec that doesn't have a payload code instead of bailing (fixes issue 
> with deferred sdp bombing on testlaw)
> 
> * switched display of codecs (reverse codec_handler_fn in sorcery.c) from 
> caps to prefs, so that cli 'pjsip show endpoint x' shows allowed codec list 
> in preference order matching the configuration
> 
> Note that it is now possible to put all at the end of an allow list, such as 
> allow=ulaw,all which will set ulaw as preferred (first in list) but still 
> have every other available codec listed.  Also possible is to remove specific 
> codecs after all, such as allow=ulaw,all,!g729 which puts ulaw first, then 
> every other codec, but removes g729.
> 
> The codec order otherwise is currently set by the internal codec table list 
> order, which is arbitrary but functional.   
> 
> 
> Diffs
> -
> 
>   /branches/12/res/res_pjsip_sdp_rtp.c 405637 
>   /branches/12/main/sorcery.c 405637 
>   /branches/12/main/frame.c 405637 
>   /branches/12/main/format_pref.c 405637 
>   /branches/12/include/asterisk/format_pref.h 405637 
> 
> Diff: https://reviewboard.asterisk.org/r/3131/diff/
> 
> 
> Testing
> ---
> 
> Tested with new test allow_all_sdp (https://reviewboard.asterisk.org/r/3132/) 
> with success.  Also ran pjsip basic_calls. 
> 
> 
> 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

Re: [asterisk-dev] [Code Review] 3051: TestSuite: Add chan_pjsip path support tests

[wdoekes]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3051/#review10619
---


Without going into the functionality of the patch, a few notes:


asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/ua1_invite_recv.xml


You're referencing more than the ";tag" bit, below:

To: [$remote_tag]



asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/ua1_invite_recv.xml


I think you can do:

From: [last_To]
To: [last_From]

But don't take my word for it.



asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/ua1_invite_recv.xml


A bit inconsitent with contacts with and without angle brackets. Not that 
it matters.


- wdoekes


On Jan. 6, 2014, 9:53 p.m., opticron wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3051/
> ---
> 
> (Updated Jan. 6, 2014, 9:53 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21084
> https://issues.asterisk.org/jira/browse/ASTERISK-21084
> 
> 
> Repository: testsuite
> 
> 
> Description
> ---
> 
> This adds a test which covers path support for outbound registrations, 
> inbound registrations, and outbound requests following an inbound 
> registration.
> 
> 
> Diffs
> -
> 
>   asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/tests.yaml 
> 4485 
>   
> asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/test-config.yaml
>  PRE-CREATION 
>   
> asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/ua1_register.xml
>  PRE-CREATION 
>   
> asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/ua1_invite_recv.xml
>  PRE-CREATION 
>   
> asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/sipp/registrar.xml
>  PRE-CREATION 
>   
> asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/configs/ast1/pjsip.conf
>  PRE-CREATION 
>   
> asterisk/trunk/tests/channels/pjsip/registration/inbound/nominal/path/configs/ast1/extensions.conf
>  PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/3051/diff/
> 
> 
> Testing
> ---
> 
> Ensured the tests behaved as expected.
> 
> 
> Thanks,
> 
> opticron
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3133: manager: Clarify eventfilter documentation

[wdoekes]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3133/
---

(Updated Jan. 17, 2014, 9:46 a.m.)


Review request for Asterisk Developers.


Changes
---

going +on


Repository: Asterisk


Description (updated)
---

Hi, I wasn't too happy with the eventfilter documentation, so I tried to 
clarify it a bit.

Suggestions are welcome.

I'm still not fond of the DAHDI.* example, since it implies that there is 
anchoring going on, which there isn't.


Diffs
-

  /branches/1.8/main/manager.c 405159 
  /branches/1.8/configs/manager.conf.sample 405159 

Diff: https://reviewboard.asterisk.org/r/3133/diff/


Testing
---

No testing needed. Changes are textual only.


Thanks,

wdoekes

-- 
_
-- 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

[asterisk-dev] [Code Review] 3133: manager: Clarify eventfilter documentation

[wdoekes]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3133/
---

Review request for Asterisk Developers.


Repository: Asterisk


Description
---

Hi, I wasn't too happy with the eventfilter documentation, so I tried to 
clarify it a bit.

Suggestions are welcome.

I'm still not fond of the DAHDI.* example, since it implies that there is 
anchoring going, which there isn't.


Diffs
-

  /branches/1.8/main/manager.c 405159 
  /branches/1.8/configs/manager.conf.sample 405159 

Diff: https://reviewboard.asterisk.org/r/3133/diff/


Testing
---

No testing needed. Changes are textual only.


Thanks,

wdoekes

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3129: chan_pjsip: Provide a means for updating device state when going on/off hold

[Mark Michelson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3129/#review10622
---

Ship it!


Ship It!

- Mark Michelson


On Jan. 16, 2014, 8:56 p.m., Jonathan Rose wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3129/
> ---
> 
> (Updated Jan. 16, 2014, 8:56 p.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Maintains a list of on hold channel uniqueids.  When a snapshot is used with 
> chan_pjsip's device state evaluating function, this list is queried to see if 
> the channel in the snapshot is on hold. As the channel receives indications 
> of HOLD/UNHOLD, this list is updated and the device state is updated. When 
> the session ends, the session channel is removed from the list if it is still 
> in it.
> 
> 
> Diffs
> -
> 
>   /branches/12/channels/chan_pjsip.c 405641 
> 
> Diff: https://reviewboard.asterisk.org/r/3129/diff/
> 
> 
> Testing
> ---
> 
> Manual testing where I watched the device state with a websocket, polled for 
> device state with Manager command GetVar and DEVICE_STATE function, and also 
> modified an automated test (channels/pjsip/hold) to have a hint which checks 
> the device state of a PJSIP channel and ExtensionStatus event is monitored to 
> evaluate device state changes as they had.
> 
> The test modifications will be in another review.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3125: http: support chunked Transfer-Encoding

[Mark Michelson]

> On Jan. 16, 2014, 5 p.m., Mark Michelson wrote:
> > /branches/12/main/http.c, line 813
> > 
> >
> > chunked_atoh shouldn't be necessary. You should be able to just use:
> > 
> > sscanf(chunk_header, %x, &chunk_length);
> > 
> > You would need to change chunk_length to be an unsigned int instead of 
> > an int, but that's fine since the length will never be negative.
> 
> Scott Griepentrog wrote:
> I had considered that, but I wanted to be overly cautious and throw extra 
> checking into the algorithm, such that any invalid character or unexpected 
> sequence would result in an error, rather than partial input and confused 
> results.
>

Fair enough. I'll add my ship it since that's the only thing I noticed.


- Mark


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3125/#review10603
---


On Jan. 13, 2014, 11:26 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3125/
> ---
> 
> (Updated Jan. 13, 2014, 11:26 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Bugs: ASTERISK-23068
> https://issues.asterisk.org/jira/browse/ASTERISK-23068
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Implements support for HTTP chunked mode Transfer-Encoding in JSON and Form 
> (post vars) body content.  Relocated code from ast_http_get_json() and 
> ast_http_get_post_vars() that reads content from stream into new function 
> ast_http_get_contents(), and added support for reading and decoding chunked 
> mode transfers.
> 
> 
> Diffs
> -
> 
>   /branches/12/main/http.c 405282 
> 
> Diff: https://reviewboard.asterisk.org/r/3125/diff/
> 
> 
> Testing
> ---
> 
> Testsuite test rest_api/chunked_transfer added (see 
> https://reviewboard.asterisk.org/r/3126/) to insure correct operation of 
> chunked and regular mode.
> 
> 
> 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

Re: [asterisk-dev] [Code Review] 3125: http: support chunked Transfer-Encoding

[Mark Michelson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3125/#review10621
---

Ship it!


Ship It!

- Mark Michelson


On Jan. 13, 2014, 11:26 p.m., Scott Griepentrog wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3125/
> ---
> 
> (Updated Jan. 13, 2014, 11:26 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Bugs: ASTERISK-23068
> https://issues.asterisk.org/jira/browse/ASTERISK-23068
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Implements support for HTTP chunked mode Transfer-Encoding in JSON and Form 
> (post vars) body content.  Relocated code from ast_http_get_json() and 
> ast_http_get_post_vars() that reads content from stream into new function 
> ast_http_get_contents(), and added support for reading and decoding chunked 
> mode transfers.
> 
> 
> Diffs
> -
> 
>   /branches/12/main/http.c 405282 
> 
> Diff: https://reviewboard.asterisk.org/r/3125/diff/
> 
> 
> Testing
> ---
> 
> Testsuite test rest_api/chunked_transfer added (see 
> https://reviewboard.asterisk.org/r/3126/) to insure correct operation of 
> chunked and regular mode.
> 
> 
> 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