[Asterisk-code-review] res parking: Increase size of parking space numbers (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/9933 )

Change subject: res_parking: Increase size of parking space numbers
......................................................................


Patch Set 2: Code-Review-1

(11 comments)

https://gerrit.asterisk.org/#/c/9933/2/main/cdr.c
File main/cdr.c:

https://gerrit.asterisk.org/#/c/9933/2/main/cdr.c@1612
PS2, Line 1612: 	char park_info[AST_MAX_EXTENSION];	/* ideally: max. parking lot strlen + max. digits of ast_parkingspace_int */
AST_MAX_EXTENSIONS is defined as 80 so you have needlessly shrunk the size of this buffer and potentially caused truncation where it didn't happen before.


https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_applications.c
File res/parking/parking_applications.c:

https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_applications.c@40
PS2, Line 40: /*** DOCUMENTATION
The XML documentation changes here should go in its own patch since they have nothing to do with this new feature and should go into the other branches.


https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_bridge.c
File res/parking/parking_bridge.c:

https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_bridge.c@127
PS2, Line 127: 		preferred_space = ((ast_parkingspace_int) ast_random() * (ast_parkingspace_int) ast_random()) % (lot->cfg->parking_stop - lot->cfg->parking_start + 1);
Only one call to ast_random is needed.  We are determining the offset into the parking lot at this time.  Besides you cannot have a parking lot with more than 2^32 spaces as you would have run out of memory long before that.


https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_bridge_features.c
File res/parking/parking_bridge_features.c:

https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_bridge_features.c@692
PS2, Line 692: 	sprintf(buf, AST_PARKINGSPACE_FORMAT, numeric_value);
             : 	ast_say_digit_str(bridge_channel->chan, buf, "", ast_channel_language(bridge_channel->chan));
Should use your parking_space_announce() macro here.


https://gerrit.asterisk.org/#/c/9933/2/res/parking/res_parking.h
File res/parking/res_parking.h:

https://gerrit.asterisk.org/#/c/9933/2/res/parking/res_parking.h@63
PS2, Line 63: 	ast_parkingspace_int parking_start;                  /*!< First space in the parking lot */
            : 	ast_parkingspace_int parking_stop;                   /*!< Last space in the parking lot */
The EOL comments should be made to line up again.


https://gerrit.asterisk.org/#/c/9933/2/res/parking/res_parking.h@90
PS2, Line 90: 	ast_parkingspace_int next_space;                     /*!< When using parkfindnext, which space we should start searching from next time we park */
The EOL comments should be made to line up again.


https://gerrit.asterisk.org/#/c/9933/2/res/parking/res_parking.h@106
PS2, Line 106: 	ast_parkingspace_int parking_space;                  /*!< Which parking space is used */
The EOL comments should be made to line up again.


https://gerrit.asterisk.org/#/c/9933/2/res/res_parking.c
File res/res_parking.c:

https://gerrit.asterisk.org/#/c/9933/2/res/res_parking.c@453
PS2, Line 453: 	return left->parking_space - right->parking_space;
You are going to have to make this comparison more complicated as you cannot return a long long difference here.

Fortunately you just need to return the standard strcmp() return meanings.
left < right if return value is less than zero
left = right if return value is zero
left > right if return value is greater than zero

so you could return -1, 0, 1 as needed.


https://gerrit.asterisk.org/#/c/9933/2/res/res_parking.c@819
PS2, Line 819: 		snprintf(space, sizeof space, AST_PARKINGSPACE_FORMAT, parkingspace);
Keep the parentheses with sizeof.  It really looks weird without them and they are required with some uses.

sizeof(space)


https://gerrit.asterisk.org/#/c/9933/2/res/res_parking.c@827
PS2, Line 827: 		arguments_string = ast_str_create(strlen(lot_cfg->name) + 1 + sizeof space);
Revert this change.  This is the initial size of the created ast_str.  It will dynamically grow as needed.


https://gerrit.asterisk.org/#/c/9933/2/res/res_parking.c@834
PS2, Line 834: 		if (parking_add_extension(lot_context, 0, space, 1, PARKED_CALL_APPLICATION, ast_str_buffer(arguments_string), parkedcall_registrar_pointer)
             : 			) {
Revert this change.  Coding guidelines want lines wrapped at 90 columns.



-- 
To view, visit https://gerrit.asterisk.org/9933
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: Ifbeee7546d5ab3ea3ce29c68b4810eb4e51decad
Gerrit-Change-Number: 9933
Gerrit-PatchSet: 2
Gerrit-Owner: Karl Brose <karlbrose at gmail.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Wed, 22 Aug 2018 19:26:32 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180822/444bb084/attachment-0001.html>