On Thu, May 7, 2015 at 12:31 AM, José Bollo <[email protected]> wrote:
> Le mercredi 06 mai 2015 à 13:19 -0700, James McMechan a écrit : > > Looking at the fixes: > > Hello all, Hello Jim, > > First of all thank you for you fiddly review ^^ > > > toys/other/stat.c for group name outputting the user name seems like a > good catch > > > > toys/posix/cp.c you are treating -p like --preserve=all not like -p > which only does mode,ownership,timestamps > > according to the man page -p does not copy over xattrs at all, which > seems kind of odd... > > You are absolutely right. I guess that Krysztof Sasiak that made the > initial commit has not expected to enter in the deep logic of arguments > and just added to functionnality here as is, breaking the common > semantic. > > To be correct, from my cp man page, I see that "-p" is same as > "--preserve=mode,ownership,timestamps" and that "--preserve[=ATTR_LIST]" > accepts the attributes: "context", "links", "xattr", "mode", > "ownership", "timestamps", and "all". coincidentally, someone on Android asked for cp --preserve this week. internal bug 21121352. i'll get around to that at some point if no one beats me to it (but not before Google I/O). > > > toys/posix/id.c seems to lose the TOYBOX_USR flag which I think insures > it is in /usr/bin like the regular version. > > Good catch. thx. > > > this is the second case where your are using > > (TOYBOX_SELINUX || TOYBOX_SMACK) > > perhaps you should add a hidden symbol like TOYBOX_SECURITY that could > have all the various versions || together and use that in place of doing it > by hand each time for the ??_SECURITY symbol. > > This is a good idea. But I'm affraid that after that, persnickety people > will then replace TOYBOX_SELINUX with TOYBOX_SECURITY_SELINUX and > TOYBOX_SMACK by TOYBOX_SECURITY_SMACK. But why being frighten by only > few hours of work? > > > also you pulled the > > if (CFG_TOYBOX_FREE) free(context); > > up into the two if statements rather leaving it after both like it was. > It should work as a good compiler will do tail combining but just having it > sitting at the end is both clearer and shorter because both cases fall > through to the same code. > > Size versus speed (or the opposite). > > > The lines 151-169 look like a extra string e.g. "LSM Smack Disabled" vs > "SELinux Disabled" and a slight shuffling of the if statements could make > that much shorter both halves seem to be almost identical... > > It is advocating to founding a security module in lib. That was > suggested by Elliott Hughes. > > Why not. I am seeing now a good reason to make it. From the viewpoint of > the security, our expect was that toybox is compiled: > - without security stuff > - with security SELinux (android) > - with security Smack (tizen) > > But it may only be true after answering this questions: > > - What other security model will enter in toybox? none? > - Should toybox be compiled in an agnostic way that adapts itself on the > kernel it runs on? > > > toybox/posix/mkdir.c does the normal mkdir really reset the process > label for all future use when you use -p & -Z ? should it set it back to > normal when the command is done? or is that per process context that > vanishes on exit? > > good catch. Yes, it vanish on the process die but it would forbid to use > it in library. > > > toys/posix/mkfifo.c lost TOYBOX_USR > > thx > > > I could not find any way to comment on git hub directly though. > > Hum! Arrrr... maybe because it is not a pull request... > > Best regards > José Bollo > > > Jim > (snip) > > -- Elliott Hughes - http://who/enh - http://jessies.org/~enh/ Android native code/tools questions? Mail me/drop by/add me as a reviewer.
_______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
