[PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Danilo Krummrich]

Add functions to convert a CString to upper- / lowercase assuming all
characters are ASCII encoded.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/str.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 7d848b83add4..d21151d89861 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -581,6 +581,16 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
         // exist in the buffer.
         Ok(Self { buf })
     }
+
+    /// Converts the whole CString to lowercase.
+    pub fn to_ascii_lowercase(&mut self) {
+        self.buf.make_ascii_lowercase();
+    }
+
+    /// Converts the whole CString to uppercase.
+    pub fn to_ascii_uppercase(&mut self) {
+        self.buf.make_ascii_uppercase();
+    }
 }
 
 impl Deref for CString {

base-commit: 610347effc2ecb5ededf5037e82240b151f883ab
-- 
2.43.0

Re: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Alice Ryhl]

On Mon, Jan 22, 2024 at 7:46 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> Add functions to convert a CString to upper- / lowercase assuming all
> characters are ASCII encoded.
>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>

What is this for?

Alice

Re: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Greg KH]

On Mon, Jan 22, 2024 at 07:45:57PM +0100, Danilo Krummrich wrote:
> Add functions to convert a CString to upper- / lowercase assuming all
> characters are ASCII encoded.
> 
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/str.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 7d848b83add4..d21151d89861 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -581,6 +581,16 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
>          // exist in the buffer.
>          Ok(Self { buf })
>      }
> +
> +    /// Converts the whole CString to lowercase.
> +    pub fn to_ascii_lowercase(&mut self) {
> +        self.buf.make_ascii_lowercase();
> +    }
> +
> +    /// Converts the whole CString to uppercase.
> +    pub fn to_ascii_uppercase(&mut self) {
> +        self.buf.make_ascii_uppercase();
> +    }

How are you handling locales?

thanks,

greg k-h

Re: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Miguel Ojeda]

On Mon, Jan 22, 2024 at 7:46 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> Add functions to convert a CString to upper- / lowercase assuming all
> characters are ASCII encoded.

Like Alice mentioned, please mention the use case, i.e. the "why?"
(perhaps also linking the Zulip discussion if you like [1]).

[1] https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/String.20manipulation.20in.20kernel.20Rust

> +    /// Converts the whole CString to lowercase.

Please use Markdown and, if possible, an intra-doc link, i.e.

    /// Converts the whole [`CString`] to lowercase.

Also perhaps we should mimic the standard library docs?

> +    pub fn to_ascii_lowercase(&mut self) {
> +        self.buf.make_ascii_lowercase();
> +    }

Why did you choose the `to_ascii_*()` name for these? In the standard
library, the in-place ones are `make_ascii_*()` (like the one you call
in the implementation).

Should the new-object-returned ones be added, by the way, if we are
adding these?

Cheers,
Miguel

Re: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Danilo Krummrich]

On Mon, Jan 22, 2024 at 08:38:26PM +0100, Miguel Ojeda wrote:
> On Mon, Jan 22, 2024 at 7:46 PM Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > Add functions to convert a CString to upper- / lowercase assuming all
> > characters are ASCII encoded.
> 
> Like Alice mentioned, please mention the use case, i.e. the "why?"

Sure, I need this in the context of converting stringified enum values
(representing different GPU chipsets) to strings in order to generate the
corresponding firmware paths. The project context is Nova (GSP only Rust
successor of Nouveau).

If preferred, I can add this to the commit message.

> (perhaps also linking the Zulip discussion if you like [1]).
> 
> [1] https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/String.20manipulation.20in.20kernel.20Rust

Sure, gonna add it.

> 
> > +    /// Converts the whole CString to lowercase.
> 
> Please use Markdown and, if possible, an intra-doc link, i.e.
> 
>     /// Converts the whole [`CString`] to lowercase.
> 
> Also perhaps we should mimic the standard library docs?

Mimic, as in copy them over (to the extent they actually apply)?

> 
> > +    pub fn to_ascii_lowercase(&mut self) {
> > +        self.buf.make_ascii_lowercase();
> > +    }
> 
> Why did you choose the `to_ascii_*()` name for these? In the standard
> library, the in-place ones are `make_ascii_*()` (like the one you call
> in the implementation).

Should be make_ascii_*(), agreed.

> 
> Should the new-object-returned ones be added, by the way, if we are
> adding these?

Sure, I'm fine adding them as well. Not sure we'll need them for Nova though.

- Danilo

> 
> Cheers,
> Miguel
> 

Re: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Danilo Krummrich]

