From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759413AbYDLBPl (ORCPT ); Fri, 11 Apr 2008 21:15:41 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1756096AbYDLBPc (ORCPT ); Fri, 11 Apr 2008 21:15:32 -0400 Received: from x35.xmailserver.org ([64.71.152.41]:51263 "EHLO x35.xmailserver.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756090AbYDLBPb (ORCPT ); Fri, 11 Apr 2008 21:15:31 -0400 X-AuthUser: davidel@xmailserver.org Date: Fri, 11 Apr 2008 18:15:26 -0700 (PDT) From: Davide Libenzi X-X-Sender: davide@alien.or.mcafeemobile.com To: Rusty Russell cc: Linux Kernel Mailing List , Arnd Bergmann , Al Viro Subject: Re: [PATCH] anon_inodes.c cleanups. In-Reply-To: <200804110835.07989.rusty@rustcorp.com.au> Message-ID: References: <200804110835.07989.rusty@rustcorp.com.au> X-GPG-FINGRPRINT: CFAE 5BEE FD36 F65E E640 56FE 0974 BF23 270F 474E X-GPG-PUBLIC_KEY: http://www.xmailserver.org/davidel.asc MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > 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(). The counter reasoning may be that, as today, if anonfds code fails it's almost 100% for ENOMEM, so every other following code will likely fail as well. I still prefer the report-error way instead of the oops generated by not checking return codes, and leave to more critical subsystems to nail the system if necessary. Anyway, I'll let this handle with Al (cc-ed now). The ananofds interface has been changed to remove the inode** and file** parameters (noone but KVM was using them), and Al already has those changes in his vfs tree (plus fixes for KVM, I think). - Davide