Hi Alistair,
It is nice to see more Rust, thanks!
Some quick consistency nits I noticed for future non-RFC versions (I
didn't check the code). Some apply in several cases.
On Fri, Nov 15, 2024 at 6:46 AM Alistair Francis <alistair@xxxxxxxxxxxxx> wrote:
>
> +#[allow(dead_code)]
It may be possible to use `expect` here instead of `allow` -- e.g. if
it does not depend on conditional compilation.
Also, later in the series, is it used? (I imagine it is a temporary
`allow`? If so, please delete it when you introduce the first use.
`expect` can help here to not forget to delete it.
> +#[repr(u8)]
It is probably a good idea to mention why it needs this `repr`. I
imagine it is related to `SpdmErrorRsp` being `packed` and so on, but
it wouldn't hurt documenting it.
> + InvalidRequest = 0x01,
Please feel free to ignore this one (especially if the idea is to
replace the C implementation eventually, or to just showcase how it
would look like if the C one was removed), but one idea here would be
to pick the values from a common C header? i.e. moving that `enum` to
its own header that both use.
> +//! Top level library, including C compatible public functions to be called
> +//! from other subsytems.
Typo.
> +/// spdm_create() - Allocate SPDM session
I think these are copied from the C one, so it is fine for the RFC,
but the subsystem ends up accepting this, then please use the usual
Markdown style of the rest of the Rust code, instead of kernel-doc
style. While we don't render the docs of these just yet, we will start
doing it at some point, and e.g. IDEs may do so too. Even if we
didn't, the comments could be copied into other docs at some point, so
it is always useful to have them formatted properly.
> + /* Negotiated state */
> + pub(crate) version: u8,
Please use `//`.
> + bindings::EINVAL
> + }
> + };
> +
> + to_result(-(ret as i32))
These are errors you create directly, so you can do directly e.g.
`Err(EINVAL)`, i.e. please avoid `bindings::`.
> + let length = unsafe {
Missing `SAFETY` comment.
If you based this on top of `rust-next` as you noted in the cover
letter, you should be getting a warning under `CLIPPY=1`. There may be
other cleanups under `CLIPPY=1` if you weren't using it so far.
> + return Ok(length); /* Truncated response is handled by callers */
Please use `//` for comments.
> +// SPDX-License-Identifier: GPL-2.0
New line between SPDX and the crate docs.
> +//! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
> +//! https://www.dmtf.org/dsp/DSP0274
This should be a link (using <>) or a link for the "DMTF Security
Protocol and Data Model (SPDM)" text.
> +//! Rust sysfs helper functions
This should be the title, at the top. In Rust the first paragraph
(which typically should be short, e.g. a single line) is considered
the "short description" and used e.g. for lists.
> +//! Copyright (C) 2024 Western Digital
This should be a comment (likely near the SPDX), rather than part of
the documentation -- see e.g. how it is done in `rust/kernel/list.rs`.
> +pub unsafe extern "C" fn rust_authenticated_show(
`unsafe` functions should have a `# Safety` section.
Thanks again!
Cheers,
Miguel
