* [lttng-dev] [PATCH lttng-tools] Fix: unchecked return value [not found] <1463501825-9240-1-git-send-email-mathieu.desnoyers@efficios.com> @ 2016-05-18 19:26 ` Jérémie Galarneau 2016-05-18 19:29 ` Mathieu Desnoyers 0 siblings, 1 reply; 5+ messages in thread From: Jérémie Galarneau @ 2016-05-18 19:26 UTC (permalink / raw) [-- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [lttng-dev] [PATCH lttng-tools] Fix: unchecked return value 2016-05-18 19:26 ` [lttng-dev] [PATCH lttng-tools] Fix: unchecked return value Jérémie Galarneau @ 2016-05-18 19:29 ` Mathieu Desnoyers 2016-05-18 19:40 ` Jérémie Galarneau 0 siblings, 1 reply; 5+ messages in thread From: Mathieu Desnoyers @ 2016-05-18 19:29 UTC (permalink / raw) [-- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [lttng-dev] [PATCH lttng-tools] Fix: unchecked return value 2016-05-18 19:29 ` Mathieu Desnoyers @ 2016-05-18 19:40 ` Jérémie Galarneau 2016-05-18 19:44 ` Mathieu Desnoyers 0 siblings, 1 reply; 5+ messages in thread From: Jérémie Galarneau @ 2016-05-18 19:40 UTC (permalink / raw) [-- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [lttng-dev] [PATCH lttng-tools] Fix: unchecked return value 2016-05-18 19:40 ` Jérémie Galarneau @ 2016-05-18 19:44 ` Mathieu Desnoyers 2016-05-18 20:00 ` Jérémie Galarneau 0 siblings, 1 reply; 5+ messages in thread From: Mathieu Desnoyers @ 2016-05-18 19:44 UTC (permalink / raw) [-- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [lttng-dev] [PATCH lttng-tools] Fix: unchecked return value 2016-05-18 19:44 ` Mathieu Desnoyers @ 2016-05-18 20:00 ` Jérémie Galarneau 0 siblings, 0 replies; 5+ messages in thread From: Jérémie Galarneau @ 2016-05-18 20:00 UTC (permalink / raw) [-- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-05-18 20:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1463501825-9240-1-git-send-email-mathieu.desnoyers@efficios.com>
2016-05-18 19:26 ` [lttng-dev] [PATCH lttng-tools] Fix: unchecked return value Jérémie Galarneau
2016-05-18 19:29 ` Mathieu Desnoyers
2016-05-18 19:40 ` Jérémie Galarneau
2016-05-18 19:44 ` Mathieu Desnoyers
2016-05-18 20:00 ` Jérémie Galarneau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox