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

[Rob Landley]

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

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

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

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

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

Rob