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

i32 vs u32 #9

Open
mcandre opened this issue Dec 26, 2018 · 2 comments
Open

i32 vs u32 #9

mcandre opened this issue Dec 26, 2018 · 2 comments

Comments

@mcandre
Copy link

mcandre commented Dec 26, 2018

I think file descriptors tend to use signed 32-bit integers, whereas the Rust crate uses unsigned. How important is it to expand the potential open files that wide, versus the trade-off of losing some type compatibility between libc vs. CloudABI?

This subtle difference makes it a bit harder to write polyglot programs that can target libc and CloudABI, depending on the toolchain applied.

@m-ou-se
Copy link
Member

m-ou-se commented Dec 26, 2018

@EdSchouten Is there a specific reason you defined fd as unsigned here?

opaque uint32 fd

Edit: Oh, looks like I wrote that line. I'm pretty sure I simply copied the type from your original C headers though.

@EdSchouten
Copy link
Member

I think we picked uint32 for the reason that CloudABI system calls don't return errors in band. The file descriptors returned by syscalls are never reused to return errors. They are opaque types, just like HANDLE on Windows.

Though I think that uint32 is technically correct in our case, I do agree that this is causing some churn when mixing pure POSIX and CloudABI code. Even inside of cloudlibc this is causing some nastiness (e.g., the casts in program_exec(), program_spawn(), recvmsg(), sendmsg() to convert between int * <-> cloudabi_fd_t *).

On the other hand, using uint32 does have the advantage that it's a bit safer on the kernel/runtime side. Observe this code in cloudabi-run's emulator:

static cloudabi_errno_t fd_table_get_entry(struct fd_table *ft,
                                           cloudabi_fd_t fd,
                                           cloudabi_rights_t rights_base,
                                           cloudabi_rights_t rights_inheriting,
                                           struct fd_entry **ret)
    REQUIRES_SHARED(ft->lock) {
  // Test for file descriptor existence.
  if (fd >= ft->size)
    return CLOUDABI_EBADF;
  struct fd_entry *fe = &ft->entries[fd];
  ...

This code will need to be extended if cloudabi_fd_t is altered. Feel free to send a pull request, but please take some time to audit the existing kernel/runtime implementations when changing this. Thanks!

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

No branches or pull requests

3 participants