On Tue, Mar 7, 2023 at 2:04 PM Daniel Almeida
<daniel.almeida@xxxxxxxxxxxxx> wrote:
>
> Feel free to point me to the best practices around Rust patch
> submission, as the C stuff like checkpatch etc probably does not apply
> yet. I did take care to run clippy though.
Yeah, some nits below as if you had run a script like that :)
> #include <linux/uaccess.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_config.h>
> #include <linux/uio.h>
Please sort these.
> +#![allow(missing_docs)]
Unless there is a very good reason (which should most likely be
documented in that case), please instead document everything. We keep
lints "deny" instead of `-Fmissing_docs` just in case there is a need
somewhere, but that does not mean not documenting is OK in general.
(Also, the module-level attributes would be placed after the docs).
> +///! Virtio abstractions
> +///!
> +///! C header: [`include/linux/virtio.h`](../../../../include/media/virtio.h)
Please add a blank line after the module-level docs.
> +unsafe impl const crate::driver::RawDeviceId for DeviceId {
`unsafe impl` need `SAFETY` comments too, just like for blocks (which
Björn mentioned). I see you did one elsewhere, though.
> + // No `data` pointer.
> + fn to_rawid(&self, _offset: isize) -> Self::RawType {
I see there is no `data` pointer compared to e.g. an `amba_id`, but
what the comment means? Is it about that that is the reason for
`_offset` being unused? Or something else?
> + from_kernel_result! {
We will be going away from `from_kernel_result!` (in case you wonder
when rebasing in the future).
> + // SAFETY: `virtio_device` is guaranteed to be a valid, non-null
> + // pointer. `priv_` was set on probe().
Markdown: probe() -> `probe()`. Same elsewhere where possible to be consistent.
> + // SAFETY:
> + // - we allocated this pointer using `T::Data::into_foreign`,
> + // so it is safe to turn back into a `T::Data`.
Please start the sentences with uppercase (same elsewhere).
> + /// The table of device ids supported by the driver.
Maybe "device ids" -> "`DeviceId`s"?
Somewhere else you used an intra-doc link, which is great. Please use
them wherever possible, e.g. if you go for the type here, maybe it
works too i.e. [`DeviceId`]
> + /// A wrapper over virtqueue_add_sgs()
If you are referring to the C one, normally we note that (i.e. in
order to distinguish it from a Rust one). Also period at the end.
For instance:
/// A wrapper over the C side `virtqueue_add_sgs()`.
However, the function should instead document what it does, rather
than just refer to the C one. Same elsewhere.
> + if buf.is_null() {
> + return None;
> + } else {
> + // SAFETY: if there is a buffer token, it came from
> + // into_foreign() as called in add_sgs.
> + <T::PrivateData as ForeignOwnable>::from_foreign(buf)
> + }
No need for `else` after `return`, so you can remove the indentation
too. Also Markdown.
> + pub(crate) unsafe extern "C" fn vq_callback(vq: *mut bindings::virtqueue) {
> + // SAFETY: the caller should guarantee that vq is valid for the lifetime
> + // of Self.
Markdown and uppercase -- here and elsewhere, e.g.
// SAFETY: The caller should guarantee that `vq` is valid for the lifetime
// of `Self`.
> + device: 0, // The protocol ID.
Please place the comment on top.
> + // let virtqueue = virtio_dev.find_virtqueue::<VirtqueueCallback>(kernel::c_str!(""))?;
// ```rust
// let ...
// ```
Thanks for the patch!
Cheers,
Miguel
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
