[Asterisk-code-review] res parking: Add dialplan functions for lot state and channel (asterisk[master])
Kevin Harwell
asteriskteam at digium.com
Fri Jul 6 18:07:11 CDT 2018
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/9375 )
Change subject: res_parking: Add dialplan functions for lot state and channel
......................................................................
Patch Set 1: Code-Review-1
(10 comments)
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c
File res/parking/parking_bridge_features.c:
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c@47
PS1, Line 47: <function name="CHECK_PARKING_SLOT" language="en_US">
I lean toward renaming this function to "IS_PARKING_SLOT_OCCUPIED".
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c@72
PS1, Line 72: <description>
: <para>This function returns true if the current parking slot
: in the parking lot space is occupied.</para>
: </description>
This descriptions appears to be copy/pasted from the other function. Update this to reflect what GET_PARKING_SLOT_CHANNEL does.
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c@772
PS1, Line 772: if (ast_strlen_zero(args.parking_space) || sscanf(args.parking_space, "%30d", &space) != 1) {
Probably want to check for a negative space value?
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c@779
PS1, Line 779:
Remove extra whitespace
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c@782
PS1, Line 782: } else {
: ret = 0;
: }
No need to set 'ret' again here as you did it above. Alternatively you could just copy the string here in this if/else branch and remove the variable altogether.
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c@788
PS1, Line 788: return 0;
Remove the extra spaces at the end of the line.
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c@823
PS1, Line 823: if (!ast_strlen_zero(args.parking_space)) {
: if (sscanf(args.parking_space, "%d", &space) != 1 || space < 0) {
Might be able to use 'ast_str_to_uint' here instead of the sscanf nomenclature? As it converts and checks for negatives. (see conversions.h)
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c@809
PS1, Line 809: if (args.argc < 2) {
: /* Didn't receive enough arguments to do anything */
: ast_log(LOG_ERROR,
: "Usage: %s(<parking_space>,<parking_lot>)\n",
: function);
: return -1;
: }
:
: lot = parking_lot_find_by_name(args.parking_lot);
: if (!lot) {
: ast_log(LOG_ERROR, "Could not find parking lot: '%s'\n", args.parking_lot);
: return -1;
: }
:
: if (!ast_strlen_zero(args.parking_space)) {
: if (sscanf(args.parking_space, "%d", &space) != 1 || space < 0) {
: ast_log(LOG_ERROR, "value '%s' for parking_space argument is invalid. Must be an integer greater than 0.\n", args.parking_space);
: return -1;
: }
: }
A lot of code is duplicated in the other function. It might make sense to combine the duplicated code into a single function called by both of these.
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c@823
PS1, Line 823: if (!ast_strlen_zero(args.parking_space)) {
: if (sscanf(args.parking_space, "%d", &space) != 1 || space < 0) {
: ast_log(LOG_ERROR, "value '%s' for parking_space argument is invalid. Must be an integer greater than 0.\n", args.parking_space);
: return -1;
: }
: }
:
: pu = parking_lot_retrieve_parked_user(lot, space);
: if (!pu) {
: return -1;
: }
tabify this block.
https://gerrit.asterisk.org/#/c/9375/1/res/parking/parking_bridge_features.c@838
PS1, Line 838: return 0;
remove extra whitespace.
--
To view, visit https://gerrit.asterisk.org/9375
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idba6ae55b8a53f734238cb3d995cedb95c0e7b74
Gerrit-Change-Number: 9375
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Elson <joshelson at gmail.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Fri, 06 Jul 2018 23:07:11 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180706/ce57b68d/attachment-0001.html>
More information about the asterisk-code-review
mailing list