some aside maintainer advice (was Re: [PATCH v3] rust: alloc: fix dangling pointer in VecExt<T>::reserve())

[David Airlie <airlied@redhat.com>]

> On Mon, 6 May 2024 at 12:22, Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > On 5/6/24 16:11, Wedson Almeida Filho wrote:
> > > On Wed, 1 May 2024 at 10:48, Danilo Krummrich <dakr@redhat.com> wrote:
> > >>
> > >> Currently, a Vec<T>'s ptr value, after calling Vec<T>::new(), is
> > >> initialized to Unique::dangling(). Hence, in VecExt<T>::reserve(), we're
> > >> passing a dangling pointer (instead of NULL) to krealloc() whenever a new
> > >> Vec<T>'s backing storage is allocated through VecExt<T> extension
> > >> functions.
> > >>
> > >> This only works as long as align_of::<T>(), used by Unique::dangling() to
> > >> derive the dangling pointer, resolves to a value between 0x0 and
> > >> ZERO_SIZE_PTR (0x10) and krealloc() hence treats it the same as a NULL
> > >> pointer however.
> > >>
> > >> This isn't a case we should rely on, since there may be types whose
> > >> alignment may exceed the range still covered by krealloc(), plus other
> > >> kernel allocators are not as tolerant either.
> > >>
> > >> Instead, pass a real NULL pointer to krealloc_aligned() if Vec<T>'s
> > >> capacity is zero.
> > >>
> > >> Fixes: 5ab560ce12ed ("rust: alloc: update `VecExt` to take allocation flags")
> > >> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > >> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > >> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> > >> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > >
> > > Reviewed-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > >
> > >> +
> > >> +        // We need to make sure that `ptr` is either NULL or comes from a previous call to
> > >> +        // `krealloc_aligned`. A `Vec<T>`'s `ptr` value is not guaranteed to be NULL and might be
> > >> +        // dangling after being created with `Vec::new`. Instead, we can rely on `Vec<T>`'s capacity
> > >> +        // to be zero if no memory has been allocated yet.
> > >> +        let ptr = match cap {
> > >> +            0 => ptr::null_mut(),
> > >> +            _ => old_ptr,
> > >> +        };
> > >
> > > I still think this should be an `if`.
> >
> > Is there any benefit using an if here, or is that your personal preference?
>
> Readability.
>
> I don't remember ever seeing anyone implement a != 0 comparison with a
> match or switch.
>
> But if you insist on innovating on what seems poor style to me, go
> ahead. I'll fix it later.

I'd like to just respond with a bit of maintainer experience here,
please try and use less suggestive language and more requirements
language in reviews if you want to see something change.

"I still think this should be an 'if'" allows for disagreement and
discussion, but clearly in some cases you want to directly inform the
patch submitter of some maintainer requirement, I consider stylistic
preference one of those cases.

If you are in the position as a code maintainer that you say "do
whatever, I'll just fix it up myself later", it comes across as
passive aggressive, whereas you could just have said that in the
initial review,  "change this to an if for consistent style" or
because of maintainer preference it leaves out any ambiguity that you
want to be see the change in order to merge it. In my experience most
contributors will just make changes and move on, and are happy with
less ambiguity.

I'd suggest if we wanted to establish conventions and rules around if
vs match we should hash it out on zulip and update some docs
somewhere, or we can just leave it as is and have maintainers state
their requirements to avoid ambiguity.

Dave.