[asterisk-dev] [Code Review] 4372: confbridge: Restore menu name associated with confbridge user to CLI output of 'confbridge list XXX'

[rmudgett]

> On Jan. 26, 2015, 4:55 p.m., Scott Griepentrog wrote:
> > /branches/13/apps/confbridge/conf_config_parser.c, line 2304
> > <https://reviewboard.asterisk.org/r/4372/diff/1/?file=71080#file71080line2304>
> >
> >     Use ast_copy_string?
> 
> rmudgett wrote:
>     I second this should be ast_copy_string() for safety.
> 
> Matt Jordan wrote:
>     There's no need to use ast_copy_string if the buffers are guaranteed to be the same length, which is the case for both structs.
>     
>     To quote further from the Coding Guidelines:
>     
>     {quote}
>     Don't use ast_copy_string (or any length-limited copy function) for copying fixed (known at compile time) strings into buffers, if the buffer is something that has been allocated in the function doing the copying. In that case, you know at the time you are writing the code whether the buffer is large enough for the fixed string or not, and if it's not, your code won't work anyway! Use strcpy() for this operation, or directly set the first two characters of the buffer if you are just trying to store a one character string in the buffer. If you are trying to 'empty' the buffer, just store a single NULL character ('\0') in the first byte of the buffer; nothing else is needed, and any other method is wasteful.
>     {quote}
>     
>     While that's referring to a slightly different situation, we know that both the source and destination are the same length. There's no need to use ast_copy_string.

True, but the sizes are defined elsewhere and not allocated right here in this function.

At the very least the /* Safe */ comment should be added like elsewhere.


- rmudgett


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


On Jan. 26, 2015, 7:33 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4372/
> -----------------------------------------------------------
> 
> (Updated Jan. 26, 2015, 7:33 p.m.)
> 
> 
> Review request for Asterisk Developers and Jonathan Rose.
> 
> 
> Bugs: ASTERISK-24723
>     https://issues.asterisk.org/jira/browse/ASTERISK-24723
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> When issuing a 'confbridge list XXXX' CLI command, the resulting output no longer displays the menu associated with a ConfBridge participant:
> 
>  Channel                        Flags  User Profile     Bridge Profile   Menu             CallerID
> ============================== ====== ================ ================ ================ ================
> PJSIP/1601-00000004                   default_user     default_bridge                    1601
> 
> The issue was caused by ASTERISK-22760. When that patch was done, it removed the copying of the menu name associated with the user from the actual user profile.
> 
> This patch fixes the issue by copying the menu name over to the user profile when the menu hooks are applied to the user. Since that function now does a little bit more than just apply the hooks, the name of the function has been changed to cover the copying of the menu name over as well.
> 
> In addition, there is a disparity between the menu name length as it is stored on the conf_menu structure and the confbridge_user structure; this patch makes the lengths match so that a strcpy can be used.
> 
> 
> Diffs
> -----
> 
>   /branches/13/apps/confbridge/include/confbridge.h 431019 
>   /branches/13/apps/confbridge/conf_config_parser.c 431019 
> 
> Diff: https://reviewboard.asterisk.org/r/4372/diff/
> 
> 
> Testing
> -------
> 
> Ran the CLI command. The output now shows the menu associated with the user.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150127/911a2e08/attachment-0001.html>