Remove incorrect mount API and allow null data arg

[SUPERCILEX]

Added in error in #1024 due to a misunderstanding of how the API works. I've made a tweak to the existing API to allow passing in null options (should be backwards compatible!) as some poorly designed FS might require this, but in practice you could have just passed in the empty string.

[morr0ne]

I have to disagree with the assertion that the API introduced in #1024 is flawed. The current implementation adheres to the documented requirements and allows necessary functionality. It is ultimately the responsibility of the user to utilize the kernel apis appropriately. The crate offers safe wrappers, and the mount2 function does not introduce unsound behavior.

That said, I am not against this changes to the api, provided they maintain backward compatibility. However, it appears that deprecating mount2 offers no tangible benefit.

Additionally, I would like to point out that I am unsure whether the proposed use of impl syntax is supported by the current msrv. As such, this change may not actually be backward compatible.

[SUPERCILEX]

It is ultimately the responsibility of the user to utilize the kernel apis appropriately.

That's not how rustix works: we don't add APIs that directly mirror the kernel syscalls. Instead, rustix APIs are "actions" that may or may not use the same underlying syscall. For examples, look at epoll, fcntl, ioctl, the entire net module, etc etc etc. If you want user responsibility, use C. :)

impl syntax

We've been using the impl trait syntax for ages, it's fine.

[morr0ne]

That's not how rustix works: we don't add APIs that directly mirror the kernel syscalls. Instead, rustix APIs are "actions" that may or may not use the same underlying syscall. For examples, look at epoll, fcntl, ioctl, the entire net module, etc etc etc. If you want user responsibility, use C. :)

I am familiar with rustix's functionality. However, I fail to identify any significant flaws in the mount2 api. The suggestion to switch to C is neither helpful nor professional. I added an api tailored to an use case which rustix did not address. These changes were approved, and I have not been presented with sufficient evidence to justify their deprecation.

[SUPERCILEX]

mount2 is a raw API and in the same spirit as writing C. There is no valid mount configuration where source and file_system_type are null, therefore it is not an appropriate rustix API.

[SUPERCILEX]

One thing I forgot to address is the path::Arg to &CStr change. I think &CStr is indeed more correct from a theoretical standpoint but annoying in practice. I'd argue path::Arg is a bad name as it's really just a "gimme CStr plz" trait which means the current API allows passing in a byte array, whereas if we go with the more theoretically correct &CStr approach, users will be on their own when it comes to conversions.

[sunfishcode]

mount2 is a raw API and in the same spirit as writing C. There is no valid mount configuration where source and file_system_type are null, therefore it is not an appropriate rustix API.

Rustix often does think in this direction, though I wouldn't consider it an absolute rule. And on the other hand, Rustix also thinks in the direction of splitting functions that have overloaded meanings into multiple functions to avoid meaningless arguments; see mmap and mmap_anonymous for example. So I don't have a strong sense either way.

Concerning path::Arg vs. &CStr, rustix's current convention in most things is to use &CStr in the public API for string arguments that are not paths. That's not technically necessary, because path::Arg is indeed just "give me a CStr", but there aren't many things in Linux's entire syscall ABI that use strings for anything other than paths, and I find it mildly nice to differentiate those places in some way. I'd be open to migrating to a different approach though.

I don't know which Rust version introduced impl trait syntax, but we do have MSRV checking in CI which should catch any problems.

[SUPERCILEX]

Are there other examples of c-like rustix interfaces? Off the top of my head an obvious one is open, but there's an item in the 1.0 release to consider splitting it between create and open so the Mode argument isn't meaningless in non-create cases. In general, I think rustix should strive to avoid meaningless arguments as much as possible—IIRC the nix crate was more focused on matching the c APIs so I kinda see folks using either crate depending on their preferred approach.

I don't have a strong preference regarding not using path::Arg, but I do think it would be quite unfortunate to lose easy byte to CStr conversions. Maybe Arg should just be moved to a convert module? And maybe pull out a super trait that doesn't have PathBuf/Path conversions (not sure that's necessary)?

[SUPERCILEX]

Actually I kind of like the idea of leaving path::Arg where it is and creating a super trait convert::Arg (or maybe ffi::Arg) which has implementations for everything not in std::path (so no components, DecInt, etc). That makes the path vs not distinction clearer while still giving you useful conversions.

Also I just saw the futex rework landed! That further reinforces the idea that rustix is moving away from c like APIs—mount2 is a step backwards.

[sunfishcode]

I think this makes sense to do.

[sunfishcode]

Since we're nearing a 1.0 release, we can just delete this function instead of deprecating it.

[sunfishcode]

Also, could you change Data to &CStr?

I know that's not quite as fun as path::Arg, but as discussed earlier in the issue, that's for filesystem paths. The idea of splitting an ffi::Arg out seems fun, but there seem to be very few things in rustix that would use it, so it's not clearly worth it. And now that Rust has C-string literals, plain &CStr is pretty ergonomic in common cases.

[SUPERCILEX]

Fair enough, done.

[sunfishcode]

Thanks!

[SUPERCILEX]

Oh wait shoot, sorry I forgot to move the impl to a normal generic.

[SUPERCILEX]

Fixed in #1317