Re: [tracepoint] cargo-culting considered harmful...

Re: [tracepoint] cargo-culting considered harmful...

[Mathieu Desnoyers]

* Al Viro (viro@ZenIV.linux.org.uk) wrote:
> On Fri, Jan 25, 2013 at 09:49:53AM -0500, Mathieu Desnoyers wrote:
> > static
> > void lttng_enumerate_task_fd(struct lttng_session *session,
> >                 struct task_struct *p, char *tmp)
> > {
> >         struct fdtable *fdt;
> >         struct file *filp;
> >         unsigned int i;
> >         const unsigned char *path;
> > 
> >         task_lock(p);
> >         if (!p->files)
> >                 goto unlock_task;
> >         spin_lock(&p->files->file_lock);
> >         fdt = files_fdtable(p->files);
> >         for (i = 0; i < fdt->max_fds; i++) {
> >                 filp = fcheck_files(p->files, i);
> >                 if (!filp)
> >                         continue;
> >                 path = d_path(&filp->f_path, tmp, PAGE_SIZE);
> >                 /* Make sure we give at least some info */
> >                 trace_lttng_statedump_file_descriptor(session, p, i,
> >                         IS_ERR(path) ?
> >                                 filp->f_dentry->d_name.name :
> >                                 path);
> >         }
> >         spin_unlock(&p->files->file_lock);
> > unlock_task:
> >         task_unlock(p);
> > }
> 
> *cringe*
> 
> a) yes, it needs d_lock for that ->d_name access
> b) iterate_fd() is there for purpose; use it, instead of open-coding the
> damn loop.  Something like
> 
> struct ctx {
> 	char *page;
> 	struct lttng_session *session,
> 	struct task_struct *p;
> };
> 	
> static int dump_one(void *p, struct file *file, unsigned fd)
> {
> 	struct ctx *ctx = p;
> 	const char *s = d_path(&file->f_path, ctx->page, PAGE_SIZE);
> 	struct dentry *dentry;
> 	if (!IS_ERR(s)) {
> 		trace_lttng_statedump_file_descriptor(ctx->session, ctx->p, fd, s);
> 		return 0;
> 	}
> 	/* Make sure we give at least some info */
> 	dentry = file->f_path.dentry;
> 	spin_lock(&dentry->d_lock);
> 	trace_lttng_statedump_file_descriptor(ctx->session, ctx->p, fd,
> 		dentry->d_name);
> 	spin_unlock(&dentry->d_lock);
> 	return 0;
> }
> 
> ...
> 	task_lock(p);
> 	iterate_fd(p->files, 0, dump_one, &(struct ctx){tmp, session, p});
> 	task_unlock(p);
> 
> assuming it wouldn't be better to pass tmp/session/p as the single pointer
> to struct in the first place - I don't know enough about the callers of
> that sucker to tell.  And yes, iterate_fd() will DTRT if given NULL as the
> first argument.  The second argument is "which descriptor should I start
> from?", callback is called for everything present in the table starting from
> that place until it returns non-zero or the end of table is reached...

Thanks !! Modulo a couple of trivial nits, I've integrated your
suggestions. I'm creating a lttng_iterate_fd() wrapper for older kernels
(yeah.. we deal with kernels back to 2.6.32).

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com