[PATCH v3 0/2] Add Rust version of might_sleep()

[PATCH v3 0/2] Add Rust version of might_sleep()

[Boqun Feng]

Hi,

This is a quick v3 of Tomo's previous version:

v2: https://lore.kernel.org/rust-for-linux/20250410225623.152616-1-fujita.tomonori@gmail.com/

Given Ingo's feedback [1], and the ongoing work at Rust side [2] (thanks
Alice!), I think it's better that we just start to use
Location::file_with_nul() if available, otherwise use a "this is not
supported" as file names for might_sleep() debug output, this should be
sufficient since might_sleep() only uses the file names for debug
output, and it will also trigger a stack trace when error, so some
callsite information can be gather from there too.

Having might_sleep() will unblock the support of read_poll_timeout()
[3], and then wait_gfw_boot_completion() [4] in nova. So hopefully this
will allow use moving forward.

I will a PR to tip including this updated version if no one objects.
Thanks.

[1]: https://lore.kernel.org/rust-for-linux/aB2aAEELa3253nBh@gmail.com/
[2]: https://github.com/rust-lang/rust/issues/141727
[3]: https://lore.kernel.org/lkml/20250220070611.214262-8-fujita.tomonori@gmail.com/
[4]: https://lore.kernel.org/rust-for-linux/20250612-nova-frts-v5-12-14ba7eaf166b@nvidia.com/

Regards,
Boqun

Boqun Feng (1):
  rust: Introduce file_from_location()

FUJITA Tomonori (1):
  rust: task: Add Rust version of might_sleep()

 init/Kconfig        |  3 +++
 rust/helpers/task.c |  6 ++++++
 rust/kernel/lib.rs  | 21 +++++++++++++++++++++
 rust/kernel/task.rs | 22 ++++++++++++++++++++++
 4 files changed, 52 insertions(+)

-- 
2.39.5 (Apple Git-154)

[PATCH v3 1/2] rust: Introduce file_from_location()

[Boqun Feng]

Most of kernel debugging facilities take a nul-terminated string for
file names for a callsite (generated from __FILE__), however the Rust
courterpart, Location, would return a Rust string (not nul-terminated)
from method .file(). And such a string cannot be passed to C debugging
function directly.

There is ongoing work to support a Location::file_with_nul(), which
returns a nul-terminated string from a Location. Since it's still
working in progress, and it will take some time before the feature
finally gets stabilized and the kernel's minimal rustc version might
also take a while to bump to a version that at least has that feature,
introduce a file_from_location() function, which return a warning string
if Location::file_with_nul() is not available.

This should work in most cases because as for now the known usage of
Location::file_with_nul() is only in debugging code (e.g. might_sleep())
and there might be other information reported by the debugging code that
could help locate the problematic function, so missing the file name is
fine at the moment.

Link: https://github.com/rust-lang/rust/issues/141727 [1]
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 init/Kconfig       |  3 +++
 rust/kernel/lib.rs | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index af4c2f085455..16e84f80bcd1 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -142,6 +142,9 @@ config RUSTC_HAS_SPAN_FILE
 config RUSTC_HAS_UNNECESSARY_TRANSMUTES
 	def_bool RUSTC_VERSION >= 108800
 
+config RUSTC_HAS_LOCATION_FILE_WITH_NUL
+	def_bool RUSTC_VERSION >= 108900
+
 config PAHOLE_VERSION
 	int
 	default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE))
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c3..3ea2391c7f91 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,7 @@
 #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))]
 #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))]
 #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))]
+#![cfg_attr(CONFIG_RUSTC_HAS_LOCATION_FILE_WITH_NUL, feature(file_with_nul))]
 
 // Ensure conditional compilation based on the kernel configuration works;
 // otherwise we may silently break things like initcall handling.
@@ -274,3 +275,23 @@ macro_rules! asm {
         ::core::arch::asm!( $($asm)*, $($rest)* )
     };
 }
