<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Sep 2, 2021 at 4:37 PM Rob Landley <<a href="mailto:rob@landley.net">rob@landley.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 9/2/21 2:47 PM, enh wrote:<br>
> On Wed, Sep 1, 2021 at 3:00 PM Rob Landley <<a href="mailto:rob@landley.net" target="_blank">rob@landley.net</a><br>
> <mailto:<a href="mailto:rob@landley.net" target="_blank">rob@landley.net</a>>> wrote:<br>
> <br>
> On 8/31/21 11:16 AM, enh via Toybox wrote:<br>
> > toys/posix/tar.c:821:7: error: ignoring return value of<br>
> > function declared with 'warn_unused_result' attribute<br>
> > write(sefd, 0, 0);<br>
> > ^~~~~ ~~~~~~~~~~<br>
> > ---<br>
> > toys/posix/tar.c | 2 +-<br>
> > 1 file changed, 1 insertion(+), 1 deletion(-)<br>
> <br>
> Yeah, I thought about that when I wrote it, but it seems like the correct thing<br>
> to do on failure here _is_ nothing? We already warned if the open() or the<br>
> write() of the xattr data failed, this is just telling it to reset after the<br>
> initial write() to set it displayed any error and set exitval.<br>
> <br>
> to me, if the open() succeeded but the write() failed --- we're in weird<br>
> uncharted "shouldn't happen" territory, and probably want to know?<br>
<br>
Hmmm, I thought the open would succeed and the write fail, but if it's reliable<br>
that the open is what fails when selinux is disabled...<br></blockquote><div><br></div><div>(note that i haven't tested that!)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> I thought about typecasting it to (void) but decided to wait for a complaint<br>
> because A)<br>
> "CROSS_COMPILE=~/android/android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/bin/x86_64-linux-android-<br>
> make distclean defconfig toybox" didn't warn about it (obviously -Wall doesn't<br>
> mean all, that would be silly), <br>
> <br>
> <br>
> (i used to find that annoying, but since my team has to update the compiler for<br>
> Android, i do now see the flip side of it: that "true" -Wall would mean you<br>
> could never update the compiler. especially in a -Werror world.<br>
<br>
There's your problem: never -Werror. </blockquote><div><br></div><div>if you don't -Werror, you inevitably check in code with warnings, spamming the build and making it easier for more warnings to creep in.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Compiler version updates always hallucinate<br>
new weirdness. If nothing else you're making it harder to build old versions in<br>
new build environments for regression testing comparisons.<br></blockquote><div><br></div><div>with hermetic builds you're using the right compiler for that version anyway. (forget features: when 100M lines of code you have, reliant on compiler bugs you will be.)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> we have enough<br>
> work just dealing with the new warnings that _do_ get added to -Wall!<br>
<br>
My objection was that -Wall doesn't do what it says, but it's hysterical raisins<br>
again, and having dug into more docs the actual -Wall is called -Weverything.<br>
(Which I admit is near useless, but it's good to _have_ for times like this.)<br>
<br>
In hindsight, -Wall shoulda been -Wstd or some such. Which is going to be a<br>
political can of worms whatever you call it, but then it already is. (In busybox<br>
I had "make defconfig" be the maximum sane configuration, as opposed to<br>
allyesconfig. And then Denys' idea of "sane" and mine diverged rapidly after the<br>
handoff. But the point is there IS an allyesconfig and there should be a -Wall<br>
that turns on all the darn warnings.)<br>
<br>
But as the second message says, -Weverything wouldn't have addressed this. :)<br>
<br>
> and any<br>
> given warning's false positive rate can be highly dependent on the specific<br>
> codebase, and i don't get the feeling compiler writers think about scaling to<br>
> "can i rebuild all of Debian?" or even just "can i rebuild all of Android?".)<br>
<br>
Their userbase kinda does.<br></blockquote><div><br></div><div>it's too late by then :-(</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> i'd have assumed (as you can tell from my patch :-) ) that "you asked me to set<br>
> the selinux labels but i couldn't" == error, not warning?<br>
<br>
Except nothing else currently works like that?<br>
<br>
} else if (S_ISDIR(ala)) {<br>
if ((mkdir(name, 0700) == -1) && errno != EEXIST)<br>
return perror_msg("%s: can't create", <a href="http://TT.hdr.name" rel="noreferrer" target="_blank">TT.hdr.name</a>);<br>
} else if (S_ISLNK(ala)) {<br>
if (symlink(TT.hdr.link_target, <a href="http://TT.hdr.name" rel="noreferrer" target="_blank">TT.hdr.name</a>))<br>
return perror_msg("can't link '%s' -> '%s'", name, TT.hdr.link_target);<br>
} else if (mknod(name, ala, TT.hdr.device))<br>
return perror_msg("can't create '%s'", name);<br>
<br>
$ grep _exit toys/*/tar.c<br>
if (val<<8 < val) error_exit("bad header");<br>
if (len && *str && *str != ' ') error_exit("bad header");<br>
if (lskip(TT.fd, len)) perror_exit("EOF");<br>
if (len+used>TT.hdr.size) error_exit("sparse overflow");<br>
if (i && i!=512) error_exit("short header");<br>
if (!is_tar_header(&tar)) error_exit("bad header");<br>
if (!TT.incl) error_exit("empty archive");<br>
else error_exit("Not tar");<br>
<br>
Those are all "cannot continue to parse the input file format", not "output had<br>
a problem"...<br></blockquote><div><br></div><div>ah, so you're talking about the perror_msg() versus perror_exit() distinction. still a _failure_ but not an immediate one. (though as someone who always uses 'v' with tar, i'd never notice the failure interactively unless it was in the last 40 lines!)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> So I _think_ your patch changes "tar x --selinux" behavior to exit instead of<br>
> spamming warnings when run on a system without sexlinux support.<br>
> <br>
> seems like a feature to me? don't say --selinux if you don't actually care?<br>
<br>
Which would mean --selinux is special and behaves differently from the rest of tar.<br></blockquote><div><br></div><div>i was going to say "but if you got the wrong selinux labels, what you wrote out probably won't work" ... but then realized that's true of chmod() too. i'm not going to be able to run that binary if its 'x' bit isn't set, any more than if it doesn't have the right selinux label.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Rob<br>
</blockquote></div></div>