[asterisk-dev] [Code Review] Continuing from 1643 (Allow specifying which MixMonitor to Stop)

[Mark Michelson]

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


I'm in agreement with Richard with regards to trying to size the buffer for the pointer. It's a bad idea to try to assume how a system will stringify a pointer when using %p. There are a couple of ways around this:

1. Size the buffer to something larger than you think is necessary, like 64 or something. Even still, I'd feel more comfortable using snprintf() for printing the value instead of sprintf(). And even still, this may not guarantee uniqueness if it by some bizarre reason turns out that a system prints its pointers some weird way, like in binary.
2. Another way to guarantee uniqueness and know the size of the data you will store is to have an unsigned integer at file scope that increases by one each time you need a unique id. You can use ast_atomic_fetchadd_int to guarantee that no two threads end up with the same value. There are examples of this used in several places in Asterisk. I know off the top of my head that ccss.c uses this for identifying each of its core instances it allocates.


/trunk/CHANGES
<https://reviewboard.asterisk.org/r/1682/#comment9767>

    "pointer" doesn't really have a meaning in the Asterisk dialplan. You should probably change this to "channel variable" instead.
    
    This doesn't make mention of the fact that the CLI and Manager have also been updated.
    



/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1682/#comment9766>

    The wording here is wonky and makes it sound like multiple channel variables can be specified. Change it to something simple like "Store the Mixmonitor's identity on this channel variable."



/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1682/#comment9760>

    spaces around "<<"



/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1682/#comment9765>

    No need for this.



/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1682/#comment9763>

    On the second if, you already know that mixmonitor_ds->audiohook is non-NULL, so you don't need to check it again.



/trunk/apps/app_mixmonitor.c
<https://reviewboard.asterisk.org/r/1682/#comment9768>

    This change seems unrelated to the addition of stopping a mixmonitor by ID. Can you explain why it's here?


- Mark


On Jan. 20, 2012, 1:07 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1682/
> -----------------------------------------------------------
> 
> (Updated Jan. 20, 2012, 1:07 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and telecos.
> 
> 
> Summary
> -------
> 
> Fixes style problems from r6 on https://reviewboard.asterisk.org/r/1643/
> Changes creation of character buffer to length of a pointer in characters + 3 (2 for 0x, 1 for terminating space)
> 
> That might not be adequate.  Richard was saying something about pointers on other operating systems having other dividing symbols in them.  Well, at least ':'s.
> 
> In my experiences though, a pointer is usually just something like 0x0123FEDC when printed with %p.
> 
> 
> This addresses bug ASTERISK-19096.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19096
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 351538 
>   /trunk/apps/app_mixmonitor.c 351538 
> 
> Diff: https://reviewboard.asterisk.org/r/1682/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120120/e4955e07/attachment-0001.htm>