+
+/// Gets the C string file name of a [`Location`].
+///
+/// If `file_with_nul()` is not available, returns a string that warns about it.
+///
+/// [`Location`]: core::panic::Location
+pub fn file_from_location<'a>(_loc: &'a core::panic::Location<'a>) -> &'a crate::str::CStr {
+    // SAFETY: `Location::file_with_nul()` guarantees to return a string with a NUL terminated.
+    #[cfg(CONFIG_RUSTC_HAS_LOCATION_FILE_WITH_NUL)]
+    unsafe {
+        crate::str::CStr::from_bytes_with_nul_unchecked(_loc.file_with_nul().to_bytes())
+    }
+
+    #[cfg(not(CONFIG_RUSTC_HAS_LOCATION_FILE_WITH_NUL))]
+    {
+        use crate::c_str;
+
+        c_str!("<Location::file_with_nul() not supported>")
+    }
+}
-- 
2.39.5 (Apple Git-154)

[PATCH v3 2/2] rust: task: Add Rust version of might_sleep()

[Boqun Feng]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

Add a helper function equivalent to the C's might_sleep(), which
serves as a debugging aid and a potential scheduling point.

Note that this function can only be used in a nonatomic context.

This will be used by Rust version of read_poll_timeout().

[boqun: Use file_from_location() to get a C string instead of changing
__might_sleep()]

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/helpers/task.c |  6 ++++++
 rust/kernel/task.rs | 22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/rust/helpers/task.c b/rust/helpers/task.c
index 31c33ea2dce6..2c85bbc2727e 100644
--- a/rust/helpers/task.c
+++ b/rust/helpers/task.c
@@ -1,7 +1,13 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/kernel.h>
 #include <linux/sched/task.h>
 
+void rust_helper_might_resched(void)
+{
+	might_resched();
+}
+
 struct task_struct *rust_helper_get_current(void)
 {
 	return current;
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 834368313088..518c42beaad1 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -400,3 +400,25 @@ fn eq(&self, other: &Kuid) -> bool {
 }
 
 impl Eq for Kuid {}
+
+/// Annotation for functions that can sleep.
+///
+/// Equivalent to the C side [`might_sleep()`], this function serves as
+/// a debugging aid and a potential scheduling point.
+///
+/// This function can only be used in a nonatomic context.
+#[track_caller]
+#[inline]
+pub fn might_sleep() {
+    #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
+    {
+        let loc = core::panic::Location::caller();
+        let file = kernel::file_from_location(loc);
+
+        // SAFETY: `file.as_ptr()` is valid for reading and guaranteed to be nul-terminated.
+        unsafe { crate::bindings::__might_sleep(file.as_ptr().cast(), loc.line() as i32) }
+    }
+
+    // SAFETY: Always safe to call.
+    unsafe { crate::bindings::might_resched() }
+}
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[Miguel Ojeda]

On Mon, Jun 16, 2025 at 5:36 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> introduce a file_from_location() function, which return a warning string

returns

>  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))]
>  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))]
>  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))]
> +#![cfg_attr(CONFIG_RUSTC_HAS_LOCATION_FILE_WITH_NUL, feature(file_with_nul))]

I would change the config name to `CONFIG_RUSTC_HAS_FILE_WITH_NUL`
since that is the actual name, i.e. without "location".

By the way, please add a comment on top, like the others, i.e. something like:

    //
    // `feature(file_with_nul)` is expected to become stable. Before Rust
    // 1.89.0, it did not exist, so enable it conditionally.

Alice: the tracking issue uses the wrong name, i.e. with the
`location_*` prefix.

> +/// If `file_with_nul()` is not available, returns a string that warns about it.

Could we give a couple examples, i.e. of each case? (No need to assert anything)

> +        use crate::c_str;

Do we need the `use`?

Thanks!

Cheers,
Miguel

Re: [PATCH v3 2/2] rust: task: Add Rust version of might_sleep()

[Miguel Ojeda]

On Mon, Jun 16, 2025 at 5:36 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> +/// Equivalent to the C side [`might_sleep()`], this function serves as

I am always pleased to see intra-doc links, but wouldn't this one
point to the Rust item? :) i.e. we need to add the link, right?

