On Wed, May 18, 2016 at 3:44 PM, Mathieu Desnoyers <[email protected]> wrote: > ----- On May 18, 2016, at 3:40 PM, Jeremie Galarneau > [email protected] wrote: > >> On Wed, May 18, 2016 at 3:29 PM, Mathieu Desnoyers >> <[email protected]> wrote: >>> ----- On May 18, 2016, at 3:26 PM, Jeremie Galarneau >>> [email protected] wrote: >>> >>>> On Tue, May 17, 2016 at 12:17 PM, Mathieu Desnoyers >>>> <[email protected]> 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 <[email protected]> >>>>> --- >>>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> >>>> >>>> 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 _______________________________________________ lttng-dev mailing list [email protected] https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
