# Meeting 2023-09-27
<!-- Leave your topic starting with ### in the relevant sections below -->

## Critical
<!-- bugs, soundness issues, urgent patches/reviews etc. -->

## Status Reports

Gary is working on the `callsite!()` macro but cannot join this week due to travelling/conference. Will report next week.

<!-- You want to report on something you are working on/request reviews.
Or you want to know the status of something someone else is doing -->

## Discussion Questions
<!-- Anything that requires lengthy discussion/more general questions also fit here -->

### Bindgen C Macro constants

Example from [Fujita's PHY driver](https://lore.kernel.org/rust-for-linux/[e-mail]/):

/// Sets duplex mode.
pub fn set_duplex(&mut self, mode: DuplexMode) {
    let phydev = Opaque::get(&self.0);
    // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
    unsafe {
        match mode {
            DuplexMode::Full => (*phydev).duplex = bindings::DUPLEX_FULL as i32,
            DuplexMode::Half => (*phydev).duplex = bindings::DUPLEX_HALF as i32,
            DuplexMode::Unknown => (*phydev).duplex = bindings::DUPLEX_UNKNOWN as i32,
        }
    }
}

I dislike the cast `bindings::DUPLEX_FULL as i32`. Bindgen generates

pub const DUPLEX_FULL: u32 = 1;

from the C code

#define DUPLEX_FULL		0x01

But it is always used as an `int` in the C side.

Miguel: I see `__u8` and `unsigned` uses too.

**Question:** is there anything we can do about this?

### Enum <-> Int conversions

Also see [this email](https://lore.kernel.org/rust-for-linux/[e-mail]/) and above.
The C code defines the constants like this:

/* Duplex, half or full. */
#define DUPLEX_HALF		0x00
#define DUPLEX_FULL		0x01
#define DUPLEX_UNKNOWN		0xff

In Rust Fujita then defines

enum DuplexMode {
    Full = 0,
    Half = 1,
    Unknown = 0xff,
}

and uses int <-> enum conversions when setting the duplex.

**Questions:**
- can we both avoid `as` casts and still have zero cost conversions?
- do we want the Rust compiler to optimize the enum layout for us?

Gary: For both questions: I don't think we need to avoid `as` casts for enums for now. Given https://github.com/rust-lang/rust/pull/81642 is closed, the current idiomatic way to do this is probably still `as` casts.

*However*, for duplex mode, I think we actually have something like this:

#[derive(PartialEq, Eq)]
pub enum Duplex {
    Half,
    Full
}

impl Duplex {
    pub fn to_int(mode: Option<Duplex>) -> i32 {
        match mode {
            Some(Duplex::Half) => bindings::DUPLEX_HALF,
            Some(Duplex::Full) => bindings::DUPLEX_FULL,
            None => bindings::DUPLEX_UNKNOWN,
        } as _
    }

    pub fn from_int(num: i32) -> Option<Duplex> {
        match num as u32 {
            bindings::DUPLEX_HALF => Some(Duplex::Half),
            bindings::DUPLEX_FULL => Some(Duplex::Full),
            _ => None,
        }
    }
}

I don't think accessing duplex mode is a perf bottleneck and using `None` to represent `Unknown` makes so much more semantic sense.

Miguel: https://github.com/rust-lang/rust-bindgen/issues/2646

Small-meeting consensus: let's discuss in the next meeting again; ask `bindgen` for more features around `enum`s and code generation (e.g. `Option` vs. `Result`, `repr`). `derive(...)` is provided via `--with-derive-custom-enum`.

### `to_result`

Miguel: Should we go with Wedson's & Tomonori's suggestion? Should `to_result` still use `Error::from_errno`? i.e. cap the negative values (outside the `ERR_PTR` case which we do differently).

Wedson: I am not aware of a function that uses very negative values for an actual value. We could have two functions if needed in the future. Should we return `c_uint` in the happy path?

Consensus: Yes to return a value from `to_result`. Yes to keep using `Error::from_errno`. Even if someone makes a mistake, this would be converting the happy path into an error, and thus the mistake should be easy to spot (and there is the `pr_warn` too).

Miguel will send a patch series.

## Miscellaneous
<!-- stuff that does not fit into other categories -->