[Asterisk-code-review] res parking: Add dialplan functions for lot state and channel (asterisk[master])

[Kevin Harwell]

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>