Re: [PATCH] earlycpio: constify find_cpio_data()'s "data" parameter

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Oct 28, 2024 at 4:51 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 28.10.2024 17:45, Andrew Cooper wrote:
> > On 28/10/2024 4:25 pm, Jan Beulich wrote:
> >> On 28.10.2024 17:18, Andrew Cooper wrote:
> >>> On 28/10/2024 4:12 pm, Jan Beulich wrote:
> >>>> On 28.10.2024 17:07, Andrew Cooper wrote:
> >>>>> On 28/10/2024 4:03 pm, Jan Beulich wrote:
> >>>>>> As with 9cbf61445cda ("xen/earlycpio: Drop nextoff parameter"): While
> >>>>>> this is imported from Linux, the parameter not being pointer-to-const 
> >>>>>> is
> >>>>>> dubious in the first place and we're not plausibly going to gain a 
> >>>>>> write
> >>>>>> through it.
> >>>>>>
> >>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>>>> You haven't tried compiling this, have you?
> >>>> Of course I have. Is there any subtlety with compiler versions? Or what
> >>>> else am I missing?
> >>> struct cpio_data's copy of this field is non-const (which you keep on
> >>> noting that new compilers will object to),
> >> New compilers? I'm afraid I'm missing context. With gcc14 the patch builds
> >> fine. I didn't try _older_ ones (but I see no reason why they might object;
> >> see below).
> >>
> >>> and you can't change that
> >>> without breaking the build in microcode.
> >> I don't need to change that, "thanks" to
> >>
> >>                      cd.data = (void *)dptr;
> >>
> >> casting away const-ness. That is - compilers ought to be fine with the
> >> change; Misra won't like it.
> >
> > You have literally complained about patches of mine on the grounds of
> > "GCC is about to start caring about casting away const on a void pointer".
>
> I still don't remember what context this was in, I'm sorry.
>
> > So which is it.
>
> I'm not adding any such casts; the (potentially problematic) cast is
> there already. I therefore still don't see what's wrong with the patch.
>

You usually don't want some const data to be silently transformed to
no-const data. In this case the "find_cpio_data" is getting a no-const
pointer "data" and returning it into "cpio_data.data". As
"cpio_data.data" is no-const for the previously stated rule the
initial data (that is "data" pointer) should not be const.

Internally you change from no-const to const with the assignment to
"p" and than "dptr". However the "find_cpio_data" function has
knowledge of the original no-const so it uses that knowledge for the
no-const conversion done by "cd.data = (void *)dptr". That makes that
conversion less "silent".

> >>> Nothing of this form can be taken until the constness is consistent in
> >>> microcode, after which yes it can mostly become const.
> >> We can move there in steps, can't we?
> >
> > Or you can stop trying to insist that I rebase around an
> > incorrect/incomplete patch, just for the sake of the const of one void
> > pointer, which can still be laundered by this function.
>
> Okay, I won't insist; take my ack as unconditional one. I still consider
> it a bad precedent though that we'd set, when elsewhere we ask for const-
> correctness wherever possible.
>
> > Especially when you could wait the ~day it will take to get an
> > otherwise-good series in, and then change cpio and get all of the const
> > problems in one go.
>
> If that turns out to be true, all will indeed be fine in the end. Question
> is whether we really want to diverge earlycpio.c by more than minimal
> changes.
>
> Jan
>

Frediano