Use zero-cost method to construct Errors

[TheSven73]

When constructing an Error, avoid the runtime check which verifies the errno invariant, by:

• trusting that negative return values from certain kernel C functions always satisfy the invariant, and
• using pre-constructed const Errors if an error return is needed in Rust code.

For the time being, no explicit Error invariant runtime checks are used or required anywhere, so we can eliminate the function which carries out the check.

[ojeda]

The first two commits have the same title and mostly the same message, which is a bit confusing: perhaps: "rust: introduce zero-cost from_kernel_int_result_uint" or something like that to see the difference between them?

(I noted to add a @ksquirrel check for this)

[ojeda]

The third commit is related to #324 (comment) and #283 -- so not sure what to do. Also, I think @foxhlchen was going to send a PR to discuss moving it to return Result<Error> (i.e. Result<Error, Error>).

I think this is a good candidate for discussion this Saturday (and likely also in a technical meeting next week or so).

[TheSven73]

so not sure what to do. Also, I think @foxhlchen was going to send a PR to discuss moving it to return Result<Error> (i.e. Result<Error, Error>).

Perhaps we can bypass this whole discussion by not removing the runtime check function? That will of course generate a warning, as the function is now unused. I can add a dead-code marker. How does that sound?

[ojeda]

match statements are great, but I think for cases like this where it reduces to a single condition it looks easier to read:

Suggested change

	match retval {
	success if success >= 0 => Ok(()),
	// SAFETY: Safety above and match arm guarantee that `errno` is a valid negative `errno`.
	errno => Err(Error::from_kernel_errno_unchecked(errno)),
	}
	if retval < 0 {
	// SAFETY: Safety above and match arm guarantee that `errno` is a valid negative `errno`.
	return Err(Error::from_kernel_errno_unchecked(errno));
	}
	Ok(())

[ojeda]

For the comment, perhaps:

Suggested change

	// SAFETY: Safety above and match arm guarantee that `errno` is a valid negative `errno`.
	// SAFETY: This condition together with the function precondition guarantee
	// `errno` is a valid negative `errno`.

[ojeda]

Perhaps we can bypass this whole discussion by not removing the runtime check function? That will of course generate a warning, as the function is now unused. I can add a dead-code marker. How does that sound?

Sure, feel free to split the PR.

[ksquirrel]

Review of ec1faf3ac524:

• ✔️ Commit 5e932f0: Looks fine!
• ✔️ Commit 79fcf6b: Looks fine!
• ✔️ Commit ec1faf3: Looks fine!

[TheSven73]

v1 -> v2:

ojeda:

• fix commit titles
• keep function and remove dead code warning instead of removing function
• use if instead of match to clarify control flow
• clarify function docs
• add unsafe blocks inside unsafe functions
• clarify various comments

[foxhlchen]

The third commit is related to #324 (comment) and #283 -- so not sure what to do. Also, I think @foxhlchen was going to send a PR to discuss moving it to return Result<Error> (i.e. Result<Error, Error>).

I think this is a good candidate for discussion this Saturday (and likely also in a technical meeting next week or so).

You have a call on Saturday?? Can you send me an invitation??

[ojeda]

Of course! We have informal calls every first Saturday of a month. I will send a remainder to the mailing list.

[foxhlchen]

Of course! We have informal calls every first Saturday of a month. I will send a remainder to the mailing list.

Great, Thank you!

[TheSven73]

@ojeda How can I assign a "Prio: low" label to this PR?