Change in libosmocore[master]: select: Migrate over to poll()

[pespin]

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/20732 )

Change subject: select: Migrate over to poll()
......................................................................


Patch Set 2: Code-Review-1

(7 comments)

https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c 
File src/select.c:

https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@111 
PS2, Line 111: 	llist_for_each_entry(entry, &osmo_fds, list) {
Indeed looks like we'd want to store that into an rb tree rather than a list.


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@158 
PS2, Line 158: 				   g_poll.poll_size + POLL_ALLOC_GRANULARITY);
rather multiply *2?


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@161 
PS2, Line 161: 		memset(p + g_poll.poll_size, 0, POLL_ALLOC_GRANULARITY*sizeof(struct pollfd));
Are you sure you can still access g_poll after talloc_realloc? It may have been moved to another region (p) and previous one not available anymore.
And in that case you actually need to zero everything, not only new bytes.

spacing *


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@305 
PS2, Line 305: 			p->events |= POLLOUT;
|= POLLERR ?


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@312 
PS2, Line 312: /* iterate over first n_fd entries of g_poll.lopp + dispatch */
.poll


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@328 
PS2, Line 328: 			/* FD might have been unregistered meanwhile */
Are you sure this can happen?


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@363 
PS2, Line 363: 	rc = poll(g_poll.poll, n_poll, polling ? 0 : osmo_timers_nearest_ms());
Please let's keep both implementations around and decide at compile time which to use. This has several advantatges:
* Debug possible main-loop related bugs due to poll/select
* Allow to run libosmocore on any system supporting select() (virtually all big ones I'm aware of).

It makes no sense to drop support for it given that we already do support it and it's well tested.



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

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I9e80da68a144b36926066610d0d3df06abe09bca
Gerrit-Change-Number: 20732
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge at osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Mon, 19 Oct 2020 09:50:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201019/d00f8231/attachment.htm>