From: Andreas Hindborg <a.hindborg@kernel.org>
To: "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com>
Cc: "Boqun Feng" <boqun.feng@gmail.com>,
"Miguel Ojeda" <ojeda@kernel.org>,
"Alex Gaynor" <alex.gaynor@gmail.com>,
"Gary Guo" <gary@garyguo.net>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Benno Lossin" <lossin@kernel.org>,
"Alice Ryhl" <aliceryhl@google.com>,
"Trevor Gross" <tmgross@umich.edu>,
"Danilo Krummrich" <dakr@kernel.org>,
"Jens Axboe" <axboe@kernel.dk>,
linux-block@vger.kernel.org, rust-for-linux@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/9] rust: block,core: rename `RawWriter` to `BufferWriter`
Date: Mon, 07 Jul 2025 16:58:11 +0200 [thread overview]
Message-ID: <87o6twoxoc.fsf@kernel.org> (raw)
In-Reply-To: <CANiq72nfeGwm17kp8OsmpgO-U6xMsuL9KBNwX34Rt1xz-Nxa1Q@mail.gmail.com> (Miguel Ojeda's message of "Mon, 07 Jul 2025 16:18:13 +0200")
"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:
> On Mon, Jun 16, 2025 at 3:26 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Rename the `RawWriter` to `BufferWriter`, wihich is a more suitable name.
>> Also move the module from `block` to `str`.
>
> The prefix should probably be "rust: block,str", or just "rust:".
OK 👍
>
> (This patch would be ideally first in the series rather than in the
> middle, by the way)
I'll move it.
>
>> -pub(crate) struct RawWriter<'a> {
>> +pub struct BufferWriter<'a> {
>
> Since you are re-exporting, can this be kept for the crate?
Yep.
>
>> + /// Create a new [`Self`] instance.
>
> It is not a big deal here, but when you have a "move" commit, please
> try to keep changes to the minimum, e.g. type renaming could be done
> before or after.
Will do.
>
>> + /// Return the position of the write pointer in the underlying buffer.
>> + pub fn pos(&self) -> usize {
>> + self.pos
>> + }
>
> This is not mentioned in the commit message (and should have been a
> different patch too).
Right.
> By the way, cannot you use `{Raw,}Formatter`? You could add a
> formatter that null-terminates automatically and/or that tracks the
> lifetime, but we add the null manually elsewhere.
I'll look into that. It looks like I could just use `RawFormatter`. I
don't recall the reason for `RawWriter`, it's been years and it was
Wedson who introduced it originally.
>
> Speaking of which, aren't you null-terminating manually anyway in `gen_disk.rs`?
>
> Ah, no, you are adding two nulls -- one at the end of the buffer, and
> one in the middle. So the current code will fail if it needs one final
> byte (i.e. when the last null would have been enough).
The null insertion at the call site should be removed, it's a leftover
from before `BufferWriter` handled that.
>
> Given all that, I would say just drop this one, and use the one we
> have. Then we can add a fancier formatter later on independently if
> needed.
Will try that. Thanks for taking a look.
Best regards,
Andreas Hindborg
next prev parent reply other threads:[~2025-07-07 14:58 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-16 13:23 [PATCH 0/9] rnull: add configfs, remote completion to rnull Andreas Hindborg
2025-06-16 13:23 ` [PATCH 1/9] rust: block: remove trait bound from `mq::Request` definition Andreas Hindborg
2025-06-16 13:23 ` [PATCH 2/9] rust: block: add block related constants Andreas Hindborg
2025-06-16 13:23 ` [PATCH 3/9] rust: block,core: rename `RawWriter` to `BufferWriter` Andreas Hindborg
2025-07-07 14:18 ` Miguel Ojeda
2025-07-07 14:58 ` Andreas Hindborg [this message]
2025-07-07 18:25 ` Miguel Ojeda
2025-07-08 8:44 ` Andreas Hindborg
2025-06-16 13:23 ` [PATCH 4/9] rnull: move driver to separate directory Andreas Hindborg
2025-06-16 13:23 ` [PATCH 5/9] rnull: enable configuration via `configfs` Andreas Hindborg
2025-06-16 13:23 ` [PATCH 6/9] rust: block: add `GenDisk` private data support Andreas Hindborg
2025-06-16 13:23 ` [PATCH 7/9] rust: block: mq: fix spelling in a safety comment Andreas Hindborg
2025-06-16 13:23 ` [PATCH 8/9] rust: block: add remote completion to `Request` Andreas Hindborg
2025-06-16 13:23 ` [PATCH 9/9] rnull: add soft-irq completion support Andreas Hindborg
2025-06-16 13:56 ` Johannes Thumshirn
2025-06-17 8:08 ` Andreas Hindborg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87o6twoxoc.fsf@kernel.org \
--to=a.hindborg@kernel.org \
--cc=alex.gaynor@gmail.com \
--cc=aliceryhl@google.com \
--cc=axboe@kernel.dk \
--cc=bjorn3_gh@protonmail.com \
--cc=boqun.feng@gmail.com \
--cc=dakr@kernel.org \
--cc=gary@garyguo.net \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lossin@kernel.org \
--cc=miguel.ojeda.sandonis@gmail.com \
--cc=ojeda@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=tmgross@umich.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.