Messages in this thread | | | Date | Sun, 13 Apr 2008 13:37:59 -0700 (PDT) | From | Davide Libenzi <> | Subject | Re: [PATCH] anon_inodes.c cleanups. |
| |
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
| |