I would normally ask for an example of typical usage, but do we have
some good C docs on that already that we could link otherwise?

Thanks!

Cheers,
Miguel

Re: [PATCH v3 2/2] rust: task: Add Rust version of might_sleep()

[FUJITA Tomonori]

On Mon, 16 Jun 2025 23:28:31 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Mon, Jun 16, 2025 at 5:36 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>>
>> +/// Equivalent to the C side [`might_sleep()`], this function serves as
> 
> I am always pleased to see intra-doc links, but wouldn't this one
> point to the Rust item? :) i.e. we need to add the link, right?

Ah, you're right.

> I would normally ask for an example of typical usage, but do we have
> some good C docs on that already that we could link otherwise?

I can't find C docs on might_sleep(). Device driver developers
probably don't need to use might_sleep() directly. I can't think of a
good example.

How about adding a link to the header file where the might_sleep macro
is defined, like this?

/// Annotation for functions that can sleep.
///
/// Equivalent to the C side [`might_sleep`] macro, this function serves as
/// a debugging aid and a potential scheduling point.
///
/// This function can only be used in a nonatomic context.
///
/// [`might_sleep`]: srctree/include/linux/kernel.h
#[track_caller]
#[inline]
pub fn might_sleep() {
    #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
    {
        let loc = core::panic::Location::caller();
        let file = kernel::file_from_location(loc);

        // SAFETY: `file.as_ptr()` is valid for reading and guaranteed to be nul-terminated.
        unsafe { crate::bindings::__might_sleep(file.as_ptr().cast(), loc.line() as i32) }
    }

    // SAFETY: Always safe to call.
    unsafe { crate::bindings::might_resched() }
}

Re: [PATCH v3 2/2] rust: task: Add Rust version of might_sleep()

[Miguel Ojeda]

On Tue, Jun 17, 2025 at 5:05 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> How about adding a link to the header file where the might_sleep macro
> is defined, like this?

Sounds good to me. Though, for APIs that have rendered docs, we
typically point to those instead:

    https://docs.kernel.org/driver-api/basics.html#c.might_sleep

(Ideally those kernel.org docs would then have a source view
themselves, but that is a different problem... :)

Cheers,
Miguel

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[Alice Ryhl]

On Mon, Jun 16, 2025 at 11:19:52PM +0200, Miguel Ojeda wrote:
> On Mon, Jun 16, 2025 at 5:36 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > introduce a file_from_location() function, which return a warning string
> 
> returns
> 
> >  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))]
> >  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))]
> >  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))]
> > +#![cfg_attr(CONFIG_RUSTC_HAS_LOCATION_FILE_WITH_NUL, feature(file_with_nul))]
> 
> I would change the config name to `CONFIG_RUSTC_HAS_FILE_WITH_NUL`
> since that is the actual name, i.e. without "location".

We will need to coordinate with
https://github.com/rust-lang/rust/pull/142579

> By the way, please add a comment on top, like the others, i.e. something like:
> 
>     //
>     // `feature(file_with_nul)` is expected to become stable. Before Rust
>     // 1.89.0, it did not exist, so enable it conditionally.
> 
> Alice: the tracking issue uses the wrong name, i.e. with the
> `location_*` prefix.

Fixed.

Alice

Re: [PATCH v3 2/2] rust: task: Add Rust version of might_sleep()

[FUJITA Tomonori]

On Tue, 17 Jun 2025 10:02:22 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Tue, Jun 17, 2025 at 5:05 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> How about adding a link to the header file where the might_sleep macro
>> is defined, like this?
> 
> Sounds good to me. Though, for APIs that have rendered docs, we
> typically point to those instead:
> 
>     https://docs.kernel.org/driver-api/basics.html#c.might_sleep

Ah, somehow, I overlooked that. Thanks!

Boqun, here is the revised version.

diff --git a/rust/helpers/task.c b/rust/helpers/task.c
index 31c33ea2dce6..2c85bbc2727e 100644
--- a/rust/helpers/task.c
+++ b/rust/helpers/task.c
@@ -1,7 +1,13 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/kernel.h>
 #include <linux/sched/task.h>
 
