svn commit: r190971 - in head/sys: conf modules modules/nfssvc nfsserver

[Bruce Evans]

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 <nfs/nfssvc.h>
> +

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