<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 7, 2015 at 12:31 AM, José Bollo <span dir="ltr"><<a href="mailto:jobol@nonadev.net" target="_blank">jobol@nonadev.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Le mercredi 06 mai 2015 à 13:19 -0700, James McMechan a écrit :<br>
> Looking at the fixes:<br>
<br>
Hello all, Hello Jim,<br>
<br>
First of all thank you for you fiddly review ^^<br>
<span class=""><br>
> toys/other/stat.c for group name outputting the user name seems like a good catch<br>
><br>
> toys/posix/cp.c you are treating -p like --preserve=all not like -p which only does mode,ownership,timestamps<br>
> according to the man page -p does not copy over xattrs at all, which seems kind of odd...<br>
<br>
</span>You are absolutely right. I guess that Krysztof Sasiak that made the<br>
initial commit has not expected to enter in the deep logic of arguments<br>
and just added to functionnality here as is, breaking the common<br>
semantic.<br>
<br>
To be correct, from my cp man page, I see that "-p" is same as<br>
"--preserve=mode,ownership,timestamps" and that "--preserve[=ATTR_LIST]"<br>
accepts the attributes: "context",  "links", "xattr", "mode",<br>
"ownership", "timestamps", and  "all".</blockquote><div><br></div><div>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).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><br>
> toys/posix/id.c seems to lose the TOYBOX_USR flag which I think insures it is in /usr/bin like the regular version.<br>
<br>
</span>Good catch. thx.<br>
<span class=""><br>
> this is the second case where your are using<br>
> (TOYBOX_SELINUX || TOYBOX_SMACK)<br>
> 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.<br>
<br>
</span>This is a good idea. But I'm affraid that after that, persnickety people<br>
will then replace TOYBOX_SELINUX with TOYBOX_SECURITY_SELINUX and<br>
TOYBOX_SMACK by TOYBOX_SECURITY_SMACK. But why being frighten by only<br>
few hours of work?<br>
<span class=""><br>
> also you pulled the<br>
> if (CFG_TOYBOX_FREE) free(context);<br>
> 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.<br>
<br>
</span>Size versus speed (or the opposite).<br>
<span class=""><br>
> 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...<br>
<br>
</span>It is advocating to founding a security module in lib. That was<br>
suggested by Elliott Hughes.<br>
<br>
Why not. I am seeing now a good reason to make it. From the viewpoint of<br>
the security, our expect was that toybox is compiled:<br>
 - without security stuff<br>
 - with security SELinux (android)<br>
 - with security Smack (tizen)<br>
<br>
But it may only be true after answering this questions:<br>
<br>
- What other security model will enter in toybox? none?<br>
- Should toybox be compiled in an agnostic way that adapts itself on the<br>
kernel it runs on?<br>
<span class=""><br>
> 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?<br>
<br>
</span>good catch. Yes, it vanish on the process die but it would forbid to use<br>
it in library.<br>
<br>
>               toys/posix/mkfifo.c lost TOYBOX_USR<br>
<br>
thx<br>
<span class=""><br>
> I could not find any way to comment on git hub directly though.<br>
<br>
</span>Hum! Arrrr... maybe because it is not a pull request...<br>
<br>
Best regards<br>
José Bollo<br>
<br>
> Jim<br>
(snip)<br>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">Elliott Hughes - <a href="http://who/enh" target="_blank">http://who/enh</a> - <a href="http://jessies.org/~enh/" target="_blank">http://jessies.org/~enh/</a><br>Android native code/tools questions? Mail me/drop by/add me as a reviewer.</div>
</div></div>