+void rust_helper_might_resched(void)
+{
+	might_resched();
+}
+
 struct task_struct *rust_helper_get_current(void)
 {
 	return current;
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 834368313088..b79d19deb02e 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -400,3 +400,27 @@ fn eq(&self, other: &Kuid) -> bool {
 }
 
 impl Eq for Kuid {}
+
+/// Annotation for functions that can sleep.
+///
+/// Equivalent to the C side [`might_sleep`] macro, this function serves as
+/// a debugging aid and a potential scheduling point.
+///
+/// This function can only be used in a nonatomic context.
+///
+/// [`might_sleep`]: https://docs.kernel.org/driver-api/basics.html#c.might_sleep
+#[track_caller]
+#[inline]
+pub fn might_sleep() {
+    #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
+    {
+        let loc = core::panic::Location::caller();
+        let file = kernel::file_from_location(loc);
+
+        // SAFETY: `file.as_ptr()` is valid for reading and guaranteed to be nul-terminated.
+        unsafe { crate::bindings::__might_sleep(file.as_ptr().cast(), loc.line() as i32) }
+    }
+
+    // SAFETY: Always safe to call.
+    unsafe { crate::bindings::might_resched() }
+}

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[Boqun Feng]

On Mon, Jun 16, 2025 at 11:19:52PM +0200, Miguel Ojeda wrote:
> On Mon, Jun 16, 2025 at 5:36 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > introduce a file_from_location() function, which return a warning string
> 
> returns
> 

Fixed.

> >  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))]
> >  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))]
> >  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))]
> > +#![cfg_attr(CONFIG_RUSTC_HAS_LOCATION_FILE_WITH_NUL, feature(file_with_nul))]
> 
> I would change the config name to `CONFIG_RUSTC_HAS_FILE_WITH_NUL`
> since that is the actual name, i.e. without "location".
> 

Sounds good to me

> By the way, please add a comment on top, like the others, i.e. something like:
> 
>     //
>     // `feature(file_with_nul)` is expected to become stable. Before Rust
>     // 1.89.0, it did not exist, so enable it conditionally.
> 

Will do, one thing though: the comment lines seem to wrap at 78 or 80
chars, so do other lines for conditional features in rust/kernel/lib.rs.
However I believe in Rust code we use 100 chars text width, any
particular reason that I should keep these new lines the same (wrapping
at 80 characters)? Otherwise I will make the new lines wrap at 100.

> Alice: the tracking issue uses the wrong name, i.e. with the
> `location_*` prefix.
> 
> > +/// If `file_with_nul()` is not available, returns a string that warns about it.
> 
> Could we give a couple examples, i.e. of each case? (No need to assert anything)
> 

Sure, will do, but I'm afraid there is only case, unless I misunderstood
you:

/// # Examples
///
/// ```
/// use kernel::file_from_location;
///
/// #[track_caller]
/// fn foo() {
///     let caller = core::panic::Location::caller();
///
///     pr_info!("{}\n", file_from_location(caller));
/// }
///
/// foo();

> > +        use crate::c_str;
> 
> Do we need the `use`?
> 

No need for that, and thanks for catching it!

Regards,
Boqun

> Thanks!
> 
> Cheers,
> Miguel

Re: [PATCH v3 2/2] rust: task: Add Rust version of might_sleep()

[Boqun Feng]

On Tue, Jun 17, 2025 at 09:42:07PM +0900, FUJITA Tomonori wrote:
> On Tue, 17 Jun 2025 10:02:22 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> > On Tue, Jun 17, 2025 at 5:05 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> How about adding a link to the header file where the might_sleep macro
> >> is defined, like this?
> > 
> > Sounds good to me. Though, for APIs that have rendered docs, we
> > typically point to those instead:
> > 
> >     https://docs.kernel.org/driver-api/basics.html#c.might_sleep
> 
> Ah, somehow, I overlooked that. Thanks!
> 
> Boqun, here is the revised version.
> 

Thanks, applied. Although I kept the parentheses because might_sleep()
is a C macro, in the same spirit of [1].

