From mboxrd@z Thu Jan 1 00:00:00 1970 From: mathieu.desnoyers@efficios.com (Mathieu Desnoyers) Date: Wed, 18 May 2016 19:44:43 +0000 (UTC) Subject: [lttng-dev] [PATCH lttng-tools] Fix: unchecked return value In-Reply-To: References: <1463501825-9240-1-git-send-email-mathieu.desnoyers@efficios.com> <483569584.12958.1463599756063.JavaMail.zimbra@efficios.com> Message-ID: <193427135.13005.1463600683528.JavaMail.zimbra@efficios.com> ----- 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 > 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 >>> 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 >>>> --- >>>> 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 >>> 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 >>> >>> >>> 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