[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 3: Code-Review-1

(3 comments)

> > Patch Set 3:
 > >
 > > What's the difference between
 > > ${EXTENSION_STATE(${parking_lot}@${parking_space})} = INUSE
 > > and
 > > ${PARK_OCCUPIED(${parking_space},${parking_lot})}
 > > ?
 > 
 > I think the difference there is that EXTENSION_STATE would require
 > that the parking lot extension be hinted. I don't know that's
 > guaranteed to be the case all the time. Might be some other odd
 > state considerations there I'd have to think through, but this at
 > least seemed to keep it fairly clean for us.
 > 
 > Would you guys prefer removing PARK_OCCUPIED in favor of relying on
 > EXTENSION_STATE?

If the EXTENSION_STATE version is sufficient then I lean toward that. Could you also not get similar from PARK_GET_CHANNEL too? If that function returns a channel name then it's occupied, otherwise if it returns nothing then the slot would be been empty/unoccupied correct?

https://gerrit.asterisk.org/#/c/9375/3/res/parking/parking_bridge_features.c
File res/parking/parking_bridge_features.c:

https://gerrit.asterisk.org/#/c/9375/3/res/parking/parking_bridge_features.c@820
PS3, Line 820:         if (!ast_strlen_zero(args.parking_space)) {
             :                 if (ast_str_to_uint(args.parking_space, &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;
             :                 }
             :         }
This section is using spaces instead of tabs. Please use tabs for indentions.


https://gerrit.asterisk.org/#/c/9375/3/res/parking/parking_controller.c
File res/parking/parking_controller.c:

https://gerrit.asterisk.org/#/c/9375/3/res/parking/parking_controller.c@168
PS3, Line 168: 	RAII_VAR(struct parked_user *, user, NULL, ao2_cleanup);
             : 
             : 	if (target < 0) {
             : 		user = ao2_callback(lot->parked_users, 0, NULL, NULL);
             : 	} else {
             : 		user = ao2_callback(lot->parked_users, 0, retrieve_parked_user_targeted, &target);
             : 	}
             : 
             : 	if (!user) {
             : 		return NULL;
             : 	}
             : 
             : 	/* Bump the ref count by 1 since the RAII_VAR will eat the reference otherwise */
             : 	ao2_ref(user, +1);
             : 	return user;
Remove the RAII_VAR use here as it adds extra overhead in this case. You can then also remove the ref bump at the end. Actually at that point this whole thing could be collapsed into a single line (if you wanted to that is):

return target < 0 ? ao2_callback(....) : ao2_callback(...)


https://gerrit.asterisk.org/#/c/9375/3/res/parking/res_parking.h
File res/parking/res_parking.h:

https://gerrit.asterisk.org/#/c/9375/3/res/parking/res_parking.h@199
PS3, Line 199:  * \since 12.0.0
Remove this since this method is being added now.



-- 
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: 3
Gerrit-Owner: Joshua Elson <joshelson at gmail.com>
Gerrit-Reviewer: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Elson <joshelson at gmail.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Mon, 09 Jul 2018 19:30:04 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180709/50b88354/attachment.html>