Re: [PATCH ndctl 2/3] cxl/monitor: retain error code in monitor_event()

["Verma, Vishal L" <vishal.l.verma@intel.com>]

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