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

Mathieu Desnoyers
2013-01-25T17:30:34Z

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