[PATCH] sulogin size reduction and new logging

[Rob Landley]

On Saturday 09 September 2006 10:45 am, Tito wrote:
> On Saturday 9 September 2006 15:59, you wrote:
> > On Friday 08 September 2006 00:40, Tito wrote:
> > > Hi,
> > > this patch reduces the size of sulogin
> > > and moves it to the new syslog mode.
> > > 
> > > Review is as always welcome.
> > 
> > -#define SULOGIN_PROMPT "\nGive root password for system maintenance\n" \
> > +#define SULOGIN_PROMPT "Give root password for system maintenance\n" \
> >         "(or type Control-D for normal startup):"
> > 
> > If it is used just once, #define seems silly.
> 
> For me it make s the code more readable as you don't need to store 
> 2 lines of text in it.

For me the code's far more readable if it's all in one place then having to 
random-seek all over the place to figure out what it's doing.

> > 
> > +       if (bb_getopt_ulflags (argc, argv, "t:", &timeout_arg)) {
> > +               if (safe_strtoi(timeout_arg, &timeout)) {
> > +                       timeout = 0;
> >                 }
> >         }
> > 
> > Where is support for -f and -h? We say that we have it...
> > 
> > # busybox sulogin --help
> > BusyBox v1.2.1 (2006.08.29-06:37+0000) multi-call binary
> > Usage: sulogin [OPTION]... [tty-device]
> > Single user login
> > Options:
> >         -f      Do not authenticate (user already authenticated)
> >         -h      Name of the remote host for this login
> >         -p      Preserve environment
> > 
> > Although it wan't there in the first place... not your fault.
> 
> I didn't notice it because I don't use verbose usage.
> I think it is best is to rip -h (collision with -h like help) and -p out of 
the help text,
> unless you ant to implement them.
> In this case i would suggest to change -h to -H.

I'm not quite sure what -h is supposed to do.  I fired up the ubuntu sulogin 
page and it doesn't have -f or -h, so I doubt many people would miss them...

> > Do not use xx_FEATURE_SYSLOG in *.c file.
> 
> #define CONFIG_FEATURE_SYSLOG 1
> #define ENABLE_FEATURE_SYSLOG 1
> #define USE_FEATURE_SYSLOG(...)  __VA_ARGS__
> #define SKIP_FEATURE_SYSLOG(...)
> 
> If it is like this is a somewhat twisted logic vs what we do for all other 
flags
> and will create trouble because people like me will try to use it 
> the same way of all other flags.

It shouldn't be CONFIG_FEATURE if it's not selectable.  FEATURE means 
something specific.

Also, this sounds like something our dependency checker should be able to 
figure out, or at the very least our makefiles.  Hard-wiring it into the 
config system like this strikes me as the wrong approach.

I suspect the best approach would be some kind of linker magic in libbb, with 
the verror() thing being a weak symbol that only gets sucked in if 
big_verror() isn't linked in first (since big_verror() provides verror()), 
and then put big_verror() in the same .o file as open_syslog() and just make 
sure to get the link order right.

(The above is vague handwaving, but I'm pretty sure I could look up how to do 
it.  uClibc does this sort of stuff all over the place...)

> > If you want to make applet's ability to print to syslog configurable,
> > then you need separate CONFIG_xxx. See CONFIG_FEATURE_UDHCP_SYSLOG.
> 
> Don't think Rob will like this one flag per applet policy......

Not particularly, no. :(

In theory it's what CONFIG_NITPICK was for, but in practice if _one_ applet 
uses syslog then they might as well all do so since the code got sucked in 
anyway...

> > +       if (!(pwd = getpwuid(0))) {
> > 
> > This one is small, but try to avoid if(... a=expr .... ) if expr is 
complex.
> > Example from crond.c:
> > if ((logfd = open(LogFile, O_WRONLY | O_CREAT | O_APPEND, 0600)) >= 
0) ....
> > Awwww my eyes....
> 
> Maybe this code is smaller if the compiler is not so smart to optimise it to 
> the same code..........and it creates less lines so more code fits in your 
screen at the same time
> 
> logfd = open(LogFile, O_WRONLY | O_CREAT | O_APPEND, 0600);
> if (logfd >= 0)

The linux-kernel guys insist on the two-line form.  I don't personally care 
much either way, but they say it reliably compiles to the same code.

I do try to avoid using extra parentheses solely to shut up the compiler 
warning about an assignment in an if statement.  If parentheses could 
reasonably be inserted because you're doing something with the result, that's 
one thing, but extra parentheses that mean something magic to the compiler 
just don't seem right to me.  Personal taste, probably.  It's not usually 
something I'll change in code I'm reviewing.

> > +                       bb_info_msg("Normal startup");
> > 
> > As opposed to error_msg, this is right. info_msg does NOT print applet 
name,
> > capital letter is ok.
> 
> ;-)
> 
> > 
> > +AUTH_ERROR:    
> > +       bb_error_msg_and_die("No password entry for `root'");
> >  }
> > 
> > I think labels are usually lowercase.
> 
> I have seen them usually UPPERCASE in most bb code and personally think
> that so they are easier to spot, but that is a matters of taste. 

I insist that PREPROCESSOR_MACROS be upper case because their semantics are 
different enough to be tricky (the error messages when something goes wrong 
are often horribly misleading), and they tend to be defined way the heck 
elsewhere (often in a different file).  If it looks like a warning, it 
should. :)

Labels are only used by gotos, and are local to the function.  I do want them 
at the left edge so you can spot 'em more easily.  Don't indent them with the 
rest of the flow control statements; their purpose is to _bypass_ normal flow 
control.  They're supposed to stand out, and they're supposed to be ugly.

But I don't care much what you call them.

Rob
-- 
Never bet against the cheap plastic solution.