Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 15 Jun 2012 12:06:32 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Pawel Jakub Dawidek <pjd@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r237070 - head/sys/kern
Message-ID:  <20120615111325.T999@besplex.bde.org>
In-Reply-To: <201206141521.q5EFLwtT002192@svn.freebsd.org>
References:  <201206141521.q5EFLwtT002192@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 14 Jun 2012, Pawel Jakub Dawidek wrote:

> Log:
>  Style fixes.
>
>  Reported by:	bde
>  MFC after:	1 month

Thanks, but these are mostly not what I reported, and are mostly backwards.

> Modified: head/sys/kern/kern_descrip.c
> ==============================================================================
> --- head/sys/kern/kern_descrip.c	Thu Jun 14 14:38:55 2012	(r237069)
> +++ head/sys/kern/kern_descrip.c	Thu Jun 14 15:21:57 2012	(r237070)
> @@ -102,7 +102,7 @@ __FBSDID("$FreeBSD$");
>
> static MALLOC_DEFINE(M_FILEDESC, "filedesc", "Open file descriptor table");
> static MALLOC_DEFINE(M_FILEDESC_TO_LEADER, "filedesc_to_leader",
> -		     "file desc to leader structures");
> +    "file desc to leader structures");

This is correct, though possibly incomplete (perhaps there should be a
tab instead of a space after `static').

> static MALLOC_DEFINE(M_SIGIO, "sigio", "sigio structures");
>
> MALLOC_DECLARE(M_FADVISE);
> @@ -114,22 +114,21 @@ static uma_zone_t file_zone;
> #define DUP_FIXED	0x1	/* Force fixed allocation */
> #define DUP_FCNTL	0x2	/* fcntl()-style errors */

Not fixed (space instead of tab after #define, and some smaller problems).

>
> -static int	closefp(struct filedesc *fdp, int fd, struct file *fp,
> +static int closefp(struct filedesc *fdp, int fd, struct file *fp,
>     struct thread *td, int holdleaders);

What I reported was the broken indentation of the continuation line.
The indentation starts relative to the function name (see stdlib.h),
but was started relative to column 0.

The change preserves this bug, and breaks the tabbing for the previous
line.  Without that tabbing, the function names don't line up, and
continuation indentation relative to them would be similarly misaligned
and thus worse than useless.

Perhaps there should also be a space after `static', to line up the
function types, but that was not used in old code so it is not a
regression to leave it alone.

> -static int	do_dup(struct thread *td, int flags, int old, int new,
> +static int do_dup(struct thread *td, int flags, int old, int new,
>     register_t *retval);

> -static int	fd_first_free(struct filedesc *, int, int);
> -static int	fd_last_used(struct filedesc *, int);
> -static void	fdgrowtable(struct filedesc *, int);

The patch is hard to read since it breaks all the tabs.

What I pointed out for these 3 prototypes is that they are missing
function parameters, unlike all the other prototypes in this file.

I also pointed out that some prototypes are unsorted.

> -static void	fdunused(struct filedesc *fdp, int fd);
> -static void	fdused(struct filedesc *fdp, int fd);
> -static int	fill_vnode_info(struct vnode *vp, struct kinfo_file *kif);
> -static int	fill_socket_info(struct socket *so, struct kinfo_file *kif);
> -static int	fill_pts_info(struct tty *tp, struct kinfo_file *kif);
> -static int	fill_pipe_info(struct pipe *pi, struct kinfo_file *kif);
> -static int	fill_procdesc_info(struct procdesc *pdp,
> -    struct kinfo_file *kif);
> -static int	fill_shm_info(struct file *fp, struct kinfo_file *kif);
> +static int fd_first_free(struct filedesc *fdp, int low, int size);
> +static int fd_last_used(struct filedesc *fdp, int size);
> +static void fdgrowtable(struct filedesc *fdp, int nfd);
> +static void fdunused(struct filedesc *fdp, int fd);
> +static void fdused(struct filedesc *fdp, int fd);
> +static int fill_pipe_info(struct pipe *pi, struct kinfo_file *kif);
> +static int fill_procdesc_info(struct procdesc *pdp, struct kinfo_file *kif);
> +static int fill_pts_info(struct tty *tp, struct kinfo_file *kif);
> +static int fill_shm_info(struct file *fp, struct kinfo_file *kif);
> +static int fill_socket_info(struct socket *so, struct kinfo_file *kif);
> +static int fill_vnode_info(struct vnode *vp, struct kinfo_file *kif);

I also pointed out namespace errors.  Probably all functions in this file
(except syscall entry points) should have a name beginning with fd.  Some
old ones do, and are named fdfoo().  Some not so old ones are slightly
gratuitously different and are named fd_foo().  Many newer ones are
named quite differently and much worse as fill_bar().  do_dup() is an
old mistake in the same direction (put a generic verb first in the name
to combine namespaces from unrelated subsystem).  But fixing these old
mistakes is beyond the scope of these fixes.

>
> /*
>  * A process is initially started out with NDFILE descriptors stored within
> @@ -183,10 +182,10 @@ struct filedesc0 {
>  */
> volatile int openfiles;			/* actual number of open files */
> struct mtx sigio_lock;		/* mtx to protect pointers to sigio */
> -void	(*mq_fdclose)(struct thread *td, int fd, struct file *fp);
> +void (*mq_fdclose)(struct thread *td, int fd, struct file *fp);

This had a tab in it.  Now the tab is not so useful, since there are no
nearby declarations to line up nicely with.  The nearby declarations of
openfiles and sigio lock were formatted without tabs.  The would have
needed 2 tabs to line up, since their type declaration is longer than
7 characters.

>
> /* A mutex to protect the association between a proc and filedesc. */
> -static struct mtx	fdesc_mtx;
> +static struct mtx fdesc_mtx;

Another type declaration longer than 7 characters, but tabbing was used for
it.

This declaration is also unsorted (it isn't with the other static data
declarations at the top of the file, and those are also missing tabbing).

>
> /*
>  * If low >= size, just return low. Otherwise find the first zero bit in the
>

indent(1) with suitable parameters can fix some of these bugs.  OTOH, it is
hard to vary the parameters to tell indent(1) to _not_ fix these bugs so as
to use it to focus on more serious bugs.  With my default parameters, it
gives:

% --- kern_descrip.c.BAK	2012-06-15 01:40:51.447682000 +0000
% +++ kern_descrip.c	2012-06-15 01:40:51.455672000 +0000
% @@ -103,2 +103,3 @@
% -static MALLOC_DEFINE(M_FILEDESC, "filedesc", "Open file descriptor table");
% -static MALLOC_DEFINE(M_FILEDESC_TO_LEADER, "filedesc_to_leader",
% +static	MALLOC_DEFINE(M_FILEDESC, "filedesc", "Open file descriptor table");
% +static 
% +MALLOC_DEFINE(M_FILEDESC_TO_LEADER, "filedesc_to_leader",

Here it fixes the corrupted tab after `static'.  It is conservative about
this, and only uses tabs for short storage class specifiers and type
declarators.

It doesn't understand the MALLOC_DEFINE() obfuscation of a declaration,
and messes up the second one.

% @@ -106 +107 @@
% -static MALLOC_DEFINE(M_SIGIO, "sigio", "sigio structures");
% +static	MALLOC_DEFINE(M_SIGIO, "sigio", "sigio structures");

Similarly.

% @@ -114,2 +115,2 @@
% -#define DUP_FIXED	0x1	/* Force fixed allocation */
% -#define DUP_FCNTL	0x2	/* fcntl()-style errors */
% +#define DUP_FIXED	0x1		/* Force fixed allocation */
% +#define DUP_FCNTL	0x2		/* fcntl()-style errors */

It fixes these too.

It makes unwanted changes to the comment indentation, since it parameters
have to choose between -c33 or -c41 and both of these are in common use
(I choose -c41 since this is slightly more common, but the above was
formatted with -c33).

% @@ -117 +118,2 @@
% -static int closefp(struct filedesc *fdp, int fd, struct file *fp,
% +static int 
% +closefp(struct filedesc *fdp, int fd, struct file *fp,

Here it starts mangling prototypes.  It doesn't understand prototypes.

% @@ -119 +121,2 @@
% -static int do_dup(struct thread *td, int flags, int old, int new,
% +static int 
% +do_dup(struct thread *td, int flags, int old, int new,

It mangles this too.

Strangely, it doesn't touch any of the other prototypes.  The above 2
are the only multi-line ones.  Apparently they look like functions to
indent.

% @@ -157,2 +160,2 @@
% -	struct file	**ft_table;
% -	SLIST_ENTRY(freetable) ft_next;
% +	struct file **ft_table;
% +		SLIST_ENTRY(freetable) ft_next;

Here it removes the fancy indentation for the first field of a struct
member member name, since fancy indentation (anything other than a
single tab after a short type declaration) is too hard.

Then it is confused by the SLIST_ENTRY() obfuscation of a type declaration.

% @@ -166 +169 @@
% -	struct	filedesc fd_fd;
% +	struct filedesc fd_fd;

Here it fixes the mindless tabbing after the first word in a long type
declaration.  This tabbing used to be normal in KNF.  It was typically
between `struct' and the struct tag, as here.  The FreeBSD replacement
is to mostly not use tabs in declarations with long type names, and
`struct foo' always gives a long type name.

% @@ -170 +173 @@
% -	SLIST_HEAD(,freetable) fd_free;
% +		SLIST_HEAD(, freetable) fd_free;

It is confused as usual by macro obfuscations.

% @@ -175 +178 @@
% -	struct	file *fd_dfiles[NDFILE];
% +	struct file *fd_dfiles[NDFILE];

Another fix for old bogusness for a struct member (in the same struct
as above).  The tabs gave worse-than-useless alignment.

% @@ -184,2 +187,2 @@
% -struct mtx sigio_lock;		/* mtx to protect pointers to sigio */
% -void (*mq_fdclose)(struct thread *td, int fd, struct file *fp);
% +struct mtx sigio_lock;			/* mtx to protect pointers to sigio */
% +void    (*mq_fdclose) (struct thread *td, int fd, struct file *fp);

Here it undoes one of your recent changes, but makes messes.

% @@ -207 +210 @@
% -		mask = ~(~(NDSLOTTYPE)0 >> (NDENTRIES - (low % NDENTRIES)));
% +		mask = ~(~(NDSLOTTYPE) 0 >> (NDENTRIES - (low % NDENTRIES)));

Here it is confused by the type name not being spelled with a _t and not
being a parameter.

% [...]

Despite all the bugs in indent(1) and kern_descrip.c, indent only produces
another 381 lines of diffs, with about 1/3 clearly correct and 1/3 to
make messes of fancily formatted data declarations.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120615111325.T999>