[1]: https://docs.kernel.org/process/maintainer-tip.html#function-references-in-changelogs

Regards,
Boqun

> diff --git a/rust/helpers/task.c b/rust/helpers/task.c
> index 31c33ea2dce6..2c85bbc2727e 100644
> --- a/rust/helpers/task.c
> +++ b/rust/helpers/task.c
> @@ -1,7 +1,13 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +#include <linux/kernel.h>
>  #include <linux/sched/task.h>
>  
> +void rust_helper_might_resched(void)
> +{
> +	might_resched();
> +}
> +
>  struct task_struct *rust_helper_get_current(void)
>  {
>  	return current;
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 834368313088..b79d19deb02e 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -400,3 +400,27 @@ fn eq(&self, other: &Kuid) -> bool {
>  }
>  
>  impl Eq for Kuid {}
> +
> +/// Annotation for functions that can sleep.
> +///
> +/// Equivalent to the C side [`might_sleep`] macro, this function serves as
> +/// a debugging aid and a potential scheduling point.
> +///
> +/// This function can only be used in a nonatomic context.
> +///
> +/// [`might_sleep`]: https://docs.kernel.org/driver-api/basics.html#c.might_sleep
> +#[track_caller]
> +#[inline]
> +pub fn might_sleep() {
> +    #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
> +    {
> +        let loc = core::panic::Location::caller();
> +        let file = kernel::file_from_location(loc);
> +
> +        // SAFETY: `file.as_ptr()` is valid for reading and guaranteed to be nul-terminated.
> +        unsafe { crate::bindings::__might_sleep(file.as_ptr().cast(), loc.line() as i32) }
> +    }
> +
> +    // SAFETY: Always safe to call.
> +    unsafe { crate::bindings::might_resched() }
> +}

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[Miguel Ojeda]

On Tue, Jun 17, 2025 at 3:42 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Will do, one thing though: the comment lines seem to wrap at 78 or 80
> chars, so do other lines for conditional features in rust/kernel/lib.rs.
> However I believe in Rust code we use 100 chars text width, any
> particular reason that I should keep these new lines the same (wrapping
> at 80 characters)? Otherwise I will make the new lines wrap at 100.

We have both styles, so up to you.

It would have been nice to at least know already if `rustfmt` would
eventually land on 80 or 100 for this, even if the automatically
wrapping is not stable :(

> Sure, will do, but I'm afraid there is only case, unless I misunderstood
> you:

I meant the "If `file_with_nul()` is not available" vs. the available
one (since it is mentioned in the docs already).

> /// use kernel::file_from_location;

I would hide this line, since it is a single import of the item itself.

> ///     pr_info!("{}\n", file_from_location(caller));

I would suggest adding a comment on top of this line mentioning the
output it could potentially show, e.g.

    // Output: ...

Thanks for this!

Cheers,
Miguel

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[Miguel Ojeda]

On Tue, Jun 17, 2025 at 11:26 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> We will need to coordinate with
> https://github.com/rust-lang/rust/pull/142579

Upstream Rust just closed it rejecting the change, so no change (so
far) needed for this patch.

It would be nice to know if they feel the signature will stay
reasonably stable now -- we can ask them tomorrow.

Cheers,
Miguel

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[Boqun Feng]

On Tue, Jun 17, 2025 at 05:28:41PM +0200, Miguel Ojeda wrote:
> On Tue, Jun 17, 2025 at 3:42 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Will do, one thing though: the comment lines seem to wrap at 78 or 80
> > chars, so do other lines for conditional features in rust/kernel/lib.rs.
> > However I believe in Rust code we use 100 chars text width, any
> > particular reason that I should keep these new lines the same (wrapping
> > at 80 characters)? Otherwise I will make the new lines wrap at 100.
> 
> We have both styles, so up to you.
> 

I will use 100 characters then.

> It would have been nice to at least know already if `rustfmt` would
> eventually land on 80 or 100 for this, even if the automatically
> wrapping is not stable :(
> 
> > Sure, will do, but I'm afraid there is only case, unless I misunderstood
> > you:
> 
> I meant the "If `file_with_nul()` is not available" vs. the available
> one (since it is mentioned in the docs already).
> 

Yes, but the example would be one, just the output would be different,
hence I said the "only case", but see below:

> > /// use kernel::file_from_location;
> 
> I would hide this line, since it is a single import of the item itself.
> 
> > ///     pr_info!("{}\n", file_from_location(caller));
> 
> I would suggest adding a comment on top of this line mentioning the
> output it could potentially show, e.g.
> 
>     // Output: ...
> 

This actually helped me find a bug in the current implementation: I
should use core::ffi::CStr::to_bytes_with_nul() instead of to_bytes().
Please see below for the update "Examples" section:

/// # Examples
///
/// ```
/// # use kernel::file_from_location;
///
/// #[track_caller]
/// fn foo() {
///     let caller = core::panic::Location::caller();
///
///     // Output:
///     // - If file_with_nul() available: "rust/doctests_kernel_generated.rs"
///     // - otherwise: "<Location::file_with_nul() not supported>"
///     pr_info!("{}\n", file_from_location(caller));
/// }
///
/// # foo();

> Thanks for this!
> 

Thank you!

Regards,
Boqun

> Cheers,
> Miguel

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[Miguel Ojeda]

On Tue, Jun 17, 2025 at 6:58 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> This actually helped me find a bug in the current implementation: I
> should use core::ffi::CStr::to_bytes_with_nul() instead of to_bytes().
> Please see below for the update "Examples" section:

Yeah, writing examples can force us to find issues :)

I guess we could conditionally (`cfg`) assert in the "otherwise" case,
since we already had one case, but I didn't suggest it earlier because
it is a bit heavy, and the interesting case is the other one anyway so
it wouldn't have caught the issue. I guess we could assert it ends
with `.rs` for the interesting one.

By the way, I would avoid the actual filename, i.e. I would give a
more "normal" example instead of the `doctests_kernel_generated` one
of the example itself. Something like:

    // Output:
    // - A path like `rust/kernel/example.rs` if `file_with_nul()` is available.
    // - `<Location::file_with_nul() not supported>` otherwise.

It could make sense to have an intermediate variable (especially if
you end up asserting anything), then you could put the comment on top
of that instead. Then the `pr_*` call could then perhaps show a
"realistic" example, and could inline the variable name, e.g.
something like `{caller_file}: my message\n`.

Anyway, no big deal either way, what you had is also OK.

By the way, I noticed a typo in "with a NUL terminated." above.

Cheers,
Miguel

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[Boqun Feng]

On Tue, Jun 17, 2025 at 07:21:06PM +0200, Miguel Ojeda wrote:
> On Tue, Jun 17, 2025 at 6:58 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > This actually helped me find a bug in the current implementation: I
> > should use core::ffi::CStr::to_bytes_with_nul() instead of to_bytes().
> > Please see below for the update "Examples" section:
> 
> Yeah, writing examples can force us to find issues :)
> 
> I guess we could conditionally (`cfg`) assert in the "otherwise" case,
> since we already had one case, but I didn't suggest it earlier because
> it is a bit heavy, and the interesting case is the other one anyway so
> it wouldn't have caught the issue. I guess we could assert it ends
> with `.rs` for the interesting one.
> 

We already know the full function name from Location::file() ;-) so the
assertion is easy:

	assert_eq!(Ok(caller.file()), file_from_location(caller).to_str());

