[lttng-dev] [PATCH lttng-tools] Fix: unchecked return value

[Jérémie Galarneau]

On Wed, May 18, 2016 at 3:44 PM, Mathieu Desnoyers
<mathieu.desnoyers at efficios.com> wrote:
> ----- On May 18, 2016, at 3:40 PM, Jeremie Galarneau jeremie.galarneau at efficios.com wrote:
>
>> On Wed, May 18, 2016 at 3:29 PM, Mathieu Desnoyers
>> <mathieu.desnoyers at efficios.com> wrote:
>>> ----- On May 18, 2016, at 3:26 PM, Jeremie Galarneau
>>> jeremie.galarneau at efficios.com wrote:
>>>
>>>> On Tue, May 17, 2016 at 12:17 PM, Mathieu Desnoyers
>>>> <mathieu.desnoyers at efficios.com> wrote:
>>>>> Found by Coverity:
>>>>>
>>>>> CID 1019971 (#1 of 1): Unchecked return value from library
>>>>> (CHECKED_RETURN)2. check_return: Calling posix_fadvise(outfd,
>>>>> orig_offset - stream->max_sb_size, stream->max_sb_size, 4) without
>>>>> checking return value. This library function may fail and return an
>>>>> error code.
>>>>>
>>>>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
>>>>> ---
>>>>>  src/common/consumer/consumer.c | 6 ++++--
>>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
>>>>> index cb05a1e..e22de4d 100644
>>>>> --- a/src/common/consumer/consumer.c
>>>>> +++ b/src/common/consumer/consumer.c
>>>>> @@ -1262,8 +1262,10 @@ void lttng_consumer_sync_trace_file(struct
>>>>> lttng_consumer_stream *stream,
>>>>>          * defined. So it can be expected to lead to lower throughput in
>>>>>          * streaming.
>>>>>          */
>>>>> -       posix_fadvise(outfd, orig_offset - stream->max_sb_size,
>>>>> -                       stream->max_sb_size, POSIX_FADV_DONTNEED);
>>>>> +       if (posix_fadvise(outfd, orig_offset - stream->max_sb_size,
>>>>> +                       stream->max_sb_size, POSIX_FADV_DONTNEED)) {
>>>>> +               DBG("Ignoring posix_fadvise() error: %s.", strerror(errno));
>>>>
>>>> posix_fadvise() does not set errno on error, it returns the error code directly.
>>>> I have merged an alternative fix as:
>>>>
>>>> commit c7a78aabfa0491974d4ffc188d72eb1e67c7344e
>>>> Author: Jérémie Galarneau <jeremie.galarneau at gmail.com>
>>>> Date:   Tue May 17 12:17:05 2016 -0400
>>>>
>>>>    Fix: unchecked posix_fadvise() return value
>>>>
>>>>    Found by Coverity:
>>>>
>>>>    CID 1019971 (#1 of 1): Unchecked return value from library
>>>>    (CHECKED_RETURN)2. check_return: Calling posix_fadvise(outfd,
>>>>    orig_offset - stream->max_sb_size, stream->max_sb_size, 4) without
>>>>    checking return value. This library function may fail and return an
>>>>    error code.
>>>>
>>>>    Signed-off-by: Jérémie Galarneau <jeremie.galarneau at efficios.com>
>>>>
>>>>
>>>> I also set this to WARN() since we'd probably want to know if we're
>>>> misusing posix_fadvise or passing a bad FD, etc.
>>>
>>> On our FreeBSD port, our posix_fadvise wrapper returns an
>>> error. Since we don't care about reporting this as a warning, I chose
>>> DBG() here.
>>>
>>> Thoughts ?
>>
>> Good point. I agree with the use of "return -ENOSYS" in the other wrappers,
>> but given the nature of posix_fadvise(), I would change the behavior of this
>> wrapper to a no-op (return 0).
>>
>> What do you think?
>
> Well it's just "not implemented" ;)
>
> I would keep the return -ENOSYS, and in the caller:
>
> if (ret == -ENOSYS) {
>   DBG();....
> } else {
>   errno = -ret;
>   PERROR();....
> }

Looks good to me. I'll add this in.

Thanks,
Jérémie

>
> Thoughts ?
>
> Thanks,
>
> Mathieu
>
>>
>> Jérémie
>>
>>>
>>> Thanks,
>>>
>>> Mathieu
>>>
>>>>
>>>>
>>>> Thanks!
>>>> Jérémie
>>>>
>>>>
>>>>> +       }
>>>>>  }
>>>>>
>>>>>  /*
>>>>> --
>>>>> 2.1.4
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Jérémie Galarneau
>>>> EfficiOS Inc.
>>>> http://www.efficios.com
>>>
>>> --
>>> 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



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com