[Toybox] [PATCH] tar: fix warn_unused_result error.

[enh]

On Thu, Sep 2, 2021 at 4:37 PM Rob Landley <rob at landley.net> wrote:

> On 9/2/21 2:47 PM, enh wrote:
> > On Wed, Sep 1, 2021 at 3:00 PM Rob Landley <rob at landley.net
> > <mailto:rob at landley.net>> wrote:
> >
> >     On 8/31/21 11:16 AM, enh via Toybox wrote:
> >     >   toys/posix/tar.c:821:7: error: ignoring return value of
> >     >   function declared with 'warn_unused_result' attribute
> >     >       write(sefd, 0, 0);
> >     >       ^~~~~ ~~~~~~~~~~
> >     > ---
> >     >  toys/posix/tar.c | 2 +-
> >     >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >     Yeah, I thought about that when I wrote it, but it seems like the
> correct thing
> >     to do on failure here _is_ nothing? We already warned if the open()
> or the
> >     write() of the xattr data failed, this is just telling it to reset
> after the
> >     initial write() to set it displayed any error and set exitval.
> >
> > to me, if the open() succeeded but the write() failed --- we're in weird
> > uncharted "shouldn't happen" territory, and probably want to know?
>
> Hmmm, I thought the open would succeed and the write fail, but if it's
> reliable
> that the open is what fails when selinux is disabled...
>

(note that i haven't tested that!)


> >     I thought about typecasting it to (void) but decided to wait for a
> complaint
> >     because A)
> >
>  "CROSS_COMPILE=~/android/android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/bin/x86_64-linux-android-
> >     make distclean defconfig toybox" didn't warn about it (obviously
> -Wall doesn't
> >     mean all, that would be silly),
> >
> >
> > (i used to find that annoying, but since my team has to update the
> compiler for
> > Android, i do now see the flip side of it: that "true" -Wall would mean
> you
> > could never update the compiler. especially in a -Werror world.
>
> There's your problem: never -Werror.


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.


> Compiler version updates always hallucinate
> new weirdness. If nothing else you're making it harder to build old
> versions in
> new build environments for regression testing comparisons.
>

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.)


> > we have enough
> > work just dealing with the new warnings that _do_ get added to -Wall!
>
> My objection was that -Wall doesn't do what it says, but it's hysterical
> raisins
> again, and having dug into more docs the actual -Wall is called
> -Weverything.
> (Which I admit is near useless, but it's good to _have_ for times like
> this.)
>
> In hindsight, -Wall shoulda been -Wstd or some such. Which is going to be a
> political can of worms whatever you call it, but then it already is. (In
> busybox
> I had "make defconfig" be the maximum sane configuration, as opposed to
> allyesconfig. And then Denys' idea of "sane" and mine diverged rapidly
> after the
> handoff. But the point is there IS an allyesconfig and there should be a
> -Wall
> that turns on all the darn warnings.)
>
> But as the second message says, -Weverything wouldn't have addressed this.
> :)
>
> > and any
> > given warning's false positive rate can be highly dependent on the
> specific
> > codebase, and i don't get the feeling compiler writers think about
> scaling to
> > "can i rebuild all of Debian?" or even just "can i rebuild all of
> Android?".)
>
> Their userbase kinda does.
>

it's too late by then :-(


> > i'd have assumed (as you can tell from my patch :-) ) that "you asked me
> to set
> > the selinux labels but i couldn't" == error, not warning?
>
> Except nothing else currently works like that?
>
>   } else if (S_ISDIR(ala)) {
>     if ((mkdir(name, 0700) == -1) && errno != EEXIST)
>       return perror_msg("%s: can't create", TT.hdr.name);
>   } else if (S_ISLNK(ala)) {
>     if (symlink(TT.hdr.link_target, TT.hdr.name))
>       return perror_msg("can't link '%s' -> '%s'", name,
> TT.hdr.link_target);
>   } else if (mknod(name, ala, TT.hdr.device))
>     return perror_msg("can't create '%s'", name);
>
> $ grep _exit toys/*/tar.c
>     if (val<<8 < val) error_exit("bad header");
>     if (len && *str && *str != ' ') error_exit("bad header");
>   if (lskip(TT.fd, len)) perror_exit("EOF");
>       if (len+used>TT.hdr.size) error_exit("sparse overflow");
>     if (i && i!=512) error_exit("short header");
>     if (!is_tar_header(&tar)) error_exit("bad header");
>     if (!TT.incl) error_exit("empty archive");
>         else error_exit("Not tar");
>
> Those are all "cannot continue to parse the input file format", not
> "output had
> a problem"...
>

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!)


> >     So I _think_ your patch changes "tar x --selinux" behavior to exit
> instead of
> >     spamming warnings when run on a system without sexlinux support.
> >
> > seems like a feature to me? don't say --selinux if you don't actually
> care?
>
> Which would mean --selinux is special and behaves differently from the
> rest of tar.
>

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.


> Rob
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20210902/c86856d8/attachment.htm>