From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 134AF22B598; Thu, 19 Jun 2025 12:41:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750336899; cv=none; b=OAMXAVKP2lZYApprf7lgzNDnGsd9w0J09eCtY68MyUXYWiGj/IFFKJjrkmWhzidC1tTNZHbqw9kZ9osiTlywG7PPAQr5GWN6is7AR3s1PSDcoAHuwaPSsLQZNxncmmCLDCXQ1RS74wHGlxiHJoUdbHfco8Mo4NZg92cfh2AMSPc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750336899; c=relaxed/simple; bh=1hCQouYpGPLYCGUMm7RuelDeleW9YnV3vKdJx2C+Kp4=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=OSAMFpjPNB9muQUqHrnIbk+domN3tGat67OiSBd1bUjp/PYqhgqcIIW5YNCG6/kSZmjilpnHNpRpWr2FMa0QdwGNh+aywoo04n2UE6X/tra1NsZUOQoETTdks1jK0DORXLZpfrbUCroBk9zylWWWI4b1QQQZh+eqniq0cjbdJqo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=nLHnHJyn; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="nLHnHJyn" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 964CFC4CEEA; Thu, 19 Jun 2025 12:41:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1750336898; bh=1hCQouYpGPLYCGUMm7RuelDeleW9YnV3vKdJx2C+Kp4=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=nLHnHJynvcZXruHY3un4m4i2As2KlH6rY6KC8PYMnX1fEcgB+rPqmwnSySo3EvVhz ROlIppyV4loCa/UndXtBvKc8MScOktM7T5F8ZWP3eG5jf2ucATD6xydNPqVlGCi7Jv E/Bxg9lBk9CsYc3gvaENUZFTSgVHq3fyQf9okpht7Ee8BarbzSFTMNRJyuwCbj7Lg/ 8PWv/erct9iJdRBH/OxI606fGGQR4wbOv/iLAin5528cdfzcWqLyuKvgsdUmkN6IjV e1hOXEq68MNmJOL/sRFRaISAde2pxY5yM4grcBwxFs9eljlWa8d/Exxc3OL8aEaxt1 VoB/UC7FHM9lg== From: Andreas Hindborg To: "Benno Lossin" Cc: "Miguel Ojeda" , "Alex Gaynor" , "Boqun Feng" , "Gary Guo" , =?utf-8?Q?Bj=C3=B6rn?= Roy Baron , "Alice Ryhl" , "Masahiro Yamada" , "Nathan Chancellor" , "Luis Chamberlain" , "Danilo Krummrich" , "Nicolas Schier" , "Trevor Gross" , "Adam Bratschi-Kaye" , , , , "Petr Pavlu" , "Sami Tolvanen" , "Daniel Gomez" , "Simona Vetter" , "Greg KH" , "Fiona Behrens" , "Daniel Almeida" , Subject: Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions In-Reply-To: (Benno Lossin's message of "Thu, 19 Jun 2025 14:17:26 +0200") References: <20250612-module-params-v3-v13-0-bc219cd1a3f8@kernel.org> <20250612-module-params-v3-v13-1-bc219cd1a3f8@kernel.org> <871prg7zoh.fsf@kernel.org> User-Agent: mu4e 1.12.9; emacs 30.1 Date: Thu, 19 Jun 2025 14:41:26 +0200 Message-ID: <87jz577vk9.fsf@kernel.org> Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain "Benno Lossin" writes: > On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote: >> I'm having a difficult time parsing. Are you suggesting that we guard >> against implementations of `TryInto` that misbehave? > > Let me try a different explanation: > > The safety requirement for implementing the `FromStrRadix`: > > /// The member functions of this trait must be implemented according to > /// their documentation. > > Together with the functions of the trait: > > /// Parse `src` to [`Self`] using radix `radix`. > fn from_str_radix(src: &BStr, radix: u32) -> Result; > > /// Return the absolute value of [`Self::MIN`]. > fn abs_min() -> u64; > > /// Perform bitwise 2's complement on `self`. > /// > /// Note: This function does not make sense for unsigned integers. > fn complement(self) -> Self; > > Doesn't make sense. What does it mean to return the "absolute value of > [`Self::MIN`]"? We don't have "absolute value" defined for an arbitrary > type. Similarly for `complement` and `from_str_radix`, what does "Parse > `src` to [`Self`] using radex `radix`" mean? It's not well-defined. > > You use this safety requirement in the parsing branch for negative > numbers (the `unsafe` call at the bottom): > > [b'-', rest @ ..] => { > let (radix, digits) = strip_radix(rest.as_ref()); > // 2's complement values range from -2^(b-1) to 2^(b-1)-1. > // So if we want to parse negative numbers as positive and > // later multiply by -1, we have to parse into a larger > // integer. We choose `u64` as sufficiently large. > // > // NOTE: 128 bit integers are not available on all > // platforms, hence the choice of 64 bits. > let val = > u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix) > .map_err(|_| EINVAL)?; > > if val > Self::abs_min() { > return Err(EINVAL); > } > > if val == Self::abs_min() { > return Ok(Self::MIN); > } > > // SAFETY: We checked that `val` will fit in `Self` above. > let val: Self = unsafe { val.try_into().unwrap_unchecked() }; > > Ok(val.complement()) > } > > But you don't mention that the check is valid due to the safety > requirements of implementing `FromStrRadix`. But even if you did, that > wouldn't mean anything as I explained above. > > So let's instead move all of this negation & u64 conversion logic into > the `FromStrRadix` trait. Then it can be safe & the `ParseInt::from_str` > function doesn't use `unsafe` (there still will be `unsafe` in the > macro, but that is fine, as it's more local and knows the concrete > types). > Alright. I guess my safety comments are slightly hand-wavy. Thanks for the suggestion, I'll apply that for next spin. Best regards, Andreas Hindborg