Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove/deprecate ENOATTR from Linux and Android #1356

Open
gnzlbg opened this issue May 22, 2019 · 5 comments
Open

Remove/deprecate ENOATTR from Linux and Android #1356

gnzlbg opened this issue May 22, 2019 · 5 comments

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented May 22, 2019

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: golang/go#753

cc @alexcrichton @mati865 @PlasmaPower

@gnzlbg
Copy link
Contributor Author

gnzlbg commented May 22, 2019

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:

@PlasmaPower
Copy link
Contributor

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.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented May 22, 2019

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.

@PlasmaPower
Copy link
Contributor

Makes sense. I'm fine with deprecating or removing it. 👍

@zackw
Copy link
Contributor

zackw commented May 17, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants