Kevin Constantine wrote:
> This patch to nfsstat.c allows for an optional argument to --sleep or
> -Z. This optional argument is the interval at which differences in
> the nfsstat block are printed.
>
> [...]
>
> Moving to a listed output format instead of the traditional nfsstat
> output makes it trivial with a simple grep to watch the stats that
> you really care about and ignore the rest.
Perhaps you could make that output format change a separate option, in a
separate patch from the optional argument to --sleep. That way we can
argue their merits separately. Also, a user could get the new output
format without specifying --sleep=N; if the new format is useful it's
worth having that.
>
>
> @@ -223,7 +230,9 @@ void usage(char *name)
> -v, --verbose, --all\tSame as '-o all'\n\
> -r, --rpc\t\tShow RPC statistics\n\
> -n, --nfs\t\tShow NFS statistics\n\
> - -Z, --sleep\t\tSaves stats, pauses, diffs current and saved\n\
> + -Z[#], --sleep[=#] Saves stats, pauses, diffs current and saved.\n\
> + if # is provided, stats will be output every\n\
> + # seconds\n\
> -S, --since file\tShows difference between current stats and those
> in 'file'\n\
> --version\t\tShow program version\n\
> --help\t\tWhat you just did\n\
This help text was already pretty clunky and clearly written by a
programmer. Perhaps you could re-express it while you're here.
> @@ -245,7 +254,7 @@ static struct option longopts[] =
>
> @@ -258,6 +267,7 @@ main(int argc, char **argv)
>
> @@ -279,7 +289,7 @@ main(int argc, char **argv)
>
> @@ -311,6 +321,9 @@ main(int argc, char **argv)
>
> @@ -384,7 +397,7 @@ main(int argc, char **argv)
>
All this option handling looks good.
> @@ -404,7 +417,33 @@ main(int argc, char **argv)
> diff_stats(clientinfo_tmp, clientinfo, 0);
> }
> }
> + if(sleep_time) {
> + while(1) {
> + if (opt_srv) {
> + get_stats(NFSSRVSTAT, serverinfo_tmp , &opt_srv,
> opt_clt, 1);
> + diff_stats(serverinfo_tmp, serverinfo, 1);
> + }
> + if (opt_clt) {
> + get_stats(NFSCLTSTAT, clientinfo_tmp, &opt_clt,
> opt_srv, 0);
> + diff_stats(clientinfo_tmp, clientinfo, 0);
> + }
> + print_stats_list(opt_prt);
> + fflush(stdout);
> +
> + update_old_counters(clientinfo_tmp, clientinfo);
> + sleep(sleep_time);
> + }
> + }
> + else {
> + print_server_stats(opt_srv, opt_prt);
> + print_client_stats(opt_clt, opt_prt);
> + }
The nfs-utils coding style is
if (...) {
...
} else {
...
}
>
> + return 0;
> +}
> +
> +static void
> +print_server_stats(int opt_srv, int opt_prt) {
The nfs-utils coding style puts the opening brace { of a function on
it's own line.
>
>
> @@ -515,10 +556,42 @@ main(int argc, char **argv)
> );
> }
> }
> -
> - return 0;
> }
>
> +static void
> +print_stats_list(int opt_prt) {
>
Looks ok on a quick scan.
>
> +
> + for (i = 0, srvtotal = 0, clttotal = 0; i < nr; i++) {
> + if (srvinfo[i])
> + srvtotal += srvinfo[i];
> + if (cltinfo[i])
> + clttotal += cltinfo[i];
The if()s here are superfluous.
I don't see an update to the manpage.
--
Greg Banks, P.Engineer, SGI Australian Software Group.
the brightly coloured sporks of revolution.
I don't speak for SGI.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
