On Sat, Nov 24, 2018 at 04:06:34PM +0100, Klemens Nanni wrote:
> Sometimes I want to see certain programs with least amount of memory,
> so this diff implements `o -field' to sort in reverse order.
>
> The logic is straight forward:
>
> 1. merge common code from argument and command loops into new setorder()
> 2. introduce global state `rev_order' (set in the helper)
> 3. move identical code to set up process objects from compare_*()
> functions into SETORDER macro using global boolean
>
> compare_*() are used by qsort(3). To sort in reverse, the macro simply
> swaps the objects used by the ORDERKEY_* macros. That is it inverts the
> comparison from `p1 > p2' into `p2 > p1' respectively `p1 < p2'.
>
> Works fine for all available fields on amd64, no behaviour change for
> "normal" order.
>
> Feedback? Objections?
I mentioned in private that I disliked the lack of parentheses in the macro
invocations, but after rereading I see that that's the style in use in the
code already, so you're fine, my bad.
No objections here to the feature in general. We already support reversing
select orderings in systat(1), which I've found useful in practice, so this
is not without precedent and is consistent with at least one other monitoring
tool in userland.
I think it's a bit unfortunate that 'r' in interactive mode is already assigned
to renice the process. If we did that here it'd be consistent with systat(1)'s
interactive keybindings and a boon for overall UX. But 'r' has mean "renice"
since before top(1) was even imported, so at this late date I don't think we can
change the binding for 'r'. However, adding an 'R' shortcut to reverse the
ordering
for the current primary key might be a reasonable addition in a later diff.
Some nits inline for documentation, error messages. Otherwise ok cheloha@.
... and I might be wrong about those nits, too. Definitely have jmc@ weigh in
on the nits and the docs changes in general.
> Index: display.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/top/display.c,v
> retrieving revision 1.57
> diff -u -p -r1.57 display.c
> --- display.c 17 Nov 2018 23:10:08 -0000 1.57
> +++ display.c 24 Nov 2018 14:09:38 -0000
> @@ -817,7 +817,8 @@ show_help(void)
> "I | i - toggle the display of idle processes\n"
> "k [-sig] pid - send signal `-sig' to process `pid'\n"
> "n|# count - show `count' processes\n"
> - "o field - specify sort order (size, res, cpu, time, pri, pid,
> command)\n"
> + "o [-]field - specify sort order (size, res, cpu, time, pri, pid,
> command)\n"
> + " (o -field sorts in reverse)\n"
"o" isn't needed, as contextually you're talking about the 'o' shortcut.
Maybe "-field reverses order" or "'-' prefix reverses sort order"?
> "P pid - highlight process `pid' (P+ switches highlighting
> off)\n"
> "p pid - display process by `pid' (p+ selects all
> processes)\n"
> "q - quit\n"
> Index: machine.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/top/machine.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 machine.c
> --- machine.c 17 Nov 2018 23:10:08 -0000 1.95
> +++ machine.c 24 Nov 2018 14:47:32 -0000
> @@ -602,6 +602,8 @@ static unsigned char sorted_state[] =
> 1 /* zombie */
> };
>
> +extern int rev_order;
> +
> /*
> * proc_compares - comparison functions for "qsort"
> */
> @@ -631,6 +633,17 @@ static unsigned char sorted_state[] =
> #define ORDERKEY_CMD \
> if ((result = strcmp(p1->p_comm, p2->p_comm)) == 0)
>
> +/* remove one level of indirection and set sort order */
> +#define SETORDER do { \
> + if (rev_order) { \
> + p1 = *(struct kinfo_proc **) pp2; \
> + p2 = *(struct kinfo_proc **) pp1; \
> + } else { \
> + p1 = *(struct kinfo_proc **) pp1; \
> + p2 = *(struct kinfo_proc **) pp2; \
> + } \
> + } while (0)
> +
> /* compare_cpu - the comparison function for sorting by cpu percentage */
> static int
> compare_cpu(const void *v1, const void *v2)
> @@ -640,9 +653,7 @@ compare_cpu(const void *v1, const void *
> struct kinfo_proc *p1, *p2;
> int result;
>
> - /* remove one level of indirection */
> - p1 = *(struct kinfo_proc **) pp1;
> - p2 = *(struct kinfo_proc **) pp2;
> + SETORDER;
>
> ORDERKEY_PCTCPU
> ORDERKEY_CPUTIME
> @@ -663,9 +674,7 @@ compare_size(const void *v1, const void
> struct kinfo_proc *p1, *p2;
> int result;
>
> - /* remove one level of indirection */
> - p1 = *(struct kinfo_proc **) pp1;
> - p2 = *(struct kinfo_proc **) pp2;
> + SETORDER;
>
> ORDERKEY_MEM
> ORDERKEY_RSSIZE
> @@ -686,9 +695,7 @@ compare_res(const void *v1, const void *
> struct kinfo_proc *p1, *p2;
> int result;
>
> - /* remove one level of indirection */
> - p1 = *(struct kinfo_proc **) pp1;
> - p2 = *(struct kinfo_proc **) pp2;
> + SETORDER;
>
> ORDERKEY_RSSIZE
> ORDERKEY_MEM
> @@ -709,9 +716,7 @@ compare_time(const void *v1, const void
> struct kinfo_proc *p1, *p2;
> int result;
>
> - /* remove one level of indirection */
> - p1 = *(struct kinfo_proc **) pp1;
> - p2 = *(struct kinfo_proc **) pp2;
> + SETORDER;
>
> ORDERKEY_CPUTIME
> ORDERKEY_PCTCPU
> @@ -732,9 +737,7 @@ compare_prio(const void *v1, const void
> struct kinfo_proc *p1, *p2;
> int result;
>
> - /* remove one level of indirection */
> - p1 = *(struct kinfo_proc **) pp1;
> - p2 = *(struct kinfo_proc **) pp2;
> + SETORDER;
>
> ORDERKEY_PRIO
> ORDERKEY_PCTCPU
> @@ -754,9 +757,7 @@ compare_pid(const void *v1, const void *
> struct kinfo_proc *p1, *p2;
> int result;
>
> - /* remove one level of indirection */
> - p1 = *(struct kinfo_proc **) pp1;
> - p2 = *(struct kinfo_proc **) pp2;
> + SETORDER;
>
> ORDERKEY_PID
> ORDERKEY_PCTCPU
> @@ -777,9 +778,7 @@ compare_cmd(const void *v1, const void *
> struct kinfo_proc *p1, *p2;
> int result;
>
> - /* remove one level of indirection */
> - p1 = *(struct kinfo_proc **) pp1;
> - p2 = *(struct kinfo_proc **) pp2;
> + SETORDER;
>
> ORDERKEY_CMD
> ORDERKEY_PCTCPU
> Index: top.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/top/top.1,v
> retrieving revision 1.70
> diff -u -p -r1.70 top.1
> --- top.1 2 Nov 2018 12:46:10 -0000 1.70
> +++ top.1 23 Nov 2018 21:50:39 -0000
> @@ -35,7 +35,7 @@
> .Op Fl 1bCHIinqSu
> .Op Fl d Ar count
> .Op Fl g Ar string
> -.Op Fl o Ar field
> +.Op Fl o Oo - Oc Ns Ar field
> .Op Fl p Ar pid
> .Op Fl s Ar time
> .Op Fl U Oo - Oc Ns Ar user
> @@ -137,13 +137,16 @@ mode.
> This is identical to
> .Em batch
> mode.
> -.It Fl o Ar field
> +.It Fl o Oo - Oc Ns Ar field
> Sort the process display area using the specified
> .Ar field
> as the primary key.
> The field name is the name of the column as seen in the output,
> but in lower case.
> The
> +.Sq -
> +prefix reverses the order.
My understanding is that an "order" is the actual progression of
a sequence, while an "ordering" refers to the underlying function
or rule that gives rise to a given "order" when applied to a set.
So, I *think* you want
"reverses the ordering for the field"
or something similar, but I'm not positive.
> +The
> .Ox
> version of
> .Nm
> @@ -327,10 +330,13 @@ This acts similarly to the command
> Show
> .Ar count
> processes.
> -.It o Ar field
> +.It o Oo - Oc Ns Ar field
> Sort the process display area using the specified
> .Ar field
> as the primary key.
> +The
> +.Sq -
> +prefix reverses the order.
Same thing here.
"reverses the ordering for the field"
> Values are the same as for the
> .Fl o
> flag, as detailed above.
> Index: top.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/top/top.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 top.c
> --- top.c 17 Nov 2018 23:10:08 -0000 1.97
> +++ top.c 24 Nov 2018 15:04:04 -0000
> @@ -74,6 +74,7 @@ extern int ncpuonline;
>
> extern int (*proc_compares[])(const void *, const void *);
> int order_index;
> +int rev_order;
>
> int displays = 0; /* indicates unspecified */
> char do_unames = Yes;
> @@ -93,6 +94,9 @@ int combine_cpus = 0;
> char topn_specified = No;
> #endif
>
> +struct system_info system_info;
> +struct statics statics;
> +
> /*
> * these defines enumerate the "strchr"s of the commands in
> * command_chars
> @@ -129,12 +133,20 @@ usage(void)
> extern char *__progname;
>
> fprintf(stderr,
> - "usage: %s [-1bCHIinqSu] [-d count] [-g string] [-o field] "
> + "usage: %s [-1bCHIinqSu] [-d count] [-g string] [-o [-]field] "
> "[-p pid] [-s time]\n\t[-U [-]user] [number]\n",
> __progname);
> }
>
> static int
> +getorder(char *field)
> +{
> + rev_order = field[0] == '-';
> +
> + return string_index(rev_order ? field + 1 : field, statics.order_names);
> +}
> +
> +static int
> filteruser(char buf[])
> {
> const char *errstr;
> @@ -311,9 +323,6 @@ parseargs(int ac, char **av)
> }
> }
>
> -struct system_info system_info;
> -struct statics statics;
> -
> int
> main(int argc, char *argv[])
> {
> @@ -381,20 +390,9 @@ main(int argc, char *argv[])
>
> /* determine sorting order index, if necessary */
> if (order_name != NULL) {
> - if ((order_index = string_index(order_name,
> - statics.order_names)) == -1) {
> - char **pp, msg[512];
> -
> - snprintf(msg, sizeof(msg),
> - "'%s' is not a recognized sorting order",
> - order_name);
> - strlcat(msg, ". Valid are:", sizeof(msg));
> - pp = statics.order_names;
> - while (*pp != NULL) {
> - strlcat(msg, " ", sizeof(msg));
> - strlcat(msg, *pp++, sizeof(msg));
> - }
> - new_message(MT_delayed, msg);
> + if ((order_index = getorder(order_name)) == -1) {
> + new_message(MT_delayed,
> + " %s: unrecognized sorting order", order_name);
And again. Maybe:
"unrecognized sort ordering"
> order_index = 0;
> }
> }
> @@ -879,10 +877,10 @@ rundisplay(void)
> new_message(MT_standout,
> "Order to sort: ");
> if (readline(tempbuf, sizeof(tempbuf)) > 0) {
> - if ((i = string_index(tempbuf,
> - statics.order_names)) == -1) {
> + if ((i = getorder(tempbuf)) == -1) {
> new_message(MT_standout,
> " %s: unrecognized sorting order",
> + tempbuf[0] == '-' ? tempbuf + 1 :
> tempbuf);
You aren't ducking the '-' during init so I don't think you should hide it here.
You could even refactor the two into something like
void setorder(const char *);
to keep the messages in sync, but that should be done in a subsequent diff.
Also, the "order" vs. "ordering" thing here.
> no_command = Yes;
> } else
> ===================================================================
> Stats: --- 45 lines 1516 chars
> Stats: +++ 49 lines 1189 chars
> Stats: 4 lines
> Stats: -327 chars
>
