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

[Yannick Lamarre]

Alright, I'll remove the assert for v4.

----- Original Message -----
From: "Mathieu Desnoyers" <mathieu.desnoyers at efficios.com>
To: "Jeremie Galarneau" <jeremie.galarneau at efficios.com>
Cc: "Yannick Lamarre" <ylamarre at efficios.com>, "lttng-dev" <lttng-dev at lists.lttng.org>, "jgalar" <jgalar at 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.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