Re: [PATCH] tracing: Propagate errors from remote event bulk updates

[Vincent Donnefort <vdonnefort@google.com>]

On Wed, Jul 15, 2026 at 07:02:01AM +0000, Jackie Liu wrote:
> 2026年7月15日 14:44, "Vincent Donnefort" <vdonnefort@google.com mailto:vdonnefort@google.com?to=%22Vincent%20Donnefort%22%20%3Cvdonnefort%40google.com%3E > 写到:
> 
> 
> > 
> > On Wed, Jul 15, 2026 at 02:04:48PM +0800, Jackie Liu wrote:
> > 
> > > 
> > > From: Jackie Liu <liuyun01@kylinos.cn>
> > >  
> > >  remote_events_dir_enable_write() ignores the return value from
> > >  trace_remote_enable_event(). If a remote rejects an event state change,
> > >  the write therefore reports success even though the affected event remains
> > >  in its previous state.
> > >  
> > >  Keep trying all events, but retain and return the first error. This matches
> > >  __ftrace_set_clr_event_nolock(), which permits partial updates while
> > >  notifying userspace when an operation fails.
> > >  
> > >  Fixes: 775cb093bc50 ("tracing: Add events/ root files to trace remotes")
> > >  Assisted-by: Codex:gpt-5.6-sol
> > >  Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
> > > 
> > I have sent a related improvement for that here [1]
> > 
> > [1] https://lore.kernel.org/all/20260605163825.1762953-3-vdonnefort@google.com/
> 
> Thanks for the pointer.
> 
> I checked the v2 series. Patch 02/18 handles registration failure cleanup,
> and patch 03/18 changes the boolean parsing in
> remote_events_dir_enable_write(), but the loop still ignores errors from
> trace_remote_enable_event().
> 
> Would you prefer this fix to be folded into your series, or should I resend
> a v2 rebased on top of it?

Ha yes my bad. Then no, no need to send it separately. 

> 
> -- 
> Jackie Liu
> 
> > 
> > > 
> > > ---
> > >  kernel/trace/trace_remote.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > >  
> > >  diff --git a/kernel/trace/trace_remote.c b/kernel/trace/trace_remote.c
> > >  index 0f6ef5c36d84..5212e685f0a3 100644
> > >  --- a/kernel/trace/trace_remote.c
> > >  +++ b/kernel/trace/trace_remote.c
> > >  @@ -1149,11 +1149,22 @@ static ssize_t remote_events_dir_enable_write(struct file *filp, const char __us
> > >  guard(mutex)(&remote->lock);
> > >  
> > >  for (i = 0; i < remote->nr_events; i++) {
> > >  + int eret;
> > >  struct remote_event *evt = &remote->events[i];

Could you declare ert after? I have tried to use the reverse christmas tree
everywhere in the trace remote code.

> > >  
> > >  - trace_remote_enable_event(remote, evt, enable);
> > >  + eret = trace_remote_enable_event(remote, evt, enable);
> > >  + /*
> > >  + * Save the first error and return that. Some events
> > >  + * may still have been enabled, but let the user
> > >  + * know that something went wrong.
> > >  + */
> > >  + if (!ret && eret)
> > >  + ret = eret;
> > >  }
> > >  
> > >  + if (ret)
> > >  + return ret;
> > >  +
> > >  return count;
> > >  }
> > >  
> > >  -- 
> > >  2.54.0
> > >
> >