Re: [PATCH] rust: types: Add `try_from_foreign()` method

[Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>]

Hi Obei,

Some extra nits below that you can fix for v3 (since a v3 will be
needed anyway due to the compilation error reported in Zulip).

On Tue, Jan 23, 2024 at 9:28 AM Obei Sideg <linux@obei.io> wrote:
>
> Currently `ForeignOwnable::from_foreign()` only works for non-null
> pointers for the existing impls (e.g. Box, Arc). It may create a few

Markdown may be used here for `Box` and `Arc` since you use it for the other.

> duplicate code like:
>
> ```rust
> // `p` is a maybe null pointer
> if p.is_null() {
>   None
> } else {
>   Some(unsafe { T::from_foreign(p) })
> }
> ``

Triple-backquote here.

Also please do the usual indentation for the code.

> Link: https://github.com/Rust-for-Linux/linux/issues/1057
>

No newline between the tags.

> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Matt Gilbride <mattgilbride@google.com>
> Cc: Miguel Ojeda <ojeda@kernel.org>

Normally you don't need to add a Cc tag with the maintainer that is
supposed to pick the patch.

For Alice and Matt, did you have a particular reason to explicitly list them?

> Reviewed-by: Trevor Gross <tmgross@umich.edu>

Since the patch changed substantially, you should not have picked the
`Reviewed-by`, i.e. he did not approve this patch (in particular, he
mentioned this in Zulip himself, so... :)

Also, please note that you should indicate v2/v3/... in the title
(please see https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format).

Finally, I think Alice's comment should be applied, i.e. the
precondition on `ptr` only applies if `ptr` is non-null for this
function, so I think we should add it.

Thanks!

Cheers,
Miguel