On Tuesday 25 November 2008 13:30, Rob Landley wrote: > > Other that NFS code - what places you don't like Rob? > > An #ifdef for _dietlibc_,
If someone sends me a patch for this, I conclude they do build it against dietlibc. Why not help them? > special casing rootfs, special casing shared subtree > flags, > the mount_option_str separately quoting "\0" (small but ugly, you > wouldn't do it for \n)... fixed > Some of it might just be that I've gotten used to looking at the toybox > infrastructure for things like option parsing, so constructs like: > > #if ENABLE_BLAH > #define ifBlah (logic) > #else > #define ifBlah 0 > #endif > > Just look really wrong, although that one was probably my fault once upon a > time. > > Lots of #if ENABLE in general that could be if (ENABLE) instead. > ENABLE_MOUNT_LABEL in resolve_mount_spec(), for example. fixed resolve_mount_spec(). > In general a static > function should be inlineable and optimizable away with gcc 4.x. should be != is. gcc is still sometimes rather stupid. #if FOO is a surefire way to DEFINITELY exclude some code. if (FOO) (actually some more complex constructs with &&, ||, etc) confuse gcc. > I'm looking > at verbose_mount() here, which only has two callers anyway so guarding it at > the call sites might be better, although the second call site has comments on > each _argument_, which is sick... > > Speaking of verbose_mount(), in its second caller you have this: > > rc = verbose_mount(/*source:*/ "", /*target:*/ argv[0], > /*type:*/ "", /*flags:*/ i, /*data:*/ ""); fixed > A quick glance at the code shows it's got lots of: > // WARNING. I am not sure this matches util-linux's > // behavior. It's possible util-linux does not > // take -o opts from mtab (takes only mount source). > > I actually explicitly tested that sort of thing over a period of 3 months and > worked out what the correct behavior should _be_, and implemented it at the > time. Yeah, users reported discrepancy but not really followed it through. I think I need to document in a comment as a minimum for the benefit of whoever will hack on it further. -- vda _______________________________________________ busybox mailing list [email protected] http://busybox.net/cgi-bin/mailman/listinfo/busybox
