libc icon indicating copy to clipboard operation
libc copied to clipboard

Remove/deprecate ENOATTR from Linux and Android

Open gnzlbg opened this issue 6 years ago • 5 comments

The ENOATTR attribute is not shipped with any system headers on these platforms. This was raised in #594 as the main argument not to add this for Linux. There was a comment that claimed that this attribute is mentioned in the man pages of setxattr and getxattr but I don't find any mention of ENOATTR there anymore =/

Then #1030 came along and added the attribute for Android, but this is not available there either AFAICT, and I can't find it on any documentation.

Go rejected adding ENOATTR from their syscall package for this reason: https://github.com/golang/go/issues/753

cc @alexcrichton @mati865 @PlasmaPower

gnzlbg avatar May 22 '19 10:05 gnzlbg

It appears that the header <attr/xattr.h>, where this constant was supposed to be, was removed in 2015: http://git.savannah.nongnu.org/cgit/attr.git/commit/?id=7921157890d07858d092f4003ca4c6bae9fd2c38

The header that one should use is <sys/xattr.h> which is available on Linux and Android, but does not contain ENOATTR in either:

  • Android: https://android.googlesource.com/platform/prebuilts/gcc/linux-x86/host/x86_64-linux-glibc2.7-4.6/+/refs/heads/ics-plus-aosp/sysroot/usr/include/sys/xattr.h

  • Linux: https://elixir.bootlin.com/linux/v4.6/source/include/linux/xattr.h

gnzlbg avatar May 22 '19 11:05 gnzlbg

It appears to be in older copies of the man pages, like https://linux.die.net/man/2/getxattr

(ENOATTR is defined to be a synonym for ENODATA in <attr/xattr.h>.)

In case that gets updated too, here's an archive.org link: https://web.archive.org/web/20160808193900/http://linux.die.net/man/2/getxattr

As per https://mail-index.netbsd.org/tech-kern/2012/04/30/msg013090.html which is also outdated but mostly correct, here's platform behavior:

  1. return ENOATTR, while ENODATA is defined and has other usages: NetBSD and MacOS X

  2. return ENODATA, while ENOATTR is not defined: Linux

  3. return ENOATTR, while ENODATA is not defined: FreeBSD

Notice that if you define ENOATTR as a synonym for ENODATA on Linux with attr/xattr.h as was done previously, then you can use ENOATTR on any platform, which is what I was doing in my code. My code isn't working and hasn't been touched for a while, so you don't need to worry about breaking that.

I think ENOATTR still makes sense for writing cross platform code, but I also agree that it doesn't belong here since Linux seems to have gotten rid of it. I'm not sure if anyone else is using it, it is rather niche, but if we're going to get rid of it we might as well deprecate it first I think.

PlasmaPower avatar May 22 '19 14:05 PlasmaPower

FYI if you need a portable wrapper over libc, you can use the nix crate for that. I don't know if they have a portable wrapper around these APIs, but they might.

The problem with adding ENOATTR synthetically to targets that don't have it is that, technically, Android and Linux could add it in the future, and with a different value of ENODATA. First, we wouldn't know about this, because we are skipping it in our tests. So some people might start to use it, and would be passing these APIs an incorrect value.

Second, if we change the value, we'd break old code that uses it as a synonym of ENODATA (code that on Linux would not compile in the first place), so we'd probably need to deprecate it anyways, and add a ENOATTR2 that people can use on Linux and Android instead, but now their "cross-platform" code isn't cross-platform anymore.

I'm not sure if anyone else is using it, it is rather niche, but if we're going to get rid of it we might as well deprecate it first I think.

This makes sense.

gnzlbg avatar May 22 '19 15:05 gnzlbg

Makes sense. I'm fine with deprecating or removing it. :+1:

PlasmaPower avatar May 22 '19 15:05 PlasmaPower

This constant doesn't exist on Linux, but it does exist on FreeBSD and probably other systems in the BSD clade, and it's actually quite important.

Suppose you want to remove an extended attribute from a file and ignore the error that you get when the attribute doesn't exist. Suppose further that you are using the xattr crate. It glosses over the difference in how you do this between OSes: removexattr on Linux, extattr_delete_file on FreeBSD, both wrapped into xattr::remove by the xattr crate. But it does not gloss over the difference in the error codes; you get a plain old std::io::Error created from the raw errno code, which will be ENODATA if you're on Linux and ENOATTR on *BSD. The stdlib does not have error kinds for these codes, and so you're gonna want to write something like

    xattr::remove("file", "attribute")
        .or_else(|e| match e.raw_os_error() {
            Some(code) if code == libc::ENODATA || code == libc::ENOATTR { Ok(()) },
            _ => { Err(e) },
        })?

but this throws deprecation errors on Linux. :-(

Absent any changes to either the stdlib or the xattr crate, how would you write this so it works on both Linux and *BSD and is future-proof?

zackw avatar May 17 '23 20:05 zackw