<p><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c">File src/select.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@108">Patch Set #2, Line 108:</a> <code style="font-family:monospace,monospace">osmo_fd_find</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is it a public API?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">yes, I thought I'd export the symbol (but then forgot to add it to a header file).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@111">Patch Set #2, Line 111:</a> <code style="font-family:monospace,monospace"> llist_for_each_entry(entry, &osmo_fds, list) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Indeed looks like we'd want to store that into an rb tree rather than a list.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">as stated, we can optimize later. The important point is no ABI nor API breakage.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@155">Patch Set #2, Line 155:</a> <code style="font-family:monospace,monospace">1></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">missing space</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@158">Patch Set #2, Line 158:</a> <code style="font-family:monospace,monospace"> g_poll.poll_size + POLL_ALLOC_GRANULARITY);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">rather multiply *2?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">what would be the advantage? I don't really care much either way.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@161">Patch Set #2, Line 161:</a> <code style="font-family:monospace,monospace"> memset(p + g_poll.poll_size, 0, POLL_ALLOC_GRANULARITY*sizeof(struct pollfd));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Are you sure you can still access g_poll after talloc_realloc? It may have been moved to another reg […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">g_poll itself is a static/global thread-local variable. So g_poll for sure is still valid. g_poll.poll is a pointer to dynamically allocated memory, and I'm not using that one here until I update it one line further below.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@305">Patch Set #2, Line 305:</a> <code style="font-family:monospace,monospace"> p->events |= POLLOUT;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">|= POLLERR ?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@312">Patch Set #2, Line 312:</a> <code style="font-family:monospace,monospace">/* iterate over first n_fd entries of g_poll.lopp + dispatch */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">.poll</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@328">Patch Set #2, Line 328:</a> <code style="font-family:monospace,monospace"> /* FD might have been unregistered meanwhile */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Are you sure this can happen?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">well, look at the old select loop function and it's restart hack / goto. That was exactly added for the fact that one ofd callback has modified the list of file descriptors while we are iterating it. Here we have the advantage that we're not iterating the list while calling the call-back. But still, one callback which we just executed might have closed down some socket (and unregistered the osmo_fd) for a fd which is also marked readable/writable and which we want to dispatch to right now. So I'm pretty sure, yes.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@363">Patch Set #2, Line 363:</a> <code style="font-family:monospace,monospace"> rc = poll(g_poll.poll, n_poll, polling ? 0 : osmo_timers_nearest_ms());</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Please let's keep both implementations around and decide at compile time which to use. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">keeping multiple implementations around has the risk that over time only one of them will be tested all the time, while the other one is not. (I have no intention of exploding our jenkins matrix by another axis to test select vs. poll).</p><p style="white-space: pre-wrap; word-wrap: break-word;">poll() is available on probably at least as many operating systems used today as select(). Also, given the amount of linux specifics we have in libosmocore, it is rather unlikely that we'll encounter a system that supports select but not poll.</p><p style="white-space: pre-wrap; word-wrap: break-word;">We will likely see an epoll implementation (and possibly also before ppoll as it does have sub-millisecond timers) at not too distant point in time, and then we'd have three implementations to maintain (and test).</p><p style="white-space: pre-wrap; word-wrap: break-word;">So yes, we can keep it around as a --disable-poll configure option for some limited amount of time (to manually test regressions/compatibility), but I would consider that only a short term thing for a few months or one year max.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/20732">change 20732</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/20732"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I9e80da68a144b36926066610d0d3df06abe09bca </div>
<div style="display:none"> Gerrit-Change-Number: 20732 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 19 Oct 2020 10:05:00 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>