Re: top: merge duplicate code into helper functions
On Wed, 31 Oct 2018 23:38:58 +0100, Klemens Nanni wrote: > New diff free of side-effects in filteruser(), rest unchanged. OK millert@ - todd
Re: top: merge duplicate code into helper functions
On Tue, Oct 30, 2018 at 02:24:46PM -0600, Todd C. Miller wrote:
> I really dislike the side-effect in filteruser(), see below.
> Otherwise looks good.
New diff free of side-effects in filteruser(), rest unchanged.
> > +static int
> > +filteruser(char buf[])
> > +{
> > + char *bufp = buf;
> > + uid_t *uidp;
> > +
> > + if (bufp[0] == '-') {
> > + bufp++[0] = ' ';
>
> Why is this needed, can't you just do "bufp++"?
Done.
The caller rundisplay() now checks for leading dash (again) and just
prints `tempbuf' or `tempbuf + 1' accordingly as done before without
replacing the character.
OK?
Index: top.c
===
RCS file: /cvs/src/usr.bin/top/top.c,v
retrieving revision 1.94
diff -u -p -r1.94 top.c
--- top.c 5 Oct 2018 18:56:57 - 1.94
+++ top.c 31 Oct 2018 22:21:38 -
@@ -83,7 +83,7 @@ int no_command = Yes;
int old_system = No;
int old_threads = No;
int show_args = No;
-pid_t hlpid = -1;
+pid_t hlpid = (pid_t)-1;
int combine_cpus = 0;
#if Default_TOPN == Infinity
@@ -131,6 +131,46 @@ usage(void)
__progname);
}
+static int
+filteruser(char buf[])
+{
+ char *bufp = buf;
+ uid_t *uidp;
+
+ if (bufp[0] == '-') {
+ bufp++;
+ uidp =
+ ps.uid = (pid_t)-1;
+ } else {
+ uidp =
+ ps.huid = (pid_t)-1;
+ }
+
+ return uid_from_user(bufp, uidp);
+}
+
+static int
+filterpid(char buf[], int hl)
+{
+ const char *errstr;
+ int pid;
+
+ pid = strtonum(buf, 0, INT_MAX, );
+ if (errstr != NULL || !find_pid(pid))
+ return -1;
+
+ if (hl == Yes)
+ hlpid = (pid_t)pid;
+ else {
+ if (ps.system == No)
+ old_system = No;
+ ps.pid = (pid_t)pid;
+ ps.system = Yes;
+ }
+
+ return 0;
+}
+
static void
parseargs(int ac, char **av)
{
@@ -150,32 +190,16 @@ parseargs(int ac, char **av)
break;
case 'U': /* display only username's processes */
- if (optarg[0] == '-') {
- if (uid_from_user(optarg+1, ) == -1)
- new_message(MT_delayed,
- "%s: unknown user", optarg);
- else
- ps.uid = (uid_t)-1;
- } else if (uid_from_user(optarg, ) == -1)
+ if (filteruser(optarg) == -1)
new_message(MT_delayed, "%s: unknown user",
optarg);
- else
- ps.huid = (uid_t)-1;
break;
- case 'p': { /* display only process id */
- const char *errstr;
-
- i = strtonum(optarg, 0, INT_MAX, );
- if (errstr != NULL || !find_pid(i))
+ case 'p': /* display only process id */
+ if (filterpid(optarg, No) == -1)
new_message(MT_delayed, "%s: unknown pid",
optarg);
- else {
- ps.pid = (pid_t)i;
- ps.system = Yes;
- }
break;
- }
case 'S': /* show system processes */
ps.system = !ps.system;
@@ -802,20 +826,12 @@ rundisplay(void)
tempbuf[1] == '\0') {
ps.uid = (uid_t)-1;
ps.huid = (uid_t)-1;
- } else if (tempbuf[0] == '-') {
- if (uid_from_user(tempbuf+1, )
== -1) {
- new_message(MT_standout,
- " %s: unknown user",
tempbuf+1);
- no_command = Yes;
- } else {
- ps.uid = (uid_t)-1;
- }
- } else if (uid_from_user(tempbuf, ) ==
-1) {
- new_message(MT_standout,
- " %s: unknown user",
tempbuf);
- no_command = Yes;
- } else {
- ps.huid = (uid_t)-1;
+ } else if (filteruser(tempbuf) == -1) {
+ new_message(MT_standout,
+ " %s: unknown user",
+
Re: top: merge duplicate code into helper functions
On Tue, Oct 30, 2018 at 02:24:46PM -0600, Todd C. Miller wrote:
> I really dislike the side-effect in filteruser(), see below.
> > +static int
> > +filteruser(char buf[])
> > +{
> > + char *bufp = buf;
> > + uid_t *uidp;
> > +
> > + if (bufp[0] == '-') {
> > + bufp++[0] = ' ';
>
> Why is this needed, can't you just do "bufp++"?
Because we cannot increment the function argument directly:
int
rundisplay(void)
{
static char tempbuf[TEMPBUFSIZE];
...
case CMD_user:
...
} else if (filteruser(tempbuf) == -1) {
Hence filteruser() uses a pointer to it. Replacing the prefixed dash
with a space it not neccessary for parsing, it merely keeps error
messages intact.
With just `bufp++' the command "u-kn" would print " -foo: unknown user".
Re: top: merge duplicate code into helper functions
On Tue, 30 Oct 2018 21:09:29 +0100, Klemens Nanni wrote:
> This introduces filteruser() and filterpid(), where the first one can
> now easily be extended to support matching numeric UIDs. The latter one
> is now used by the `highlight` command as well.
I really dislike the side-effect in filteruser(), see below.
Otherwise looks good.
- todd
> Index: top.c
> ===
> RCS file: /cvs/src/usr.bin/top/top.c,v
> retrieving revision 1.94
> diff -u -p -r1.94 top.c
> --- top.c 5 Oct 2018 18:56:57 - 1.94
> +++ top.c 30 Oct 2018 19:56:07 -
> @@ -83,7 +83,7 @@ int no_command = Yes;
> int old_system = No;
> int old_threads = No;
> int show_args = No;
> -pid_t hlpid = -1;
> +pid_t hlpid = (pid_t)-1;
> int combine_cpus = 0;
>
> #if Default_TOPN == Infinity
> @@ -131,6 +131,46 @@ usage(void)
> __progname);
> }
>
> +static int
> +filteruser(char buf[])
> +{
> + char *bufp = buf;
> + uid_t *uidp;
> +
> + if (bufp[0] == '-') {
> + bufp++[0] = ' ';
Why is this needed, can't you just do "bufp++"?
> + uidp =
> + ps.uid = (pid_t)-1;
> + } else {
> + uidp =
> + ps.huid = (pid_t)-1;
> + }
> +
> + return uid_from_user(bufp, uidp);
> +}
> +
> +static int
> +filterpid(char buf[], int hl)
> +{
> + const char *errstr;
> + int pid;
> +
> + pid = strtonum(buf, 0, INT_MAX, );
> + if (errstr != NULL || !find_pid(pid))
> + return -1;
> +
> + if (hl == Yes)
> + hlpid = (pid_t)pid;
> + else {
> + if (ps.system == No)
> + old_system = No;
> + ps.pid = (pid_t)pid;
> + ps.system = Yes;
> + }
> +
> + return 0;
> +}
> +
> static void
> parseargs(int ac, char **av)
> {
Re: top: merge duplicate code into helper functions
Another approach to merging code from options and interactive commands.
This introduces filteruser() and filterpid(), where the first one can
now easily be extended to support matching numeric UIDs. The latter one
is now used by the `highlight` command as well.
Feedback? OK?
Index: top.c
===
RCS file: /cvs/src/usr.bin/top/top.c,v
retrieving revision 1.94
diff -u -p -r1.94 top.c
--- top.c 5 Oct 2018 18:56:57 - 1.94
+++ top.c 30 Oct 2018 19:56:07 -
@@ -83,7 +83,7 @@ int no_command = Yes;
int old_system = No;
int old_threads = No;
int show_args = No;
-pid_t hlpid = -1;
+pid_t hlpid = (pid_t)-1;
int combine_cpus = 0;
#if Default_TOPN == Infinity
@@ -131,6 +131,46 @@ usage(void)
__progname);
}
+static int
+filteruser(char buf[])
+{
+ char *bufp = buf;
+ uid_t *uidp;
+
+ if (bufp[0] == '-') {
+ bufp++[0] = ' ';
+ uidp =
+ ps.uid = (pid_t)-1;
+ } else {
+ uidp =
+ ps.huid = (pid_t)-1;
+ }
+
+ return uid_from_user(bufp, uidp);
+}
+
+static int
+filterpid(char buf[], int hl)
+{
+ const char *errstr;
+ int pid;
+
+ pid = strtonum(buf, 0, INT_MAX, );
+ if (errstr != NULL || !find_pid(pid))
+ return -1;
+
+ if (hl == Yes)
+ hlpid = (pid_t)pid;
+ else {
+ if (ps.system == No)
+ old_system = No;
+ ps.pid = (pid_t)pid;
+ ps.system = Yes;
+ }
+
+ return 0;
+}
+
static void
parseargs(int ac, char **av)
{
@@ -150,32 +190,16 @@ parseargs(int ac, char **av)
break;
case 'U': /* display only username's processes */
- if (optarg[0] == '-') {
- if (uid_from_user(optarg+1, ) == -1)
- new_message(MT_delayed,
- "%s: unknown user", optarg);
- else
- ps.uid = (uid_t)-1;
- } else if (uid_from_user(optarg, ) == -1)
+ if (filteruser(optarg) == -1)
new_message(MT_delayed, "%s: unknown user",
optarg);
- else
- ps.huid = (uid_t)-1;
break;
- case 'p': { /* display only process id */
- const char *errstr;
-
- i = strtonum(optarg, 0, INT_MAX, );
- if (errstr != NULL || !find_pid(i))
+ case 'p': /* display only process id */
+ if (filterpid(optarg, No) == -1)
new_message(MT_delayed, "%s: unknown pid",
optarg);
- else {
- ps.pid = (pid_t)i;
- ps.system = Yes;
- }
break;
- }
case 'S': /* show system processes */
ps.system = !ps.system;
@@ -802,20 +826,10 @@ rundisplay(void)
tempbuf[1] == '\0') {
ps.uid = (uid_t)-1;
ps.huid = (uid_t)-1;
- } else if (tempbuf[0] == '-') {
- if (uid_from_user(tempbuf+1, )
== -1) {
- new_message(MT_standout,
- " %s: unknown user",
tempbuf+1);
- no_command = Yes;
- } else {
- ps.uid = (uid_t)-1;
- }
- } else if (uid_from_user(tempbuf, ) ==
-1) {
- new_message(MT_standout,
- " %s: unknown user",
tempbuf);
- no_command = Yes;
- } else {
- ps.huid = (uid_t)-1;
+ } else if (filteruser(tempbuf) == -1) {
+ new_message(MT_standout,
+ "%s: unknown user", tempbuf);
+ no_command = Yes;
}
putr();
} else
@@ -854,23 +868,10 @@ rundisplay(void)
tempbuf[1] == '\0') {
ps.pid =
