[Toybox] [CLEANUP] makedevs

[Rob Landley]

Commit 1362: http://landley.net/hg/toybox/rev/1362

First question: Are there any test command lines I can use for this?

The 64 byte limitation on "name" is arbitrary, it's the start of the
string so we might as well just null terminate and use that memory.

The help text says to feed '-' to unnecessary fields, but scanf aborts
when it hits '-' for %u, so if you feed '-' in any of the last 5 fields
the rest of them are ignored. Might as well just tell them not to supply
unnecessary fields?

We do no error checking if they feed letters into the numeric fields?
Ok... (Fixing this seems to involve adding an %n after very field, and
then there's the "pass in -" bit so I'd have to test for and _not_
complain about that... Not sure it's worth it? The lack of properly
created devices wouldn't exactly be subtle and the only users of this
are not just sysadmins but system developers....)

We skip leading "/" but not leading "../../..", do we really care about
restricting the mknod to within -d (or under current directory)? Because
if so we can abspath() both arguments and strstart() to enforce it. (I'm
_guessing_ no, since this has to run as root anyway.) Meanwhile, change
the if (*a == '/') a++; into a while() so we at least catch "//" paths.
(Unless we _want_ to be able to say // if they run mknod from /home
instead of / and they didn't specify -d... What's the intended behavior
here? There's no spec.)

Major and minor numbers on devices went beyond 8 bits each years ago.
Dunno if lanana has any, but the sycall certainly does, so the "invalid
line" check isn't quite right.

This code initializes mode to default 755, which then gets overwritten
by the sscanf. The only time that's useful is if sscanf() stops parsing
before writing that (say the line is _just_ "dev/filename f" for
example). But user[] and group[] aren't initialized, so if they don't
get filled out we have random stack crap in them, and then *group will
at best fail the lookup...

How _do_ we handle short lines? What's the shortest line that makes
sense? I guess we need at least node and type to create zero length
files? Except 'f' expects the file to already exist? What exactly is
that _for_? You can specify ownership and permissions for initramfs
without root access when using the kernel's gen_init_cpio, and you can
similarly specify ownership and permissions out of band for mksquashfs
and genext2fs. I do all three in Aboriginal Linux, see
image_filesystem() ala
http://landley.net/hg/aboriginal/file/38095bbf7794/sources/functions.sh#l375

Back to "shortest useful line", block or char devices need major/minor,
so instead of the > 255 test, it should be if the value returned by the
scanf >= 3 do the getpwnam() stuff, and >= 4 do the getgrnam() stuff?
Meh, just init *user = *group = 0 so the current test actually works...

Possibly there should be a lib function for xgetpwnamuid(), or have
xgetpwnam() automatically call xgetpwuid() as a fallback, but I'll worry
about that one later because that's involves looking at the other
existing users pf getpwnam() and getpwuid() to see what they're doing.
In the meantime, looking up user and group info seems unnecessarily
verbose inline, fixing it brings up the next issue: warnings vs errors!

The warning/error mix here is... odd? We exit on user or group lookup
failure, but can't create directory or chmod/chown is just a warning?
I'm unsure of the intent here? What are the criteria for a fatal error
vs continuing?

Also, why do some perror_msg() lines include line number, but others
don't? If the logic is "don't need the line number when we say the
filename"... they all say the filename?

(I thought about using toys.rebound to continue instead of exiting so we
could use the xfunctions() as warnings instead of errors, but between
the "when do we exit vs continue" and the line numbers, I didn't. The
behavior is not consistent enough to factor out.)

Speaking of library functions, there's a wfchmodat() that prints a
warning if it can't do it, but not a wfchownat(). I looked at the
chown/chgrp commands under toys/posix and they couldn't immediately
benefit from this (due to a -f flag suppressing errors), so stick it on
the todo list...

Why is do we call chmod() right after feeding mode to mknod()? Right,
let's move the chown/chmod logic to the end of the loop so it's common
code we can fall through to instead of being repeated three times... The
mknod() loop is going out of its way to allow a "cnt" of 0 to be
synonymous with 1? Is that expected behavior? (There's no spec for this
command, and no test cases...) Except if it _is_, why would incr = 0
screw up this line:

  dev = makedev(major, minor + (i - st_val) * incr);

Because the most obvious way to get cnt 0 is a short line that doesn't
specify all the fields, so we leave it at the default value (initialized
to 0), yet incr is _after_ that, and also initialized to 0, but incr 0
means minor is always the same for all the nodes we create...

Note: the "-" advice in the help text still causes scanf("%u") to
prematurely end lines. Possibly this was meant to be %d instead? Except...

  #include <stdio.h>

  int main(int argc, char *argv[])
  {
    int one, two, three;

    sscanf(argv[1], "%d %d %d", &one, &two, &three);
    printf("%d %d %d\n", one, two, three);
  }

  $ gcc test.c
  $ ./a.out "42 - 17"
  42 0 0

That doesn't work either.

Right, checking in the first pass, but there are several unanswered
questions here. Could somebody send me some test input for this thing so
I have a better idea what it should do before promoting it?


 1403698244.0