From owner-svn-src-head@FreeBSD.ORG Fri Jun 15 02:06:51 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (unknown [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 69B52106566B; Fri, 15 Jun 2012 02:06:51 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail01.syd.optusnet.com.au (mail01.syd.optusnet.com.au [211.29.132.182]) by mx1.freebsd.org (Postfix) with ESMTP id DBED08FC12; Fri, 15 Jun 2012 02:06:50 +0000 (UTC) Received: from c122-106-171-232.carlnfd1.nsw.optusnet.com.au (c122-106-171-232.carlnfd1.nsw.optusnet.com.au [122.106.171.232]) by mail01.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q5F26bGx018816 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 15 Jun 2012 12:06:43 +1000 Date: Fri, 15 Jun 2012 12:06:32 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pawel Jakub Dawidek In-Reply-To: <201206141521.q5EFLwtT002192@svn.freebsd.org> Message-ID: <20120615111325.T999@besplex.bde.org> References: <201206141521.q5EFLwtT002192@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: r237070 - head/sys/kern 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: Fri, 15 Jun 2012 02:06:51 -0000 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