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 -0000 1.57
> +++ display.c 27 Nov 2018 21:09:56 -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"
> "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 27 Nov 2018 21:09:56 -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 27 Nov 2018 21:09:56 -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.
> +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.
> 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 27 Nov 2018 22:54:42 -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,24 @@ 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)
> +{
> + int i, r = field[0] == '-';
> +
> + i = string_index(r ? field + 1 : field, statics.order_names);
> + if (i != -1)
> + rev_order = r;
> +
> + return i;
> +}
> +
> +static int
> filteruser(char buf[])
> {
> const char *errstr;
> @@ -311,9 +327,6 @@ parseargs(int ac, char **av)
> }
> }
>
> -struct system_info system_info;
> -struct statics statics;
> -
> int
> main(int argc, char *argv[])
> {
> @@ -381,20 +394,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);
> order_index = 0;
> }
> }
> @@ -879,10 +881,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);
> no_command = Yes;
> } else
>