On Mon, Jan 22, 2024 at 11:35:29AM -0800, Greg KH wrote:
> On Mon, Jan 22, 2024 at 07:45:57PM +0100, Danilo Krummrich wrote:
> > Add functions to convert a CString to upper- / lowercase assuming all
> > characters are ASCII encoded.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/kernel/str.rs | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> > index 7d848b83add4..d21151d89861 100644
> > --- a/rust/kernel/str.rs
> > +++ b/rust/kernel/str.rs
> > @@ -581,6 +581,16 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
> >          // exist in the buffer.
> >          Ok(Self { buf })
> >      }
> > +
> > +    /// Converts the whole CString to lowercase.
> > +    pub fn to_ascii_lowercase(&mut self) {
> > +        self.buf.make_ascii_lowercase();
> > +    }
> > +
> > +    /// Converts the whole CString to uppercase.
> > +    pub fn to_ascii_uppercase(&mut self) {
> > +        self.buf.make_ascii_uppercase();
> > +    }
> 
> How are you handling locales?

For ASCII only? Not at all, I guess.

However, std::slice::make_ascii_{lower,upper]case() doesn't seem to handle the
extended range, which tolower() / toupper(), according to _ctype[], does. Do
you mean that?

- Danilo

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Greg KH]

On Mon, Jan 22, 2024 at 11:38:34PM +0100, Danilo Krummrich wrote:
> On Mon, Jan 22, 2024 at 11:35:29AM -0800, Greg KH wrote:
> > On Mon, Jan 22, 2024 at 07:45:57PM +0100, Danilo Krummrich wrote:
> > > Add functions to convert a CString to upper- / lowercase assuming all
> > > characters are ASCII encoded.
> > > 
> > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > ---
> > >  rust/kernel/str.rs | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> > > index 7d848b83add4..d21151d89861 100644
> > > --- a/rust/kernel/str.rs
> > > +++ b/rust/kernel/str.rs
> > > @@ -581,6 +581,16 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
> > >          // exist in the buffer.
> > >          Ok(Self { buf })
> > >      }
> > > +
> > > +    /// Converts the whole CString to lowercase.
> > > +    pub fn to_ascii_lowercase(&mut self) {
> > > +        self.buf.make_ascii_lowercase();
> > > +    }
> > > +
> > > +    /// Converts the whole CString to uppercase.
> > > +    pub fn to_ascii_uppercase(&mut self) {
> > > +        self.buf.make_ascii_uppercase();
> > > +    }
> > 
> > How are you handling locales?
> 
> For ASCII only? Not at all, I guess.

Ah, this is ascii, yes, sorry.  So this is a replacement of
toupper()/tolower() in the C api?

> However, std::slice::make_ascii_{lower,upper]case() doesn't seem to handle the
> extended range, which tolower() / toupper(), according to _ctype[], does. Do
> you mean that?

You should support whatever those functions in the kernel support today,
otherwise why add it?  And why not just call the kernel function to be
sure?

thanks,

greg k-h

Re: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Danilo Krummrich]

On Mon, Jan 22, 2024 at 03:12:04PM -0800, Greg KH wrote:
> On Mon, Jan 22, 2024 at 11:38:34PM +0100, Danilo Krummrich wrote:
> > On Mon, Jan 22, 2024 at 11:35:29AM -0800, Greg KH wrote:
> > > On Mon, Jan 22, 2024 at 07:45:57PM +0100, Danilo Krummrich wrote:
> > > > Add functions to convert a CString to upper- / lowercase assuming all
> > > > characters are ASCII encoded.
> > > > 
> > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > > ---
> > > >  rust/kernel/str.rs | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> > > > index 7d848b83add4..d21151d89861 100644
> > > > --- a/rust/kernel/str.rs
> > > > +++ b/rust/kernel/str.rs
> > > > @@ -581,6 +581,16 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
> > > >          // exist in the buffer.
> > > >          Ok(Self { buf })
> > > >      }
> > > > +
> > > > +    /// Converts the whole CString to lowercase.
> > > > +    pub fn to_ascii_lowercase(&mut self) {
> > > > +        self.buf.make_ascii_lowercase();
> > > > +    }
> > > > +
> > > > +    /// Converts the whole CString to uppercase.
> > > > +    pub fn to_ascii_uppercase(&mut self) {
> > > > +        self.buf.make_ascii_uppercase();
> > > > +    }
> > > 
> > > How are you handling locales?
> > 
> > For ASCII only? Not at all, I guess.
> 
> Ah, this is ascii, yes, sorry.  So this is a replacement of
> toupper()/tolower() in the C api?

It's not a replacement, but it's kinda analogous to that, since the CString
module is mainly used for interoperability with kernel APIs that take C strings.

And I say mainly, because there is no other string implementation in kernel
Rust, hence it might be used independed of whether interoperability is required
or not.

