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

[jeremie.galarneau@efficios.com (Jérémie Galarneau)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

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?

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