[Asterisk-code-review] openr2(3/6): Convert r2links to standard Asterisk AST_LIST* (...asterisk[13])

[Oron Peled]

Oron Peled has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11535 )

Change subject: openr2(3/6): Convert r2links to standard Asterisk AST_LIST*
......................................................................


Patch Set 2:

(3 comments)

Thanks for the review.
Please see my notes:
* The primary motivation to use lists instead of the original implementation is to allow dynamic adding/removal of openr2 channels/links.
* As a result, the link numbering may have "holes"

https://gerrit.asterisk.org/#/c/11535/2/channels/chan_dahdi.c 
File channels/chan_dahdi.c:

https://gerrit.asterisk.org/#/c/11535/2/channels/chan_dahdi.c@769 
PS2, Line 769: struct r2link_entry {
> Why did you wrap dahdi_mfcr2 with an r2link_entry instead of just storing it directly in the linked  […]
Wrapping allows to allocate the dahdi_mfcr2 and the list entry in one call.
This technique lower the number of possible error paths (first allocation fails, second allocation fails) to handle.

It also doesn't "cost" anything, so I don't see what we lose.


https://gerrit.asterisk.org/#/c/11535/2/channels/chan_dahdi.c@11949 
PS2, Line 11949: 		struct r2link_entry *tmp = NULL;
               : 		int new_idx = r2links_count + 1;
               : 		int i;
               : 		for (i = 1; i <= r2links_count; i++) {
               : 			int i_unused = 1;
               : 			AST_LIST_TRAVERSE(&r2links, tmp, list) {
               : 				if (i == tmp->mfcr2.index) {
               : 					i_unused = 0;
               : 					break;
               : 				}
               : 			}
               : 			if (i_unused) {
               : 				new_idx = i;
               : 				break;
               : 			}
               : 		}
> This loop appears to be unneeded. […]
The assumption that the next free index would be last index + 1 only holds if there are no removals of channels/links.

The next patches adds exactly this capability -- i.e: dynamic addition/removal of channels and openr2 links.

That was the main motivation to convert the original array into a linked list.


https://gerrit.asterisk.org/#/c/11535/2/channels/chan_dahdi.c@19466 
PS2, Line 19466: 		int x = 0;
> variable 'x' appears to be unused aside from incrementing below, so can be removed.
The only use of variable 'x' is to allow better error reporting.
If ast_pthread_create() fails, the user knows which iteration failed.

(yes, that's not an important use-case, so if you prefer I can remove this)



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11535
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: Ibcb2401515a58782a1488c0b9efbed201c3f3a17
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 2
Gerrit-Owner: Oron Peled <oron.peled at xorcom.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Oron Peled <oron.peled at xorcom.com>
Gerrit-Reviewer: Tzafrir Cohen <tzafrir.cohen at xorcom.com>
Gerrit-Comment-Date: Mon, 22 Jul 2019 14:07:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Comment-In-Reply-To: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190722/b97fc711/attachment-0001.html>