[Toybox] [patch] add renice

[Rob Landley]

On 07/28/2013 04:19:50 PM, Felix Janda wrote:
> Some comments below.
> 
> Strake wrote:
> > From feeb53bf11e949e48f80752a642fe551c284d7a7 Mon Sep 17 00:00:00  
> 2001
> > From: Strake <strake888 at gmail.com>
> > Date: Sun, 28 Jul 2013 09:30:35 -0500
> > Subject: [PATCH] add renice
> >
> > ---
> >  toys/pending/renice.c | 48  
> ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >  create mode 100644 toys/pending/renice.c
> >
> > diff --git a/toys/pending/renice.c b/toys/pending/renice.c
> > new file mode 100644
> > index 0000000..44219c2
> > --- /dev/null
> > +++ b/toys/pending/renice.c
> > @@ -0,0 +1,48 @@
> > +/* renice.c - renice process
> > + *
> > + * Copyright 2013 CE Strake <strake888 at gmail.com>
> > + *
> > + * See http://pubs.opengroup.org/onlinepubs/9699919799/utilities/
> > + * See  
> http://refspecs.linuxfoundation.org/LSB_4.1.0/LSB-Core-generic/LSB-Core-generic/cmdbehav.html
> > +
> > +USE_RENICE(NEWTOY(renice, "gpun#", TOYFLAG_BIN))
> > +
> > +config RENICE
> > +  bool "renice"
> > +  default n
> > +  help
> > +    usage: renice [-gpu] -n increment ID ...
> > +*/
> > +
> > +#define FOR_renice
> > +#include "toys.h"
> > +
> > +GLOBALS(
> > +  long nArgu;
> > +)
> > +
> > +void renice_main (void) {
> > +  int ii;
> > +  int which = toys.optflags & FLAG_g ? PRIO_PGRP :
> > +              toys.optflags & FLAG_u ? PRIO_USER :
> > +              PRIO_PROCESS;
> > +
> > +  if (!(toys.optflags & FLAG_n)) error_exit ("no increment given");
> 
> Rob has documented the suffix "|" for the option parsing string to  
> denote
> an option which is mandatory. However it is not implemented yet. IMO  
> it
> would be useful to comment that if the "|" suffix is implemented at  
> some
> point this toy could make use of it.

The right thing for me to do is finish the "|" feature. :)

I hadn't bothered because it's fairly easy to check if (!  
(toys.optflags & FLAG_a|FLAG_b|Flag_c)) at runtime, but it's something  
the infrastructure really should do for us. The hard part of having the  
infrastructure do it is actually the error reporting, but if I ignore  
the "what if the required option is a --longopt" possibility (which  
probably should never happen), then error reporting of something like:

   Need -abcdx

Becomes a bit more strightforward. Requires traversing a list to get  
the option flags, but eh...

Hmmm, looking at renice, the example the man page gives is:

   renice +1 987 -u daemon root -p 32

Let's look at the problems with option parsing:

1) no -n, but it's ok. First argument becomes that if it's not  
supplied. (So what would happen if you did "renice 987 -n 1"? No idea.)

2) The -u and -p are gearshifts, not affecting the whole command line  
but only arguments that come after the most recent one. Which my  
argument parsing logic is _not_ set up to cope with.

3) Is this an absolute or relative prority? Does the + mean something?  
If it's relative, how would you indicate absolute negative priorities?  
(Is that what -n does?)

Sigh. Poorly specified. Let's pull up posix...

   renice [-g|-p|-u] -n increment ID...

And now we have -g, no gearshift, and -n is required. And this  
clarifies that it's always an offset relative to the current priority.  
And that matches what you implemented a lot more closely...

> > +
> > +  for (ii = 0; ii < toys.optc; ii++) {
> 
> You don't really need the index. Just loop over toys.optargs, which  
> are
> conveniently null terminated.
> 
> > +    id_t id;
> > +
> > +    if (isdigit (toys.optargs[ii][0])) id = strtoul  
> (toys.optargs[ii], 0, 10);
> > +    else if (toys.optflags & FLAG_u) id = getpwnam  
> (toys.optargs[ii])
> > -> pw_uid;
> 
> Mail mungling FTW.
> 
> This logic looks broken if a username should start with a digit.  
> POSIX says
> that if -u is specified, first each argument should be interpreted as  
> a user
> name and only then as a decimal ID.

I'd also worry that if getpwnam() is fed a nonexistent user you  
dereference a null pointer.

I can clean this one up from here...

Rob

 1375266336.0