[asterisk-dev] [Code Review]: Prevent manager show commands cutting off the command name in favor of other, less important information.

[jrose]

> On Oct. 4, 2012, 10:21 a.m., Matt Jordan wrote:
> > /trunk/main/manager.c, lines 1811-1819
> > <https://reviewboard.asterisk.org/r/2143/diff/1/?file=31701#file31701line1811>
> >
> >     What happens if the length of cur->action is greater than 85?
> >     
> >     If the goal of this patch is to never truncate the name of the manager action, than it should gracefully handle the case where the manager action name is sufficiently long enough to remove any description.
> 
> jrose wrote:
>     If we ever have an action name longer than 85 characters, we have failed.
> 
> Tilghman Lesher wrote:
>     Sounds like a test that could be written (enumerate the list of manager actions, test the length of each, fail if any are >85 with a note pointing to this code).

I think that's entirely unnecessary. It's not ever going to happen for one thing (laziness trumps stupidity), and for another it's easy enough to adjust this not to care. I just finished checking what happens with this code if a string over 85 characters is used and it's somewhat interesting.  Apparently the format specifier breaks on negative numbers, so the name itself is displayed perfectly normally, but the whole synopsis is written as well.  All we need to add to make this not truncate and omit the synopsis if a command is over 85 characters long is to set a hard floor of 0 for the synopsis string. Then if we have an name over 85 characters, the synopsis field will just be skipped entirely including the header with the dashes.

Seriously though, 85 character command names for manager are absurd.


- jrose


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


On Oct. 4, 2012, 10:15 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2143/
> -----------------------------------------------------------
> 
> (Updated Oct. 4, 2012, 10:15 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> First, this patch cuts out information about the required authorization level.  There is really no need for it here and this information is freely avaialble with the 'manager show command [command]' information.
> 
> Second, the size field for the name is now picked based on the size of the largest command name. Any remaining space (out of 85, based on the sum of all previous fields, though I could add 2 since I removed some spaces as well and still be length neutral) will be given to the description.
> 
> 
> This addresses bug ASTERISK-20396.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20396
> 
> 
> Diffs
> -----
> 
>   /trunk/main/manager.c 374334 
> 
> Diff: https://reviewboard.asterisk.org/r/2143/diff
> 
> 
> Testing
> -------
> 
> I used the command. It did what I thought it would. Seems pretty simplish.
> 
> Old output:
>   Action           Privilege        Synopsis                                               
>   ------           ---------        --------                                               
>   [...]           
>   StopMixMonitor   <none>           Stop recording a call through MixMonitor, and free the 
>   MixMonitor       <none>           Record a call and mix the audio during the recording.  
>   MixMonitorMute   <none>           Mute / unMute a Mixmonitor recording.                  
>   ConfbridgeSetSi  call,all         Set a conference user as the single video source distri
>   [...]
> 
> New output:
> 
> *CLI> manager show commands
>   Action                       Synopsis                                                  
>   ------                       --------                                                  
>   [...]
>   StopMixMonitor               Stop recording a call through MixMonitor, and free the rec
>   MixMonitor                   Record a call and mix the audio during the recording.  Use
>   MixMonitorMute               Mute / unMute a Mixmonitor recording.                     
>   ConfbridgeSetSingleVideoSrc  Set a conference user as the single video source distribut
>   [...]
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20121004/4d67d470/attachment.htm>