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

fix wrong xattr syscalls on NetBSD #2961

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

SteveLauC
Copy link
Contributor

I am not 100% sure that these bindings are wrong because I can not find them in the NetBSD manual.

But according to sys/sys/xattr.h, they seems to be wrong:

int	setxattr(const char *, const char *, const void *, size_t, int);
int	lsetxattr(const char *, const char *, const void *, size_t, int);

int	fremovexattr(int, const char *);
// Current bindings in `libc`:

pub fn setxattr(
    path: *const ::c_char,
    name: *const ::c_char,
    value: *const ::c_void,
    size: ::size_t,
) -> ::c_int;
pub fn lsetxattr(
    path: *const ::c_char,
    name: *const ::c_char,
    value: *const ::c_void,
    size: ::size_t,
) -> ::c_int;
pub fn fremovexattr(
    fd: ::c_int, 
    path: *const ::c_char, 
    name: *const ::c_char
) -> ::c_int;

Friendly ping @devnexen who originally added them in #2514 .

@rust-highfive
Copy link

r? @JohnTitor

(rust-highfive has picked a reviewer for you, use r? to override)

@JohnTitor
Copy link
Member

I think this is a breaking change as it changes the interface. I'm, however, fine to accept it as long as they aren't usable at all across all NetBSD versions (that makes it possible to assume there's no real user). Could you make sure of that?

@SteveLauC
Copy link
Contributor Author

Yes, this is a breaking change. I will try to fire up a NetBSD VM to test the current APIs.

@SteveLauC
Copy link
Contributor Author

SteveLauC commented Nov 4, 2022

Hi, guys, I just have NetBSD installed (including Rust toolchain and the GCC compiler), so I am ready to test the existing bindings.
Screenshot from 2022-11-04 17-39-43

But I may need some help on how to test them.

What values should be passed for those function parameters that do not exist? (the path argument in removexattr) For parameters that are not exposed, should we just ignore them (flag argument of setxattr and lsetxattr)?

Friendly ping @JohnTitor


UPDATE: I just found that the extended attribute is not enabled by default on NetBSD.

#include <sys/xattr.h>
#include <stdio.h>   
#include <errno.h>
 
int main(int ac, char *av[]) {
        int res = setxattr("/home/steve/workspace/c/file", "user.first_ea", "first_ea", 8, 0);
 
 
        if (res == -1) {
                printf("errno = %d\n", errno);
                perror(NULL);
        } else {
                printf("success");
        }
        return 0;
}
$ gcc main.c && ./a.out
errno = 86
Not supported

So now I need to find a way to enable it...

@SteveLauC
Copy link
Contributor Author

SteveLauC commented Nov 4, 2022

Though EA is not enabled, I still tested the following code snippet:

use libc::{__errno, c_char, c_void, lsetxattr, perror, fremovexattr, setxattr};
use std::{        
    ptr::null_mut,
    os::unix::io::AsRawFd,     
    fs::File,                                  
};       
            
fn test_setxattr() {        
    println!("Testing setxattr:");
    let res = unsafe {
        setxattr(
            "/home/steve/workspace/rust/Cargo.toml\0".as_ptr() as *mut c_char,
            "user.first\0".as_ptr() as *mut c_char,
            "first\0".as_ptr() as *mut c_void,
            5,                                                              
        )                     
    };               
                                                                                          
    if res == -1 {
        unsafe {
            perror(null_mut());
            println!("errno = {}", *__errno());
        }                      
    } else {                                   
        println!("success");
    }       
    println!();             
}    
               
 
fn test_lsetxattr() {
    println!("Testing lsetxattr:");
    let res = unsafe {
        lsetxattr(   
            "/home/steve/workspace/rust/Cargo.toml\0".as_ptr() as *mut c_char,
            "user.first\0".as_ptr() as *mut c_char,
            "user.first\0".as_ptr() as *mut c_char,
            "first\0".as_ptr() as *mut c_void,
            5,
        )
    };

    if res == -1 {
        unsafe {
            perror(null_mut());
            println!("errno = {}", *__errno());
        }
    } else {
        println!("success");
    }
    println!();
}

fn test_fremovexattr() {
    println!("Testing fremovexattr:");
    let file = File::open("/home/steve/workspace/rust/Cargo.toml").unwrap();
    let fd = file.as_raw_fd();
    let res = unsafe{
        fremovexattr(fd, "user.fist\0".as_ptr() as *mut c_char, null_mut() as *mut c_char)
    };
    if res == -1 {
        unsafe {
            perror(null_mut());
            println!("errno = {}", *__errno());
        }
    } else {
        println!("success");
    }
    println!();
}

fn main() {
    test_setxattr();
    test_lsetxattr();
    test_fremovexattr();
}
$ cargo r -q
Testing setxattr:
Not supported
errno = 86

Testing lsetxattr:
Not supported
errno = 86

Testing fremovexattr:
Not supported
errno = 86

The existing bindings seemed to work, at least they showed me that EA is not supported.

@SteveLauC
Copy link
Contributor Author

NetBSD 10 will be the first official release with full extended attribute support in FFS

If we would like to test these bindings further, we have to wait for the NetBSD 10 release

@tgross35
Copy link
Contributor

Has the situation changed here? I'm having trouble finding headers for these.

1.0 is coming soon so we can do breaking changes if needed.

@tgross35 tgross35 added this to the 1.0 milestone Nov 20, 2024
@tgross35
Copy link
Contributor

If we can find sources with these fields then we can merge this to main.

@rustbot author

@SteveLauC
Copy link
Contributor Author

Has the situation changed here? I'm having trouble finding headers for these.

Hi, looks like the header file is still there: https://github.com/NetBSD/src/blob/a623cf0b61440d709d496b4325d530e07f25fb94/sys/sys/xattr.h#L61-L77

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...I read that file about five times and missed the extra int each time. Thanks for the link, changes LGTM but please rebase.

@tgross35 tgross35 force-pushed the netbsd-wrong-xattr-syscall branch from 3a73b09 to 91f1fea Compare November 25, 2024 22:06
@rustbot rustbot added the O-unix label Nov 25, 2024
@tgross35 tgross35 enabled auto-merge November 25, 2024 22:06
@tgross35 tgross35 added this pull request to the merge queue Nov 25, 2024
Merged via the queue into rust-lang:main with commit 42e5708 Nov 25, 2024
45 checks passed
@tgross35 tgross35 added the stable-declined This change is breaking, difficult to backport, low priority, or otherwise not relevant for 0.2 label Nov 25, 2024
@SteveLauC
Copy link
Contributor Author

Emm, thanks for rebasing for me 🫡

@SteveLauC SteveLauC deleted the netbsd-wrong-xattr-syscall branch November 26, 2024 00:00
@tgross35
Copy link
Contributor

The button was there so figured why not :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breakage-candidate O-unix S-waiting-on-author S-waiting-on-ci stable-declined This change is breaking, difficult to backport, low priority, or otherwise not relevant for 0.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants