[lttng-dev] [PATCH lttng-tools v2] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll

[Mathieu Desnoyers]

----- On Mar 25, 2019, at 11:10 AM, Jeremie Galarneau jeremie.galarneau at efficios.com wrote:

> On Fri, 22 Mar 2019 at 11:04, Mathieu Desnoyers
> <mathieu.desnoyers at efficios.com> wrote:
>>
>> ----- On Mar 21, 2019, at 4:25 PM, Jeremie Galarneau
>> jeremie.galarneau at 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