I didn't add the assertion of the otherwise case because that would
involve either writing another string "<Location ... not supported>" or
define it as a const, both are a bit overkilling to me.

> By the way, I would avoid the actual filename, i.e. I would give a
> more "normal" example instead of the `doctests_kernel_generated` one

Good point.

> of the example itself. Something like:
> 
>     // Output:
>     // - A path like `rust/kernel/example.rs` if `file_with_nul()` is available.
>     // - `<Location::file_with_nul() not supported>` otherwise.
> 
> It could make sense to have an intermediate variable (especially if
> you end up asserting anything), then you could put the comment on top
> of that instead. Then the `pr_*` call could then perhaps show a
> "realistic" example, and could inline the variable name, e.g.
> something like `{caller_file}: my message\n`.
> 

How aobut something below? (I use "" instead of `` on purpose because
the output variable there is a string)

/// Gets the C string file name of a [`Location`].
///
/// If `file_with_nul()` is not available, returns a string that warns about it.
///
/// [`Location`]: core::panic::Location
///
/// # Examples
///
/// ```
/// # use kernel::file_from_location;
///
/// #[track_caller]
/// fn foo() {
///     let caller = core::panic::Location::caller();
///
///     // Output:
///     // - A path like "rust/kernel/example.rs" if file_with_nul() available.
///     // - "<Location::file_with_nul() not supported>" otherwise.
///     let caller_file: &kernel::str::CStr = file_from_location(caller);
///
///     // Prints out the message with caller's file name.
///     pr_info!("foo() called in file {caller_file}\n");
///
///     # if cfg!(CONFIG_RUSTC_HAS_FILE_WITH_NUL) {
///     #     assert_eq!(Ok(caller.file()), caller_file.to_str());
///     # }
/// }
///
/// # foo();
/// ```


