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? _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
