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

[mathieu.desnoyers@efficios.com (Mathieu Desnoyers)]

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

----- 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();....
}

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