Re: [PATCH] nfsstat.c: Print diff stats every N seconds

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Mar 12, 2009 at 10:46:40PM -0700, Kevin Constantine wrote:
> Greg Banks wrote:
>> 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.
>>
> Should the --list option patch be written against the --sleep[#] patch,  
> or against the current repo?

Just make it 2 patches that apply one after the other; I don't think the
order matters much, so I'd just do whatever's most convenient for you.

The revised patch looks fine to me, thanks.  Steve's the nfs-utils guy
(steved@redhat.com), so include him on the email when you've got a
version you want applied.

--b.

>
>
>>>
>>> @@ -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.
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html