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

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

[Jérémie Galarneau]

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

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.


Thanks!
Jérémie


> +       }
>  }
>
>  /*
> --
> 2.1.4
>



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

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

[Mathieu Desnoyers]

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

----- 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 ?

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

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

[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

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

[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

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

[Jérémie Galarneau]

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

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