Message ID | 20240122184608.11863-1-dakr@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-34003-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:2bc4:b0:101:a8e8:374 with SMTP id hx4csp2781623dyb; Mon, 22 Jan 2024 11:14:20 -0800 (PST) X-Google-Smtp-Source: AGHT+IEWM31hQ9+K5YOOXN1zu4HcVkmSGAjQ5FP9KFecV1WwyGVUMvGjTubZroDhcxohor4JhvCq X-Received: by 2002:a17:902:ed8b:b0:1d5:c08e:52eb with SMTP id e11-20020a170902ed8b00b001d5c08e52ebmr4201848plj.65.1705950860813; Mon, 22 Jan 2024 11:14:20 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1705950860; cv=pass; d=google.com; s=arc-20160816; b=nHKZlGGH5gMS6pqrDJipwtAOfrU7RYwAFvzeFGMQx0NGQEET+XKkmhcUSfSTvqAGZB j9Vh9kvoK+C5rKWB/mWqXtTjrwbE6yAnWxupm2v6Q2kVFkoggNm5rhdbDR2JdZ9gZ8M8 dvQC35otUO2yDy/vC8GtO0uUEqENPd1M8+kEy/cATKDVW8jpf2Ex8RsZgsnkF1vjxg+b FIHIY/1KcQtgdBFjZ6MAd2uJrsyYuomlFJ229kezX76mcPKkxUsWJ95QgKTWHho9/x1m iL/P+DPoCuXN9XoLEnVZka7xbG0gdhKZodhAcD93OLsZx0LsCzsy3ivKyK71vPFSt4HM 3orQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=9k/EegaiCDxQTsPJYM7AgRXIenTX4Qdv+pyOcBo0/BE=; fh=r7CWFz9DB7Mz6NSuRAzR7V5h8o/NL8HicmuqihPlAkU=; b=XZ8iy3aHlEvLYCtGvUcbdz+8di9uIyo94eF2R+eAWtgKXoE5fFpvVSmhnbMiM1o433 xIE/y4HxgQIwF5l2IQSSqr83ELQ4EioN60Tac5RJn4Zs8mC3LLEUSHzgAkA120dZS/ki kYUewGOejrtQkQgDF4XKIPc+odvmsa+R9jf5zo85o4VIPfVrFO2vB5bLqvZEEk63FogK 9YmnsacBe+tPURV5/QpgJBFGdQoch1ZtOXv1nANWvx4aoYGnQaPmE9aU0JqyErLLGSkl ScqQ4hdIUnrrAhDVGpChfYQMXVFsyDtdVdKeDpLA58vztPgoYhJh/cI9SYCa3ZwYJPPY V7vg== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=IuB9HuEB; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-34003-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-34003-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id k6-20020a170902ba8600b001d5d05ecc1bsi8174771pls.644.2024.01.22.11.14.20 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 22 Jan 2024 11:14:20 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-34003-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=IuB9HuEB; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-34003-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-34003-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 65C20295C95 for <ouuuleilei@gmail.com>; Mon, 22 Jan 2024 19:05:06 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 1A7F35D738; Mon, 22 Jan 2024 18:46:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="IuB9HuEB" Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2827D5C8E7 for <linux-kernel@vger.kernel.org>; Mon, 22 Jan 2024 18:46:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705949178; cv=none; b=lUaMlDcg/KkMUbPCiLyv0TWz6966RoiKHK5AKRHYORrfKROacckvOiG2D+9gHsiIweySoEN5Qac8T4T9GwvPKcvAIifZTbD51CpitmObRY6uftjdsFM7qlobZCavk3ByMlxvrpFhNaNxd/ZCU1kaLCHjDcc8FdtTdE+wMoKeKa8= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705949178; c=relaxed/simple; bh=lwHcE6C75LYyexBkvNLU1jyUyNNxt7s+6RxQN71P8CA=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version; b=HGZucqd9lR62Bem9jp8SBRJjOiZ5xjmikBazsNBuSZSTwa3GyppbnDdqmCHsinC3zsaltJp28w41sYSrrh3/Vq3b77/o1MlekA+U3Fjksqq07wQhcLlU7M1Iu6BrkoJN38CPi0Lg8eaXHguU98gaKEa1U66sqM+e1LjmACI/40o= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=IuB9HuEB; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705949176; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=9k/EegaiCDxQTsPJYM7AgRXIenTX4Qdv+pyOcBo0/BE=; b=IuB9HuEBS4rRHCPWKgRQdeAdDrjcfQg17DmV9JI+OMud3EMfqWUzQgKoXU6l8Lzi0NUbmT AYnhKziQKnp2J9h5J8I8UccVJazN92Gbr4xzsAd8RDE9w+NYR8g6vUiLqEe5o06xdW+s45 HTkCWAx3ZKCOui9maRO6EwGEpzRMJgc= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-519-ysjZvPqyM5ijBPTkPlwEKA-1; Mon, 22 Jan 2024 13:46:14 -0500 X-MC-Unique: ysjZvPqyM5ijBPTkPlwEKA-1 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-33921b95da8so2113638f8f.1 for <linux-kernel@vger.kernel.org>; Mon, 22 Jan 2024 10:46:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705949173; x=1706553973; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=9k/EegaiCDxQTsPJYM7AgRXIenTX4Qdv+pyOcBo0/BE=; b=i+XS3xwoHMoGDeUc5DjSUmrk/Hdl8zzZtMQpULDlWxII1y8VTmbwbHa3BboWC0tSyQ 4VLXG+rW2Shbw5ttDJmYc2Lh2BTS+3Yh8/dIGoQAP4Yd9QcP0fimFLNIhlE26yXpL+VK LO3sc0SsB93owtIkzaPE3wawwUy0w8wZ4AT84O8pOAIupuolfCaywEsSys2O3dJfk9pF pm49pvoaHv1h2Af8qbf3ZnCFsl9qxb3xFLBvhtF+b2FpCHGtiuM/61S8TYPfdp9L5Fn7 dxac52ESmy60JdXuZanckSBXcOSN4GUvJz+aK0VzOjg4Gy8876o1u9IlmZbTKVEijGoV rbsA== X-Gm-Message-State: AOJu0YwbZmO9/aCtrex5ch3/PYszZ4kLxBNCARwQS2RWR+1XCUZbeQXZ XPFx0klh7k14vZJVAqeiPBL27vyBuVDOjY1uB5VcEHCMsilvn1klbCV0/7MGXqdPTzMrStnhvFg 57ohbvpwuPtF1q58jnTjCAAN0wKfZLJwYebLJwRkpP6ZzzBFk6pujXDxvwRvG7w== X-Received: by 2002:a05:600c:6a12:b0:40e:5b66:f4db with SMTP id jj18-20020a05600c6a1200b0040e5b66f4dbmr2605029wmb.88.1705949173072; Mon, 22 Jan 2024 10:46:13 -0800 (PST) X-Received: by 2002:a05:600c:6a12:b0:40e:5b66:f4db with SMTP id jj18-20020a05600c6a1200b0040e5b66f4dbmr2605024wmb.88.1705949172795; Mon, 22 Jan 2024 10:46:12 -0800 (PST) Received: from cassiopeiae.. ([2a02:810d:4b3f:ee94:642:1aff:fe31:a19f]) by smtp.gmail.com with ESMTPSA id l6-20020a7bc346000000b0040d81ca11casm38940253wmj.28.2024.01.22.10.46.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 22 Jan 2024 10:46:12 -0800 (PST) From: Danilo Krummrich <dakr@redhat.com> To: ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, benno.lossin@proton.me, a.hindborg@samsung.com, aliceryhl@google.com Cc: rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, Danilo Krummrich <dakr@redhat.com> Subject: [PATCH] rust: str: add to_ascii_{upper,lower}case() to CString Date: Mon, 22 Jan 2024 19:45:57 +0100 Message-ID: <20240122184608.11863-1-dakr@redhat.com> X-Mailer: git-send-email 2.43.0 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1788819129466737419 X-GMAIL-MSGID: 1788819129466737419 |
Series |
rust: str: add to_ascii_{upper,lower}case() to CString
|
|
Commit Message
Danilo Krummrich
Jan. 22, 2024, 6:45 p.m. UTC
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(+)
base-commit: 610347effc2ecb5ededf5037e82240b151f883ab
Comments
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
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
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
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 >
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 >
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
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 >
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
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
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
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 {