Alright, I'll remove the assert for v4. ----- Original Message ----- From: "Mathieu Desnoyers" <mathieu.desnoy...@efficios.com> To: "Jeremie Galarneau" <jeremie.galarn...@efficios.com> Cc: "Yannick Lamarre" <ylama...@efficios.com>, "lttng-dev" <lttng-dev@lists.lttng.org>, "jgalar" <jga...@efficios.com> Sent: Monday, March 25, 2019 11:56:11 AM Subject: Re: [lttng-dev] [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll
----- On Mar 25, 2019, at 11:10 AM, Jeremie Galarneau jeremie.galarn...@efficios.com wrote: > On Fri, 22 Mar 2019 at 11:04, Mathieu Desnoyers > <mathieu.desnoy...@efficios.com> wrote: >> >> ----- On Mar 21, 2019, at 4:25 PM, Jeremie Galarneau >> jeremie.galarn...@efficios.com wrote: >> >> [...] >> >> + assert(idx < events->wait.nb_fd); >> > >> > Not needed. Using assert() is fine to highlight assumptions across >> > interfaces/functions and/or catch catastrophic internal errors. Here, >> > this would be a very local condition that seems to be already >> > enforced. >> > >> > Reworking the 'for' condition to use 'i < events->wait.nb_fd' and >> > breaking out early of the loop is also an option here. >> >> The reason for using assert() here is to ensure that there is no discrepancy >> between the number of active poll fd in the set and the value returned by >> poll indicating that same number of active fds. >> >> We'd need a kernel bug (or memory corruption) to have a mismatch, but I >> thought >> having an assert in there would not hurt, since it might help diagnose issues >> that cross user-kernel boundaries. >> >> Thoughts ? > > Hi, > > I understand the intent, but I don't feel these type of kernel tests/checks > belong in the code. > > However, if there is a specific concern that our use of poll() could highlight > this type of bug (in the kernel or in the interactions with lttng-modules), > we can add tests for that. There is no specific concern. We can remove the assert. Thanks, Mathieu > > Thanks, > Jérémie > >> >> Mathieu >> >> -- >> Mathieu Desnoyers >> EfficiOS Inc. >> http://www.efficios.com > > > -- > Jérémie Galarneau > EfficiOS Inc. > http://www.efficios.com -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
