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

[rfc] C API redesign #33

Closed
2 tasks done
cyphar opened this issue Jul 16, 2024 · 2 comments
Closed
2 tasks done

[rfc] C API redesign #33

cyphar opened this issue Jul 16, 2024 · 2 comments

Comments

@cyphar
Copy link
Member

cyphar commented Jul 16, 2024

The current C API is quite unergonomic, and all of the infrastructure for handling errors is quite unfortunate. A much simpler solution would be to just make the API fd-based.

In order to preserve the "nice" errors we have at the moment, rather than doing the Linux thing and return -errno we can return -id where id a unique ID for the error (which can be reused after the user gets the error information). One large benefit of doing it that way is that we no longer need runtime.LockOSThread for our Go bindings and you can still get nice error messages while keeping a mostly-C-like fd-based API.

It would look something like this (using the README.md example and the code from examples/cat.c):

int open_in_root(const char *root_path, const char *unsafe_path)
{
	int liberr = 0;
	int rootfd = -EBADF, handlefd = -EBADF, fd = -EBADF;

	rootfd = open(root_path, O_PATH|O_DIRECTORY);
	if (rootfd < 0) {
		perror("open root path");
		return -1;
	}

	handlefd = pathrs_resolve(rootfd, unsafe_path);
	if (handlefd < 0) {
		liberr = handlefd;
		goto err;
	}

	fd = pathrs_reopen(handlefd, O_RDONLY);
	if (liberr < 0) {
		liberr = fd;
		goto err;
	}

err:
	if (liberr < 0) {
		pathrs_error_t *error = pathrs_errorinfo(err);
		print_error(error);
		pathrs_errorinfo_free(error);
	}

	if (rootfd >= 0)
		close(rootfd);
	if (handlefd >= 0)
		close(handlefd);
	return fd;
}

For operations that don't return an fd, we can just do liberr = pathrs_readlink(...).

(I'm suggesting we drop pathrs_open here too because it isn't really needed, but we could include it. There would be no practical difference other than the error message formatting.)

Some open questions:

  • How will we deal with configuration? Should we just pass a config struct for all operations? Seems kind of ugly... We could make all configuration global but that's not ideal either...
  • Should we keep backtraces in our errors? Will any C user actually care about them? AFAICS, no higher-level language lets you insert your own backtrace information into their backtraces for errors.
@cyphar
Copy link
Member Author

cyphar commented Jul 18, 2024

I think exposing backtraces through the C API is a bit silly for two reasons:

  1. It seems incredibly unlikely that the information will actually be used by anybody (given that you can almost never splice backtraces between languages through bindings). libpathrs is not a big enough library to need such detailed information when debugging and backtraces are still available within Rust.
    • C users almost certainly will never use it too.
  2. snafu 0.8 has migrated to using std::backtrace which hasn't yet stabilised BacktraceFrame (and the current version of BacktraceFrame is so opaque that there's really no use for it).

It's possible we could splice backtraces for Python if we switched to pyo3 (I haven't checked if that's possible though) but even then we wouldn't need to expose it in the C API.

@cyphar
Copy link
Member Author

cyphar commented Jul 18, 2024

The current draft in #34 removes all of the config logic entirely. I think that at best we might just want to have a config object you configure separately (probably using something like #21 which looks like the fsconfig API) and then you pass it to special pathrs_..._c versions of each function that take a config argument.

But we can always add this later.

@cyphar cyphar closed this as completed Jul 18, 2024
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

1 participant