> Anyway, no big deal either way, what you had is also OK.
> 
> By the way, I noticed a typo in "with a NUL terminated." above.
> 

You mean it should be "with an NUL terminated"? Or it should be "with
a `NUL` byte terminated"?

Regards,
Boqun

> Cheers,
> Miguel

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[Miguel Ojeda]

On Tue, Jun 17, 2025 at 8:12 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> How aobut something below? (I use "" instead of `` on purpose because
> the output variable there is a string)

Looks much better, thanks!

> You mean it should be "with an NUL terminated"? Or it should be "with
> a `NUL` byte terminated"?

Ah, I meant that "terminated" sounded strange to me, i.e. it sounds as
if the NUL is what is terminated. But I am not a native speaker.

I would have expected e.g. "a NUL terminated string" or variations
like "a string terminated with a NUL" or"a string with a NUL
termination byte", if that makes sense.

Cheers,
Miguel

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[John Hubbard]

On 6/17/25 11:22 AM, Miguel Ojeda wrote:
> On Tue, Jun 17, 2025 at 8:12 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>>
>> How aobut something below? (I use "" instead of `` on purpose because
>> the output variable there is a string)
> 
> Looks much better, thanks!
> 
>> You mean it should be "with an NUL terminated"? Or it should be "with
>> a `NUL` byte terminated"?
> 
> Ah, I meant that "terminated" sounded strange to me, i.e. it sounds as
> if the NUL is what is terminated. But I am not a native speaker.
> 
> I would have expected e.g. "a NUL terminated string" or variations
> like "a string terminated with a NUL" or"a string with a NUL
> termination byte", if that makes sense.


Yes, instead of this:

    "guarantees to return a string with a NUL terminated."

...you will want this:

     "guarantees to return a NUL-terminated string."

And depending on whether you prefer to speak Kernel, or R4L here,
note that the kernel normally spells this as NULL (two L's).

thanks,
John Hubbard

Re: [PATCH v3 1/2] rust: Introduce file_from_location()

[Boqun Feng]

On Tue, Jun 17, 2025 at 08:22:19PM +0200, Miguel Ojeda wrote:
> On Tue, Jun 17, 2025 at 8:12 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > How aobut something below? (I use "" instead of `` on purpose because
> > the output variable there is a string)
> 
> Looks much better, thanks!
> 
> > You mean it should be "with an NUL terminated"? Or it should be "with
> > a `NUL` byte terminated"?
> 
> Ah, I meant that "terminated" sounded strange to me, i.e. it sounds as
> if the NUL is what is terminated. But I am not a native speaker.
> 
> I would have expected e.g. "a NUL terminated string" or variations
> like "a string terminated with a NUL" or"a string with a NUL
> termination byte", if that makes sense.
> 

I made it "a NUL-terminated string" to align with other parts of our
documentation in kernel::str.

Regards,
Boqun

> Cheers,
> Miguel