Re: [PATCH] anon_inodes.c cleanups.

[Davide Libenzi]

On Sun, 13 Apr 2008, Rusty Russell wrote:

> On Saturday 12 April 2008 11:15:26 Davide Libenzi wrote:
> > On Fri, 11 Apr 2008, Rusty Russell wrote:
> > > Arnd pointed me at anon_inode_getfd(), and the code annoyed me enough
> > > to send this patch.
> > >
> > > Mainly because the init routine carefully checks for errors, then panics
> > > (because we shouldn't run out of memory at boot).  Unfortunately, it's
> > > actually worse than simply oopsing, where we'd know what had failed.
> > >
> > > 1) anon_inode_inode can be read_mostly, same as anon_inode_mnt.
> >
> > Sure.
> >
> > > 3) anon_inode_mkinode has one caller, so it's a little confusing.
> >
> > Hmm? The function groups the code necessary to create the anonfds inode.
> > If every function that has one call site would be inlined, we'd have
> > monster long functions. Functions also have the purpose to group some code
> > that does some task, into a single unit (and the function name hopefully
> > gives an hint about what's doing). The compiler (not that in this case
> > really matter, since it's not even a slow-path, is a once-run path) may
> > take care of inlining, if sees that appropriate.
> 
> If you'd called it, say, "setup_anon_inode()", it would be fine.  It seems 
> overly generic unless you planned on calling it elsewhere.

That's fine with me. I'll wait for Al's tree to get merged and I'll change 
the name (and the read_mostly bits).



> > > 2) The IS_ERR(anon_inode_inode) check is unneeded, since we panic on
> > >    boot if that were true.
> > > 4) Don't clean up before panic.
> > > 5) Panic gives less information than an oops would, plus is untested.
> >
> > I remember we changed the failure-path of anonfds a couple of times along
> > the way, but I can't find email traces about why we did it.
> > So, I prefer error-checked code instead of oopses, and given that the
> > anonfds subsystem is not a required one for most of the components of the
> > kernel/userspace, I'd rather prefer to drop the panic().
> 
> We've seen this debate before, and I'm firmly on the "don't turn oopses into  
> errors on boot paths" side.  I know others disagree.
> 
> Given that it should never happen, I'd argue the highest priority minimal 
> amount of code, and second is ease of debugging if it ever did happen to 
> someone.  Oopsing has those features.

This, I'd prefer to have the bounce error back to do_initcalls() instead 
of nailing the system (given the non criticality of the anonfds subsystem). 




- Davide


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/