| Message ID | 20230217-coverity-fixes-v1-2-043fac896a40@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c407a4ea34a002bda79ea98dcc70c8bc5bc5e1e1 |
| Headers | show |
| Series | cxl/monitor: coverity and misc other fixes | expand |
On 2/17/23 5:40 PM, Vishal Verma wrote: > Static analysis reports that the error unwinding path in monitor_event() > overwrites 'rc' with the return from cxl_event_tracing_disable(). This > masks the actual error code from either epoll_wait() or > cxl_parse_events() which is the one that should be propagated. > > Print a spot error in case there's an error while disabling tracing, but > otherwise retain the rc from the main body of the function. > > Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events") > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > cxl/monitor.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/cxl/monitor.c b/cxl/monitor.c > index 31e6f98..749f472 100644 > --- a/cxl/monitor.c > +++ b/cxl/monitor.c > @@ -130,7 +130,8 @@ static int monitor_event(struct cxl_ctx *ctx) > } > > parse_err: > - rc = cxl_event_tracing_disable(inst); > + if (cxl_event_tracing_disable(inst) < 0) > + err(&monitor, "failed to disable tracing\n"); > event_en_err: > epoll_ctl_err: > close(fd); >
Vishal Verma wrote: > Static analysis reports that the error unwinding path in monitor_event() > overwrites 'rc' with the return from cxl_event_tracing_disable(). This > masks the actual error code from either epoll_wait() or > cxl_parse_events() which is the one that should be propagated. > > Print a spot error in case there's an error while disabling tracing, but > otherwise retain the rc from the main body of the function. > > Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events") > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > cxl/monitor.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/cxl/monitor.c b/cxl/monitor.c > index 31e6f98..749f472 100644 > --- a/cxl/monitor.c > +++ b/cxl/monitor.c > @@ -130,7 +130,8 @@ static int monitor_event(struct cxl_ctx *ctx) > } > > parse_err: > - rc = cxl_event_tracing_disable(inst); > + if (cxl_event_tracing_disable(inst) < 0) > + err(&monitor, "failed to disable tracing\n"); Is this even worth printing? Perhaps just make cxl_event_tracing_disable() return void? Either way: Reviewed-by: Ira Weiny <ira.weiny@intel.com> > event_en_err: > epoll_ctl_err: > close(fd); > > -- > 2.39.1 > >
On Tue, 2023-02-21 at 17:56 -0800, Ira Weiny wrote: > Vishal Verma wrote: > > Static analysis reports that the error unwinding path in monitor_event() > > overwrites 'rc' with the return from cxl_event_tracing_disable(). This > > masks the actual error code from either epoll_wait() or > > cxl_parse_events() which is the one that should be propagated. > > > > Print a spot error in case there's an error while disabling tracing, but > > otherwise retain the rc from the main body of the function. > > > > Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events") > > Cc: Dave Jiang <dave.jiang@intel.com> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > --- > > cxl/monitor.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/cxl/monitor.c b/cxl/monitor.c > > index 31e6f98..749f472 100644 > > --- a/cxl/monitor.c > > +++ b/cxl/monitor.c > > @@ -130,7 +130,8 @@ static int monitor_event(struct cxl_ctx *ctx) > > } > > > > parse_err: > > - rc = cxl_event_tracing_disable(inst); > > + if (cxl_event_tracing_disable(inst) < 0) > > + err(&monitor, "failed to disable tracing\n"); > > Is this even worth printing? Perhaps just make > cxl_event_tracing_disable() return void? I thought about it, but the underlying tracefs_trace_off() returns an int, which is probably why cxl_event_tracing_disable() does too. Having the print satisfies static analyzers that we're checking the return value - other than that I agree it doesn't add much. > > Either way: > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> Thanks Ira! > > > event_en_err: > > epoll_ctl_err: > > close(fd); > > > > -- > > 2.39.1 > > > > > >
diff --git a/cxl/monitor.c b/cxl/monitor.c index 31e6f98..749f472 100644 --- a/cxl/monitor.c +++ b/cxl/monitor.c @@ -130,7 +130,8 @@ static int monitor_event(struct cxl_ctx *ctx) } parse_err: - rc = cxl_event_tracing_disable(inst); + if (cxl_event_tracing_disable(inst) < 0) + err(&monitor, "failed to disable tracing\n"); event_en_err: epoll_ctl_err: close(fd);
Static analysis reports that the error unwinding path in monitor_event() overwrites 'rc' with the return from cxl_event_tracing_disable(). This masks the actual error code from either epoll_wait() or cxl_parse_events() which is the one that should be propagated. Print a spot error in case there's an error while disabling tracing, but otherwise retain the rc from the main body of the function. Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events") Cc: Dave Jiang <dave.jiang@intel.com> Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- cxl/monitor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)