> 
> > However, std::slice::make_ascii_{lower,upper]case() doesn't seem to handle the
> > extended range, which tolower() / toupper(), according to _ctype[], does. Do
> > you mean that?
> 
> You should support whatever those functions in the kernel support today,
> otherwise why add it?  And why not just call the kernel function to be
> sure?

Well, given that CString serves as interoperability layer for kernel APIs that
take C strings, I agree that seems natural.

On the other hand, CString and CStr are designed after the implementation in the
Rust std library and there were requests already to align those functions as
well.

We also need to consider that simply wrapping tolower() and toupper() would make
slice::make_ascii_{lower,upper]case(), str::make_ascii_{lower,upper]case(),
char::make_ascii_{lower,upper]case() and CString::make_ascii_{lower,upper]case()
inconsistent. The former ones already only consider 'a' to 'z' and 'A' to 'Z'
respectively.

As already mentioned in [1], it might just depend on whether we see CString only
as interoperability layer or as the way to deal with strings in kernel Rust in
general.

Just to clarify, personally I'm not worried about whether we consider the
extended range in this specific case or not. I think it's more interesting to
generlly figure out if, for such modules, we want the caller to expect C
bindings to be called or C logic to applied respectively, or if we want the
caller to expect that everything is aligned with the Rust std library.

[1] https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/String.20manipulation.20in.20kernel.20Rust

- Danilo

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Miguel Ojeda]

On Mon, Jan 22, 2024 at 11:16 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> If preferred, I can add this to the commit message.

Ah, yeah, I meant in the commit message. Thanks!

> Mimic, as in copy them over (to the extent they actually apply)?

Yeah -- well, if you think they are better (sometimes they may be,
i.e. I typically mention it when we have something close to the
standard library as a potential source for inspiration).

> Sure, I'm fine adding them as well. Not sure we'll need them for Nova though.

Up to you -- I think either way would be fine, i.e. I would say it is
reasonable to think the others would be useful if these already have a
user (similarly, say, not adding `uppercase` because we only have a
`lowercase` use so far sounds a bit too strict I guess).

In fact, in the Zulip use case you showed, you were using the
new-object one rather than the in-place, no? Or that changed?

Cheers,
Miguel

Re: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Miguel Ojeda]

On Tue, Jan 23, 2024 at 6:24 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> We also need to consider that simply wrapping tolower() and toupper() would make
> slice::make_ascii_{lower,upper]case(), str::make_ascii_{lower,upper]case(),
> char::make_ascii_{lower,upper]case() and CString::make_ascii_{lower,upper]case()
> inconsistent. The former ones already only consider 'a' to 'z' and 'A' to 'Z'
> respectively.

Latter, right? i.e. the kernel ones are the ones that consider the
extended ones.

> Just to clarify, personally I'm not worried about whether we consider the
> extended range in this specific case or not. I think it's more interesting to
> generlly figure out if, for such modules, we want the caller to expect C
> bindings to be called or C logic to applied respectively, or if we want the
> caller to expect that everything is aligned with the Rust std library.

Yeah, it is normal to provide Rust abstractions that follow the naming
and logic of the C side. Having said that, in this particular case, as
you say, since some of these APIs are already in Rust's `core`, I
think it is OK to have the Rust ones completed for `CString` etc. But
if we are to provide the C logic, then we should use the C names.

In other words, in general, what we should definitely avoid is mixing
them, i.e. using the C logic when Rust std names are used, or vice
versa. And maybe we need both the C and the Rust ones in some cases
(should be rare, since it is likely only to come up for things in
`core` like this or perhaps for well-known things in, say, `std`, but
at least for those we do not use them in the kernel so it is a bit
less confusing).

Similarly, it does not hurt to mention whether an API has any subtle
difference (or not) with a similar C API. Sadly, we cannot (easily) do
that also for the existing ones already in `core` too, but it is not a
big deal.

Cheers,
Miguel

Re: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString

[Alice Ryhl]

On Mon, Jan 22, 2024 at 7:46 PM Danilo Krummrich <dakr@redhat.com> wrote:
> +    /// Converts the whole CString to lowercase.
> +    pub fn to_ascii_lowercase(&mut self) {
> +        self.buf.make_ascii_lowercase();
> +    }
> +
> +    /// Converts the whole CString to uppercase.
> +    pub fn to_ascii_uppercase(&mut self) {
> +        self.buf.make_ascii_uppercase();
> +    }
>  }

It looks like these methods are defined on `CString`. However, there's
no requirement that you need *ownership* of the c string to change its
contents - you just need mutable access.

I think it would make more sense to introduce an `impl DerefMut for
CString` that returns a `&mut CStr`, and then define these methods on
`CStr` as `&mut self`. That way, you can still call them on `CString`,
but you can also call it on other mutable c strings.

Alice