Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56684263:Fastmail
Message-ID: <c9b27926-fcf5-b343-e70c-799a684faa75@ryhl.io>
Date:   Tue, 2 May 2023 20:17:04 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH] rust: str: add conversion from `CStr` to `CString`
Content-Language: en-US
To:     Benno Lossin <benno.lossin@proton.me>,
        Wedson Almeida Filho <wedsonaf@gmail.com>
Cc:     Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>,
        =?UTF-8?Q?Bj=c3=b6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>,
        rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
        patches@lists.linux.dev
References: <20230502125306.358283-1-aliceryhl@google.com>
 <CANeycqo+htksD_nj-eyCTOtE0TQHJR+dHZ1Hu8cL5eXNt4cd=A@mail.gmail.com>
 <LUCgVpZBQNex70PxHGlNVauiTqldfXpTUbd9l0tkBl4yHBQ6qWIhIQu4eZn03UG4a3jrKqt4FyL81321CSXgdISmzYnn69DejmZ-UgXJgD0=@proton.me>
From:   Alice Ryhl <alice@ryhl.io>
In-Reply-To: <LUCgVpZBQNex70PxHGlNVauiTqldfXpTUbd9l0tkBl4yHBQ6qWIhIQu4eZn03UG4a3jrKqt4FyL81321CSXgdISmzYnn69DejmZ-UgXJgD0=@proton.me>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/2/23 20:02, Benno Lossin wrote:
> On 02.05.23 18:59, Wedson Almeida Filho wrote:
>> On Tue, 2 May 2023 at 09:53, Alice Ryhl <aliceryhl@google.com> wrote:
>>>
>>> +impl<'a> TryFrom<&'a CStr> for CString {
>>> +    type Error = TryReserveError;
>>
>> Wouldn't `AllocError` make more sense? Or even Error (with ENOMEM value).
>>
>> `TryReserveError` is documented as "The error type for try_reserve
>> methods." -- that fact the we use a `Vec` is an implementation detail,
>> I feel it's better not to leak this fact through the public API.
> 
> I agree, it should be `AllocError`. There is a `From<AllocError> for Error`
> with `ENOMEM` as the value, so `AllocError` is the most compatible, since it
> simply converts to `Error` via `?`.

Sounds good to me.

> Technically, `TryReserveError` represents two different kinds of errors:
> - CapacityOverflow -- triggered when exceeding `isize::MAX` bytes of size
> - AllocError -- memory allocation failed
> 
> I think it is fine to coalesce these into `AllocError`, since allocating
> `isize::MAX` might as well be considered an OOM error.
In fact, the `isize::MAX` case is unreachable since that would require 
you to already have a `&CStr` of that size, which Rust does not allow.

> With that fixed:
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>

Thanks both of you. I'll submit a v2 tomorrow.

Alice