[asterisk-dev] [Code Review] 464: Group Variables
Mark Michelson
reviewboard at asterisk.org
Fri Aug 23 14:41:02 CDT 2013
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/464/#review9503
-----------------------------------------------------------
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18594>
Based on what the code does, this documentation is a bit off. The documentation makes it sound as though you can provide a group without a category or a category without a group. The code requires that a group is present, but the category is optional. I think this description can just be removed.
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18576>
s/a list the/a list of/
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18577>
Use a <see-also> block for this.
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18578>
Use a <see-also> block for this.
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18579>
These aren't necessary since astman_get_header will never return NULL. It returns an empty string if the header is not present.
XML documentation for this manager action states that one of either group or category must be present. There is nothing in this function that enforces that requirement.
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18580>
group_category is leaked. You'll either need to free the variable or switch to a method that does not heap allocate.
I peeked into app.c and noticed that no matter what you put into the second parameter to ast_app_group_set_channel(), the function only will care about the first 80 characters of the group and the first 80 characters of the category. While there's no external documentation that states this, I'd accept a statically sized
char group_category[80 + 80 + 2];
as long as there is a comment explaining why the sizes were chosen.
Also, should you choose to keep this heap-allocated, using ast_asprintf() would be an easier way of allocating the variable. Also, should you choose to keep this heap-allocated, be sure to check for allocation failure.
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18581>
Braces around if statements. Also log a warning explaining why the function invocation failed.
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18585>
Use a tab to indent
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18596>
Providing an empty string for the value should be allowable. It is for the manager version of the command, at least.
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18597>
Use braces.
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18588>
This cannot be NULL. astman_get_header will return an empty string if the header is not present.
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18591>
Like in other comments, this is not necessary since astman_get_header() will not return NULL.
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18598>
We have a handy macro that can replace the ternary logic you're using here:
S_OR(gi->category, "")
trunk/funcs/func_groupcount.c
<https://reviewboard.asterisk.org/r/464/#comment18599>
This can be massively simplified by using an ast_str (which automatically expands if necessary).
struct ast_str *str = ast_str_create(64);
AST_LIST_TRAVERSE(headp, vardata, entries) {
ast_str_append(&str, 0, "Variable(%s): %s\r\n");
}
/* Send manager event */
astman_append(...);
ast_free(str);
No need to keep track of the allocation space yourself this way.
trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/464/#comment18616>
This is unused.
trunk/main/app.c
<https://reviewboard.asterisk.org/r/464/#comment18583>
I know that the code in app.h sets a poor example, but typically the doxygen for a public function goes in the .h file instead of the .c file. The rationale is that you shouldn't even have to look at the implementation in order to understand what a function does.
Please do this for all public functions you've added to app.c. For internal functions, it's fine to have the doxygen in the .c file.
trunk/main/app.c
<https://reviewboard.asterisk.org/r/464/#comment18600>
This warning message will attempt to print a NULL value. On most systems this will result in the value "(null)" being printed, but on others this will crash.
trunk/main/app.c
<https://reviewboard.asterisk.org/r/464/#comment18601>
You've sort of mixed up the list traversal macros here. The "safe" variant is intended to be used if you need to insert or delete an item from the list during a traversal.
Thus the inner list traversal should use AST_LIST_TRAVERSE_SAFE_BEGIN and it should use AST_LIST_REMOVE_CURRENT to remove the items from the list.
Meanwhile, the outer traversal does not modify the list that is being traversed, so there is no need to use a safe traversal for it.
trunk/main/app.c
<https://reviewboard.asterisk.org/r/464/#comment18602>
This is probably going to be the most painful point when trying to update this patch further.
Manager events in Asterisk 12 are a whole new ballgame from what they were in previous versions.
First, new manager events should have XML documentation. That's easy enough.
But more importantly, it's incredibly uncommon now to raise manager events in-line. Instead, generate a stasis event The manager can then forward this event out over AMI. When adding new events in Asterisk, always think of stasis first since other conveyances (such as AMI) should be using stasis as the basis for sending messages.
In your case, what I would suggest you do is look through the code for uses of ast_channel_publish_blob() since what you are publishing is a channel and corresponding data. Use those as an example for how to publish the stasis message. From there, you can work out how to then get that stasis message converted to an AMI event.
A simple example of how to turn a stasis event for a particular message type into an AMI event can be found in main/stasis_channels.c for the ast_channel_varset_type.
You can also use these wiki pages as guides:
https://wiki.asterisk.org/wiki/display/AST/Stasis+Message+Bus
https://wiki.asterisk.org/wiki/display/AST/Using+the+Stasis+Message+Bus
This same comment applies to other manager events that have been added in this file.
trunk/main/app.c
<https://reviewboard.asterisk.org/r/464/#comment18582>
Spacing appears off on these two if statements. Be sure to indent with tabs.
trunk/main/app.c
<https://reviewboard.asterisk.org/r/464/#comment18605>
This comment should be removed.
trunk/main/app.c
<https://reviewboard.asterisk.org/r/464/#comment18607>
Another place where you need to indent with a tab.
trunk/main/app.c
<https://reviewboard.asterisk.org/r/464/#comment18613>
Since the list is not being modified in this traversal there's no need for a safe traversal.
trunk/main/cli.c
<https://reviewboard.asterisk.org/r/464/#comment18614>
Use braces.
trunk/main/cli.c
<https://reviewboard.asterisk.org/r/464/#comment18615>
Another possible use of S_OR.
S_OR(gmi->category, "(Default)")
- Mark Michelson
On July 29, 2013, 7:02 p.m., kobaz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/464/
> -----------------------------------------------------------
>
> (Updated July 29, 2013, 7:02 p.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Bugs: 16614
> https://issues.asterisk.org/jira/browse/16614
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> This patch allows for setting variables on a group of channels.
> First a channel is assigned a group via dialplan GROUP() or GroupSet via the ami (new command)
> Then, variables can be set on that group with the GROUP_VAR() dialplan function, or GroupVarSet in the manager
>
> This patch also adds manager events for channel group changes, and variable updates
>
> When a group is no longer used (all channels referencing the group are hung up, or all channels referencing the group have their group changed/unset), variables are destroyed
>
> new manager commands:
> GroupSet - adds functionality to the manager to be able to set a GROUP() on a channel.
> GroupsShowChannels - show each channel and it's associated groups (a channel will be repeated for each group at category it's a member of)
> GroupsShowVariables - show variables in each group at category, one event per group, all variables are contained in each group at category event
> GroupVarSet - set a group variable (the group has to exist)
> GroupVarGet - get a group variable
>
> new manager events:
> GroupCreate
> GroupChannelAdd
> GroupChannelRemove
> GroupDestroy
> GroupVarSet
>
> Example AMI:
> Action: GroupVarSet
> Group: 1000
> Category: ConferenceBridge
> Variable: LastUpdateCheck
> Value: 1263502512
>
> Example CLI:
> demo1*CLI> group show variables
> Group Variables Category
> 1000 ConferenceBridge
> LastUpdateCheck=1263502512
> 1000 ConferenceModerator
> 2 active groups
>
>
> Adds support for:
> Group variables
> dialplan access: GROUP_VAR()
> cli access: group show variables
> manager access: GroupVarSet GroupVarGet
> Group information:
> manager access: GroupSet GroupsShow GroupsShowChannels GroupsShowVariables
>
>
>
> Diffs
> -----
>
> trunk/funcs/func_groupcount.c 395697
> trunk/include/asterisk/app.h 395697
> trunk/include/asterisk/channel.h 395697
> trunk/main/app.c 395697
> trunk/main/cli.c 395697
>
> Diff: https://reviewboard.asterisk.org/r/464/diff/
>
>
> Testing
> -------
>
> dialplan:
> Set(GROUP()=foo);
> Set(GROUP()=foobar at foocat);
>
> Set(GROUP_VAR(foo,myvar)=123);
> Set(GROUP_VAR(foo,myvar2)=456);
> NoOp(${GROUP_VAR(foo,myvar)});
> NoOp(${GROUP_VAR(foo,myvar2)});
>
> Set(GROUP_VAR(foobar at foocat,myvar)=123);
> NoOp(${GROUP_VAR(foobar at foocat,myvar)});
>
> Set(GROUP_VAR(something,myvar)=123);
> NoOp(${GROUP_VAR(something,myvar)});
>
> Wait(10000);
> Hangup();
>
>
> CLI:
> -------------------
> > group show channels
> -------------------
> channel Group Category
> IAX2/branch-15569 foo (default)
> IAX2/branch-15569 foobar foocat
> 2 active channels
>
> -------------------
> > group show variables
> -------------------
> Group Variables Category
> foo (Default)
> myvar2=456
> myvar=123
> foobar foocat
> myvar=123
> 2 active groups
>
>
> AMI:
> -------------------
> Action: GroupsShow
> -------------------
> Response: Success
> Eventlist: start
> Message: Groups will follow
>
> Event: GroupsShow
> Group: foo
> Category:
>
> Event: GroupsShow
> Group: foobar
> Category: foocat
>
> Event: GroupsShowComplete
> EventList: Complete
> ListItems: 2
>
>
> -------------------
> Action: GroupsShowChannels
> -------------------
> Response: Success
> Eventlist: start
> Message: Group channels will follow
>
> Event: GroupsShowChannels
> Group: foo
> Category:
> Channel: IAX2/branch-14981
>
> Event: GroupsShowChannels
> Group: foobar
> Category: foocat
> Channel: IAX2/branch-14981
>
> Event: GroupsShowChannelsComplete
> EventList: Complete
> ListItems: 2
>
>
> -------------------
> Action: GroupsShowVariables
> -------------------
> Response: Success
> Eventlist: start
> Message: Group variables will follow
>
> Event: GroupsShowVariables
> Group: foo
> Category:
> VariablesStart: Variables will follow
> myvar2: 456
> myvar: 123
>
>
> Event: GroupsShowVariables
> Group: foobar
> Category: foocat
> VariablesStart: Variables will follow
> myvar: 123
>
> Event: GroupsShowVariablesComplete
> EventList: Complete
> ListItems: 2
>
>
> -------------------
> Action: GroupVarGet
> Group: foo
> Variable: myvar
> -------------------
> Response: Success
> Message: Result will follow
>
> Event: GroupVarGetResponse
> Group: foo
> Category:
> Variable: myvar
> Value: 123
>
>
> -------------------
> Action: GroupVarSet
> Group: foobar
> Category: foocat
> Variable: something
> Value: 1238091283123
> -------------------
>
> Response: Success
> Message: Variable Set
>
>
> -------------------
> Action: GroupVarSet
> Group: doesntexist
> Category: foocat
> Variable: something
> Value: 1238091283123
> -------------------
>
> Response: Error
> Message: Variable set failed (group doesn't exist)
>
>
> Thanks,
>
> kobaz
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130823/5ea97612/attachment-0001.htm>
More information about the asterisk-dev
mailing list