On Tue, Nov 27, 2018 at 11:28:31AM -0600, Scott Cheloha wrote:
> 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?
>
> [...]
After lingering on this I missed a bug in my initial review. See inline.
> > 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);
> > +}
> > +
You need to check that the field has an ordering before potentially
modifying rev_order, otherwise a bogus input will have a side effect.
For example, with the current code the input "o -notafield" reverses
the ordering.
> > +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
> >
