From owner-svn-src-head@FreeBSD.ORG Mon Apr 13 08:50:32 2009 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D628C106566B; Mon, 13 Apr 2009 08:50:32 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 5C6D28FC12; Mon, 13 Apr 2009 08:50:31 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-107-120-227.carlnfd1.nsw.optusnet.com.au (c122-107-120-227.carlnfd1.nsw.optusnet.com.au [122.107.120.227]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id n3D8oRH6028758 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 13 Apr 2009 18:50:30 +1000 Date: Mon, 13 Apr 2009 18:50:27 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Rick Macklem In-Reply-To: <200904121904.n3CJ4RTa035520@svn.freebsd.org> Message-ID: <20090413170824.N52089@delplex.bde.org> References: <200904121904.n3CJ4RTa035520@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r190971 - in head/sys: conf modules modules/nfssvc nfsserver X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Apr 2009 08:50:33 -0000 On Sun, 12 Apr 2009, Rick Macklem wrote: > Log: > Change nfsserver so that it uses the nfssvc() system call provided > in sys/nfs/nfs_nfssvc.c by registering with it using the > nfsd_call_nfsserver function pointer. Also, add the build glue for > nfs_nfssvc.c optionally based on "nfsserver" and also as a loadable > module. > ... > Modified: head/sys/nfsserver/nfs.h > ============================================================================== > --- head/sys/nfsserver/nfs.h Sun Apr 12 17:43:41 2009 (r190970) > +++ head/sys/nfsserver/nfs.h Sun Apr 12 19:04:27 2009 (r190971) > @@ -40,6 +40,8 @@ > #include "opt_nfs.h" > #endif > > +#include > + Nested includes are style bugs or bugs (namespace pollution). > @@ -447,6 +442,13 @@ int nfsrv_symlink(struct nfsrv_descript > struct mbuf **mrq); > int nfsrv_write(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp, > struct mbuf **mrq); > +/* > + * #ifdef _SYS_SYSPROTO_H_ so that it is only defined when sysproto.h > + * has been included, so that "struct nfssvc_args" is defined. > + */ > +#ifdef _SYS_SYSPROTO_H_ > +int nfssvc_nfsserver(struct thread *, struct nfssvc_args *); > +#endif None of sysproto.h, or an ifdef for it, or a declaration of "struct nfssvc_args", or a comment about this are needed here. Only a declaration of "struct nfssvc_args *" in the correct scope is needed here. This should be provided by declaring it in file scope without a comment, as is done in hundreds or thousands of similar cases. grep shows 304 similar cases in /sys/sys/*.h, though only 7 cases matching "^struct.*args;". Forward declarations of a syscall args struct are relatively rarely needed since the args struct rarely need to be used outside of the main file that implements the syscall. "struct nfssvc_args *" is self-declared in the prototype but this is not in file scope so it is inconsistent unless it repeats a file-scope declaration that is always visible. > Modified: head/sys/nfsserver/nfs_srvkrpc.c > ============================================================================== > --- head/sys/nfsserver/nfs_srvkrpc.c Sun Apr 12 17:43:41 2009 (r190970) > +++ head/sys/nfsserver/nfs_srvkrpc.c Sun Apr 12 19:04:27 2009 (r190971) > ... > @@ -163,25 +166,14 @@ int32_t (*nfsrv3_procs[NFS_NPROCS])(stru > * - sockaddr with no IPv4-mapped addresses > * - mask for both INET and INET6 families if there is IPv4-mapped overlap > */ > -#ifndef _SYS_SYSPROTO_H_ > -struct nfssvc_args { > - int flag; > - caddr_t argp; > -}; > -#endif These sysproto ifdefs and their contents were to make the struct definitions visible nearby (so that you don't have to look in sysproto.h and decipher its macros) in such a way that they can be checked for compatibility with the actual declarations. This replaces the 4.4BSD method of declaring structs in pseudo-code in some places where args structs are used, in the form struct nfssvc_args /* { int flag; caddr_t argp; } */ *uap; in 4.4BSD-Lite1, and in the form struct nfssvc_args /* { syscallarg(int) flag; syscallarg(caddr_t) argp; } */ *uap; in 4.4BSD-Lite2, to match the change of adding syscallarg() in the actual declarations. 4.4BSD uses this most consistently for vfs syscalls and doesn't use it in any form for nfssvc_args -- it just declares the complete nfssvc_args struct unportably only once just before it is used, the same as for most syscall args structs in Net/2. NetBSD as of 2005 uses essentially exactly the version with syscallargs() above. The 4.4BSD-Lite1 method is bad since it is difficult to decipher pseudo-code so that it can be automatically compared with the actual declarations (the pseudo-code of course rots unless it is checked automatically). The 4.4BSD-Lite2 method is worse because it requires deciphering the syscallarg() macros to understand the pseudo-code. The 4.4BSD-Lite2 method also doesn't actually work in general, since it doesn't provide any control over padding between the fields and it only provides limited control over the layout within the fields. FreeBSD uses more complicated macros to provide more control over both. Automatic checking of the pseudo-code has not been implemented for FreeBSD and is further away than when the comments were changed to ifdefs. All of the pseudo-code has rotted perfectly by not keeping up with any of the changes to use macros in the actual code. The pseudo-code would be too hard to decipher if it had kept up. Urk, I just notice that FreeBSD still does this most bogusly for vfs syscalls. It has sysproto ifdefs in most cases, and also has 4.4BSD-Lite1-style comments in most cases, mainly as a result of mismerging 4.4BSD-Lite2. FreeBSD had moved all of the pseudo-code in comments to code in ifdefs. Then the merge brought back most of the comments in their modified form (with syscallarg()). Later, FreeBSD only removed the syscallarg()'s. Later, a few syscalls like lchflags() were imported from NetBSD. These usually have the pseudo-code in comments but not code in ifdefs. Similar pseudo-code is used in some cases (mainly in older code?) for automatically-generated args structs for vnops. It looks like: % /* % * Open called. % */ % /* ARGSUSED */ % static int % ufs_open(ap) % struct vop_open_args /* { % struct vnode *a_vp; % int a_mode; % struct ucred *a_cred; % struct thread *a_td; % } */ *ap; % { Bugs in this include: - the first comment adds less than nothing - /* ARGSUSED */ is bitrot. It was to quiet lint before args structs were used. Then vnops were passed a long list of args, often with many unused placeholder args. - the a_mode line has an extra space or space instead of a tab. With a new-style function declaration, the corresponding pseudo-code would be even harder to format normally. Many syscall entry points have a rotted /* ARGSUSED */ due to incomplete removal of their only unused arg (retval). Bruce