Page MenuHomeFreeBSD

vmstat: fix style(9) violations and bump WARNS
ClosedPublic

Authored by kaktus on Nov 24 2017, 4:08 PM.

Details

Summary

This is the first version of the larger cleanup in vmstat code. I fixed many style formatting and sorting issues, removed unused variables and function, initialised struct nlist correctly. This allowed to bump WARNS to 6.

I'd take a look at the PRI formatting macros and bde@ comments on xo_emit in later versions.

Submited by: Pawel Biernacki <pawel.biernacki@gmail.com>
Sponsored by: Mysterious Code Ltd.

Test Plan

No functional changes should occur.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kaktus created this revision.Nov 24 2017, 4:08 PM
kib added inline comments.Nov 26 2017, 9:52 AM
usr.bin/vmstat/vmstat.c
90 ↗(On Diff #35694)

What do you mean by 'initialize nlist correctly' ? Why the list of all members for struct nlist is needed ?

I think that the less intrusive and more vivid change is to switch to the designated intializers both for structure members and for array elements, if changing this at all.

131 ↗(On Diff #35694)

Why this reshuffling is needed ?

186 ↗(On Diff #35694)

Perhaps extract not strictly formatting changes into separate review.

496 ↗(On Diff #35694)

This blank line is excessive.

505 ↗(On Diff #35694)

Since you clear the code, move declaration into the function' locals block.

775 ↗(On Diff #35694)

There too.

1173 ↗(On Diff #35694)

And there.

1231 ↗(On Diff #35694)

Is this line too long ?

1243 ↗(On Diff #35694)

Merge all int's local declarations.

1312 ↗(On Diff #35694)

Reorder locals: pointer first, then integers according to the required alignment.

1321 ↗(On Diff #35694)

Would not note this more times. Also, perhaps use u_long, unsigned long is too long.

1697 ↗(On Diff #35694)

Blank line is still needed even if no locals.

kaktus added inline comments.Nov 29 2017, 4:18 PM
usr.bin/vmstat/vmstat.c
131 ↗(On Diff #35694)

In the name of sorting by size and then alphabetical as taught by style(9).

186 ↗(On Diff #35694)

Leaving it like this don't allow bumping WARNS because of -Wmissing-variable-declarations.

kaktus updated this revision to Diff 35961.Nov 29 2017, 4:21 PM

Address @kib comments. Also more and more style(9) cleanup like moving variables from body to top of the functions, removing unneeded empty lines, PRIu64 changed to %ju with uintmax_t cast, sorting, etc...

kaktus edited the summary of this revision. (Show Details)Nov 29 2017, 5:45 PM
kib added a comment.Dec 1 2017, 6:22 PM
commit 1bdb99ca310aa8b145bdc139aa59ba5373e0c306
Author: Konstantin Belousov <kib@freebsd.org>
Date:   Fri Dec 1 20:20:22 2017 +0200

    More style fixes.

diff --git a/usr.bin/vmstat/vmstat.c b/usr.bin/vmstat/vmstat.c
index f69b1d6d83b..288dc5c096f 100644
--- a/usr.bin/vmstat/vmstat.c
+++ b/usr.bin/vmstat/vmstat.c
@@ -171,7 +171,7 @@ static struct __vmmeter {
 } sum, osum;
 
 #define	VMSTAT_DEFAULT_LINES	20	/* Default number of `winlines'. */
-static volatile sig_atomic_t wresized;		/* Tty resized, when non-zero. */
+static volatile sig_atomic_t wresized;		/* Tty resized when non-zero. */
 static int winlines = VMSTAT_DEFAULT_LINES; /* Current number of tty rows. */
 
 static int	aflag;
@@ -216,10 +216,10 @@ static char	**getdrivedata(char **);
 int
 main(int argc, char *argv[])
 {
+	char *bp, *buf, *memf, *nlistf;
 	float f;
 	int bufsize, c, len, reps, todo;
 	unsigned int interval;
-	char *bp, *buf, *memf, *nlistf;
 	char errbuf[_POSIX2_LINE_MAX];
 
 	memf = nlistf = NULL;
@@ -229,7 +229,7 @@ main(int argc, char *argv[])
 
 	argc = xo_parse_args(argc, argv);
 	if (argc < 0)
-		return argc;
+		return (argc);
 
 	while ((c = getopt(argc, argv, "ac:fhHiM:mN:n:oPp:stw:z")) != -1) {
 		switch (c) {
@@ -285,7 +285,8 @@ main(int argc, char *argv[])
 #ifdef notyet
 			todo |= TIMESTAT;
 #else
-			xo_errx(EX_USAGE, "sorry, -t is not (re)implemented yet");
+			xo_errx(EX_USAGE,
+			    "sorry, -t is not (re)implemented yet");
 #endif
 			break;
 		case 'w':
@@ -318,8 +319,9 @@ retry_nlist:
 	if (kd != NULL && (c = kvm_nlist(kd, namelist)) != 0) {
 		if (c > 0) {
 			bufsize = 0, len = 0;
+
 			/*
-			 * 'cnt' was renamed to 'vm_cnt'. If 'vm_cnt' is not
+			 * 'cnt' was renamed to 'vm_cnt'.  If 'vm_cnt' is not
 			 * found try looking up older 'cnt' symbol.
 			 * */
 			if (namelist[X_SUM].n_type == 0 &&
@@ -327,6 +329,7 @@ retry_nlist:
 				namelist[X_SUM].n_name = "_cnt";
 				goto retry_nlist;
 			}
+
 			for (c = 0; c < (int)(nitems(namelist)); c++)
 				if (namelist[c].n_type == 0)
 					bufsize += strlen(namelist[c].n_name)
@@ -428,19 +431,19 @@ getdrivedata(char **argv)
 	num_devices = cur.dinfo->numdevs;
 	generation = cur.dinfo->generation;
 
-	specified_devices = (char **)malloc(sizeof(char *));
+	specified_devices = malloc(sizeof(char *));
 	for (num_devices_specified = 0; *argv; ++argv) {
 		if (isdigit(**argv))
 			break;
 		num_devices_specified++;
-		specified_devices = (char **)realloc(specified_devices,
+		specified_devices = realloc(specified_devices,
 		    sizeof(char *) * num_devices_specified);
 		specified_devices[num_devices_specified - 1] = *argv;
 	}
 	dev_select = NULL;
 
 	if (nflag == 0 && maxshowdevs < num_devices_specified)
-			maxshowdevs = num_devices_specified;
+		maxshowdevs = num_devices_specified;
 
 	/*
 	 * People are generally only interested in disk statistics when
@@ -455,7 +458,6 @@ getdrivedata(char **argv)
 	if ((num_devices_specified == 0) && (num_matches == 0)) {
 		if (devstat_buildmatch(da, &matches, &num_matches) != 0)
 			xo_errx(1, "%s", devstat_errbuf);
-
 		select_mode = DS_SELECT_ADD;
 	} else
 		select_mode = DS_SELECT_ONLY;
@@ -609,8 +611,7 @@ getcpuinfo(u_long *maskp, int *maxidp)
 	long *times;
 	u_long mask;
 	size_t size;
-	int empty, i, j;
-	int maxcpu, maxid, ncpus;
+	int empty, i, j, maxcpu, maxid, ncpus;
 
 	if (kd != NULL)
 		xo_errx(1, "not implemented");
@@ -671,8 +672,7 @@ dovmstat(unsigned int interval, int reps)
 	u_long cpumask;
 	size_t size;
 	time_t uptime, halfuptime;
-	int ncpus, maxid;
-	int rate_adj, retval;
+	int ncpus, maxid, rate_adj, retval;
 
 	uptime = getuptime() / 1000000000LL;
 	halfuptime = uptime / 2;
@@ -755,7 +755,7 @@ dovmstat(unsigned int interval, int reps)
 		case -1:
 			xo_errx(1, "%s", devstat_errbuf);
 			break;
-		case 1: {
+		case 1:
 			num_devices = cur.dinfo->numdevs;
 			generation = cur.dinfo->generation;
 
@@ -776,7 +776,7 @@ dovmstat(unsigned int interval, int reps)
 			default:
 				break;
 			}
-		}
+			break;
 		default:
 			break;
 		}
@@ -815,7 +815,8 @@ dovmstat(unsigned int interval, int reps)
 
 		xo_open_container("paging-rates");
 		xo_emit("{:page-reactivated/%3lu} ",
-		    (unsigned long)rate(sum.v_reactivated - osum.v_reactivated));
+		    (unsigned long)rate(sum.v_reactivated -
+		    osum.v_reactivated));
 		xo_emit("{:paged-in/%3lu} ",
 		    (unsigned long)rate(sum.v_swapin + sum.v_vnodein -
 		    (osum.v_swapin + osum.v_vnodein)));
@@ -865,13 +866,12 @@ printhdr(int maxid, u_long cpumask)
 	int i, num_shown;
 
 	num_shown = MIN(num_selected, maxshowdevs);
-	if (hflag) {
+	if (hflag)
 		xo_emit("{T:procs}  {T:memory}       {T:/page%*s}", 19, "");
-	} else {
+	else
 		xo_emit("{T:procs}     {T:memory}        {T:/page%*s}", 19, "");
-	}
 	if (num_shown > 1)
-		xo_emit(" {T:/disks %*s}", num_shown * 4 - 7, ""); 
+		xo_emit(" {T:/disks %*s}", num_shown * 4 - 7, "");
 	else if (num_shown == 1)
 		xo_emit("   {T:disks}");
 	xo_emit("   {T:faults}      ");
@@ -885,10 +885,10 @@ printhdr(int maxid, u_long cpumask)
 		xo_emit("   {T:cpu}\n");
 	if (hflag) {
 		xo_emit("{T:r} {T:b} {T:w}  {T:avm}   {T:fre}   {T:flt}  {T:re}"
-	    "  {T:pi}  {T:po}    {T:fr}   {T:sr} ");
+		    "  {T:pi}  {T:po}    {T:fr}   {T:sr} ");
 	} else {
 		xo_emit("{T:r} {T:b} {T:w}     {T:avm}     {T:fre}  {T:flt}  "
-	    "{T:re}  {T:pi}  {T:po}    {T:fr}   {T:sr} ");
+		    "{T:re}  {T:pi}  {T:po}    {T:fr}   {T:sr} ");
 	}
 	for (i = 0; i < num_devices; i++)
 		if ((dev_select[i].selected) &&
@@ -1141,8 +1141,7 @@ doforkst(void)
 static void
 devstats(void)
 {
-	long double transfers_per_second;
-	long double busy_seconds;
+	long double busy_seconds, transfers_per_second;
 	long tmp;
 	int di, dn, state;
 
@@ -1156,8 +1155,8 @@ devstats(void)
 
 	xo_open_list("device");
 	for (dn = 0; dn < num_devices; dn++) {
-		if ((dev_select[dn].selected == 0) ||
-		    (dev_select[dn].selected > maxshowdevs))
+		if (dev_select[dn].selected == 0 ||
+		    dev_select[dn].selected > maxshowdevs)
 			continue;
 
 		di = dev_select[dn].position;
@@ -1289,14 +1288,13 @@ read_intrcnts(unsigned long **intrcnts)
 
 static void
 print_intrcnts(unsigned long *intrcnts, unsigned long *old_intrcnts,
-    char *intrnames, unsigned int nintr,
-    size_t istrnamlen, long long period_ms)
+    char *intrnames, unsigned int nintr, size_t istrnamlen, long long period_ms)
 {
-	uint64_t inttotal, old_inttotal, total_count, total_rate;
 	unsigned long *intrcnt, *old_intrcnt;
+	char *intrname;
+	uint64_t inttotal, old_inttotal, total_count, total_rate;
 	unsigned long count, rate;
 	unsigned int i;
-	char *intrname;
 
 	inttotal = 0;
 	old_inttotal = 0;
@@ -1327,11 +1325,11 @@ print_intrcnts(unsigned long *intrcnts, unsigned long *old_intrcnts,
 static void
 dointr(unsigned int interval, int reps)
 {
-	long long old_uptime, uptime, period_ms;
-	size_t clen, inamlen, istrnamlen;
 	unsigned long *intrcnts, *old_intrcnts;
+	char *intrname, *intrnames;
+	long long period_ms, old_uptime, uptime;
+	size_t clen, inamlen, istrnamlen;
 	unsigned int nintr;
-	char *intrnames, *intrname;
 
 	old_intrcnts = NULL;
 	uptime = getuptime();

is my current diff over your patch. If you are fine with this, I will commit the cumulative change.

kaktus added a comment.Dec 1 2017, 6:32 PM

I'm fine with it. I've one question about what is the correct way of sorting local variables? style(9) use two different versions (or I'm reading it wrong):

int
main(int argc, char *argv[])
{
        char *ep;
        long num;
        int ch;

vs.

struct foo one, *two;
double three;
int *four, five;
char *six, seven, eight, nine, ten, eleven, twelve;
kib added a comment.Dec 1 2017, 6:36 PM

Apparently tinderbox is busted. Additionally at least the following chunk is needed.

commit 07e3214722f4cb9928a8ef908e7a4be4a3cb6bcd
Author: Konstantin Belousov <kib@freebsd.org>
Date:   Fri Dec 1 20:34:44 2017 +0200

    Appease gcc.

diff --git a/usr.bin/vmstat/vmstat.c b/usr.bin/vmstat/vmstat.c
index 288dc5c096f..74528ab3d40 100644
--- a/usr.bin/vmstat/vmstat.c
+++ b/usr.bin/vmstat/vmstat.c
@@ -1179,14 +1179,14 @@ devstats(void)
 }
 
 static void
-percent(const char *name, double pct, int *over)
+percent(const char *name, double pctv, int *over)
 {
 	int l;
 	char buf[10];
 	char fmt[128];
 
 	snprintf(fmt, sizeof(fmt), " {:%s/%%*s}", name);
-	l = snprintf(buf, sizeof(buf), "%.0f", pct);
+	l = snprintf(buf, sizeof(buf), "%.0f", pctv);
 	if (l == 1 && *over) {
 		xo_emit(fmt, 1, buf);
 		(*over)--;
kib added a comment.Dec 1 2017, 6:38 PM
In D13228#278078, @pawel.biernacki-gmail.com wrote:

I'm fine with it. I've one question about what is the correct way of sorting local variables? style(9) use two different versions (or I'm reading it wrong):

int
main(int argc, char *argv[])
{
        char *ep;
        long num;
        int ch;

vs.

struct foo one, *two;
double three;
int *four, five;
char *six, seven, eight, nine, ten, eleven, twelve;

It is said 'order by alignment required', for whatever arch you consider. This would explain floated order for double.

kib added a comment.Dec 1 2017, 10:30 PM

One more issue, recording it there to keep all diffs against original submission.

commit 2b9fe1c7cf023c6700f69bd25268c8ce04267872
Author: Konstantin Belousov <kib@freebsd.org>
Date:   Fri Dec 1 23:37:17 2017 +0200

    Initialize variable to silence gcc.

diff --git a/usr.bin/vmstat/vmstat.c b/usr.bin/vmstat/vmstat.c
index 74528ab3d40..86f248e054d 100644
--- a/usr.bin/vmstat/vmstat.c
+++ b/usr.bin/vmstat/vmstat.c
@@ -679,6 +679,7 @@ dovmstat(unsigned int interval, int reps)
 	rate_adj = 1;
 	ncpus = 1;
 	maxid = 0;
+	cpumask = 0;
 
 	/*
 	 * If the user stops the program (control-Z) and then resumes it,
This revision was automatically updated to reflect the committed changes.