Messages in this thread | | | Subject | Re: [RFC][PATCH 4/4] tracing/uprobes: Fail to unregister if probe event files are open | From | Steven Rostedt <> | Date | Thu, 01 Aug 2013 10:25:54 -0400 |
| |
On Thu, 2013-08-01 at 16:08 +0200, Oleg Nesterov wrote: > On 07/03, Steven Rostedt wrote: > > > > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org> > > Acked-by: Oleg Nesterov <oleg@redhat.com>
Thanks!
> > > Just a couple of nits in the case you are going to redo this change, > > > Modules do with with the module owner set (automated > > from the VFS layer). > > This logic is dead, I think. > > > The ftrace buffer instances have a ref count added > > to the trace_array when the enabled file is opened > > This is too. >
Yeah, the change log needs an update.
> > -static void cleanup_all_probes(void) > > +static int cleanup_all_probes(void) > > { > > struct trace_uprobe *tu; > > + int ret = 0; > > > > mutex_lock(&uprobe_lock); > > while (!list_empty(&uprobe_list)) { > > tu = list_entry(uprobe_list.next, struct trace_uprobe, list); > > - unregister_trace_uprobe(tu); > > + ret = unregister_trace_uprobe(tu); > > + if (ret) > > + break; > > } > > mutex_unlock(&uprobe_lock); > > + return ret; > > } > > Again, it is not clear what exactly we should do and I won't argue > either way. But note that (with or without this patch) this doesn't > match kprobe's release_all_trace_probes() which checks (tries to, > actually) trace_probe_is_enabled() for every probe first. Perhaps > we should cleanup this later.
Agreed on all accounts. Especially the "we should cleanup this later" part ;-)
> > > static int probes_open(struct inode *inode, struct file *file) > > { > > + int ret = 0; > > + > > if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) > > - cleanup_all_probes(); > > + ret = cleanup_all_probes(); > > + if (ret) > > + return ret; > > Cosmetic, but perhaps it would be a bit more clean to move this check > (with "int ret") under if (WRITE && TRUNC) block. >
Yeah, agreed.
-- Steve
| |