Re: top: allow reverse sort order
On Wed, Nov 28, 2018 at 12:07:37AM +0100, Klemens Nanni wrote:
> On Tue, Nov 27, 2018 at 03:52:52PM -0600, Scott Cheloha wrote:
> > > > 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.
> Good catch.
>
> Updated diff to fix that, rest unchanged.
>
> Note how an empty field is silently treated as the default field
> "state", but that's an independent issue I'd like to address in a
> separate diff for string_index().
>
> OK?
ok cheloha@
> 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 - 1.57
> +++ display.c 27 Nov 2018 21:09:56 -
> @@ -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"
> "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 - 1.95
> +++ machine.c 27 Nov 2018 21:09:56 -
> @@ -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
>
Re: top: allow reverse sort order
On Wed, Nov 28, 2018 at 12:07:37AM +0100, Klemens Nanni wrote: > Note how an empty field is silently treated as the default field > "state", but that's an independent issue I'd like to address in a > separate diff for string_index(). Not a problem of string_index() actually. We consistently handle empty input for all commands silently as no-op.
Re: top: allow reverse sort order
On Tue, Nov 27, 2018 at 03:52:52PM -0600, Scott Cheloha wrote:
> > > 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.
Good catch.
Updated diff to fix that, rest unchanged.
Note how an empty field is silently treated as the default field
"state", but that's an independent issue I'd like to address in a
separate diff for string_index().
OK?
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 - 1.57
+++ display.c 27 Nov 2018 21:09:56 -
@@ -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"
"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 - 1.95
+++ machine.c 27 Nov 2018 21:09:56 -
@@ -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
Re: top: allow reverse sort order
On Tue, Nov 27, 2018 at 11:28:31AM -0600, Scott Cheloha wrote:
> 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.
I thought about a toggle as well but went with the "-" prefix semantics
since the existing toggles we currently have (`C', `h', `H') are
distinct features not related to filters like `g' or `o'. That is,
currently you cannot toggle something like "only user kn/all but kn".
> > 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 - 1.57
> > +++ display.c 24 Nov 2018 14:09:38 -
> > @@ -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.
Keeping "o" is consistent with the rest: "g+ selects all commands" and
"u -user hides user" for example.
> > @@ -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
We're also not "ducking" it for `u' in the option parsing. Admittedly,
this is inconsistent but I'd like to keep both in the command loop. The
command line options are parsed once so with an erroneous `-U --kn'
users might see "--kn: unknown user" only once on startup when in fact
"-kn" is the unknown user.
Stripping the dash in error messages from the command line which is
likely used repeatedly at runtime ensures that the correct username
is printed.
> void setorder(const char *);
>
> to keep the messages in sync, but that should be done in a subsequent diff.
So I if at all, I suggest stripping the dash in the option parsing
routine as well for consistency in separate diff.
Re: top: allow reverse sort order
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 - 1.57
> > +++ display.c 24 Nov 2018 14:09:38 -
> > @@ -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 - 1.95
> > +++ machine.c 24 Nov 2018 14:47:32 -
> > @@ -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
Re: top: allow reverse sort order
On Tue, Nov 27, 2018 at 07:38:59PM +, Jason McIntyre wrote: > On Tue, Nov 27, 2018 at 11:28:31AM -0600, Scott Cheloha wrote: > > > @@ -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. > > > > hi. > > i've never heard of "ordering" as a noun like this. i think klemens' > language was fine (and simpler). I can live with that. kn: just make sure the two error messages match up.
Re: top: allow reverse sort order
On Tue, Nov 27, 2018 at 11:28:31AM -0600, Scott Cheloha wrote: > > @@ -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. > hi. i've never heard of "ordering" as a noun like this. i think klemens' language was fine (and simpler). jmc
Re: top: allow reverse sort order
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 - 1.57
> +++ display.c 24 Nov 2018 14:09:38 -
> @@ -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 - 1.95
> +++ machine.c 24 Nov 2018 14:47:32 -
> @@ -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
top: allow reverse sort order
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?
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 - 1.57
+++ display.c 24 Nov 2018 14:09:38 -
@@ -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"
"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 - 1.95
+++ machine.c 24 Nov 2018 14:47:32 -
@@ -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 =
