Re: GRUB 2.12~rc1 released

[Daniel Kiper]

Hi Vitaly,

On Tue, Aug 22, 2023 at 11:14:54AM +0000, Vitaly Kuzmichev wrote:
> Hi Glenn,
>
> On Mon, 2023-08-21 at 14:52 -0500, Glenn Washburn wrote:
> > On Sun, 20 Aug 2023 12:28:57 +0000
> > Vitaly Kuzmichev <vitaly.kuzmichev@rtsoft.de> wrote:
> >
> > > Hello Daniel, Hello Glenn,
> >
> > Hi Vitaly,
> >
> > >
> > > I'm now finally completing my patch for search command to add
> > > support
> > > to search partition devices by PARTUUID, which I submitted in Apr
> > > 2021 [1].
> > > I also integrated support to search by partition label submitted by
> > > Daniel
> > > Wagenknecht in Sep 2022 [2].
> >
> > Awesome! I've had on my list to do this also, but haven't gotten to
> > it.
> > So thank you!
> >
> > > I (hopefully) addressed all the review points raised for both
> > > patches.
> > > I split all the changes into a set of 16 patches to ease your
> > > review.
> >
> > 16 patches sounds like a lot. Does it make sense to break it into so
> > many? I have found that larger patch sets tend to move more slowly,
> > or
> > not at all on this list. I'd suggest you break it up to at least two
> > patch series, one for PARTUUID and one for PARTLABEL.
> >
>
> Ok, I can merge some patches together. Most of them are just adding
> some helper functions to be used in a later patch. I think it is ok to
> fold them into one.
>
> I do not like to split into two patch series because I now have single
> patch for all documentation update. If I split it, the changes would
> intersect and may cause some conflicts if you change the order of
> applying them.
>
> Also I have minor update for 4 filesystem modules to replace some
> common code with a helper function introduced in one of the patches.
> I will fold them too and use "fs:" prefix for subject, if it is ok for
> you.
>
> This should reduce patch series to 7 or so.

I would suggest, as Glenn did, to send them as is. Even if you have 16
of them. Then Glenn and I will be able to suggest what should have to be
merged and what should not.

> > > My question is whould you like to review them now and include in
> > > upcoming
> > > Grub 2.12 release, or should I submit the patchset after release?
> >
> > I would have liked this functionality to be in the upcoming release,
> > especially considering that it will likely be a couple years until
> > the
> > next release. I think there is likely less risk in these changes
> > because they add compartmentalized new features that should not
> > effect
> > current features. So if its broken, then GRUB won't lose any features
> > it didn't already have. On the other hand if its broken in an
> > exploitable way, that would be worse than not including it. I'd
> > suggest
> > sending the patches now and I'll take a look at them. I suspect the
> > chances of Daniel wanting to include them are not great, but in that
> > case it can be picked backup after the release with the series
> > already
> > in progress.
>
> Ok I will send the patches within a day.

Great!

> > > There were some other few attempts to add this functionality to
> > > Grub from
> > > different people since 2021, I think this is worth to be added in
> > > 2.12.
> >
> > I think the point here, if I understand you correctly, is that since
> > this has been submitted before the feature freeze, that it might be
> > eligible as part of the exception to the feature freeze. I think it
> > does add a little weight, but may be not enough and really depends on
> > the nature of the changes (how risky are they?).
>
> I tested the changes in a virtual machine and they work ok.
> PARTUUID related code is running on our x86_64 embedded devices for
> years already.

Cool!

> I saw in the mailing list a problem with GUID unaligned access on Ia64.
> Perhaps this could affect my code too, as I do read of 'struct
> grub_guid_t' members from on-stack 'struct grub_gpt_partentry'. (Should
> I use grub_get_unaligned16/-32 in this case?)

I think we have to sort out this first. I am looking at the issue right
now and will be discussing proposed solutions shortly. Though this should
not hold you on posting the patches.

> Apart from this the risk of including these patches is low.
>
> You can drop the changes related to FS modules if you find them too
> risky.

I cannot promise I will merge your patches because it is late and risky.
Though when we can see the patches we can decide what we can do or cannot.

Daniel