On Tue, Aug 02, 2022 at 01:17:25PM -0500, Glenn Washburn wrote:
> On Tue, 2 Aug 2022 18:17:44 +0200
> Daniel Kiper <dki...@net-space.pl> wrote:
>
> > On Fri, Jul 22, 2022 at 02:43:14AM -0500, Glenn Washburn wrote:
> > > A user can now specify UUID strings with dashes, instead of having to
> > > remove
> > > dashes. This is backwards-compatability preserving and also fixes a source
> > > of user confusion over the inconsistency with how UUIDs are specified
> > > between file system UUIDs and cryptomount UUIDs. Since cryptsetup, the
> > > reference implementation for LUKS, displays and generates UUIDs with
> > > dashes
> > > there has been additional confusion when using the UUID strings from
> > > cryptsetup as exact input into GRUB does not find the expected cryptodisk.
> >
> > I expect the confusion comes from differences between older and newer
> > cryptsetup implementations. AIUI older versions used UUIDs without
> > dashes, and this is what GRUB expects today, but newer ones use UUIDs
> > with dashes. Right? If it is true it took me some time to get it from
> > the text above. So, I think it should be improved a bit to make it more
> > clear...
>
> This may be true, I honestly don't know nor recall that being old
> behavior of cryptsetup. Do you want me to assume that it is true, and
> to modify the commit message accordingly? I could hedge and add a
> sentence here like: "It is possible that very old versions of
> cryptsetup displayed the UUID without dashes, accounting for the LUKS
> handling of UUIDs without dashes".
I thought you know this. If not you can ignore my comment.
> > > A new function grub_uuidcasecmp is added that is general enough to be used
> > > other places where UUIDs are being compared.
> > >
> > > Signed-off-by: Glenn Washburn <developm...@efficientek.com>
> > > ---
> > > grub-core/disk/cryptodisk.c | 4 ++--
> > > grub-core/disk/geli.c | 2 +-
> > > grub-core/disk/luks.c | 21 ++++-----------------
> > > grub-core/disk/luks2.c | 15 ++++-----------
> > > include/grub/misc.h | 27 +++++++++++++++++++++++++++
> > > 5 files changed, 38 insertions(+), 31 deletions(-)
> >
> > [...]
> >
> > > diff --git a/include/grub/misc.h b/include/grub/misc.h
> > > index 776dbf8af..39957ecf9 100644
> > > --- a/include/grub/misc.h
> > > +++ b/include/grub/misc.h
> > > @@ -243,6 +243,33 @@ grub_strncasecmp (const char *s1, const char *s2,
> > > grub_size_t n)
> > > - (int) grub_tolower ((grub_uint8_t) *s2);
> > > }
> > >
> > > +/* Do a case insensitive compare of two UUID strings by ignoring all
> > > dashes */
> > > +static inline int
> > > +grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n)
> > > +{
> > > + if (n == 0)
> > > + return 0;
> > > +
> > > + while (*s1 && *s2 && --n)
> > > + {
> > > + /* Skip forward to non-dash on both UUIDs. */
> > > + while ('-' == *s1)
> > > + ++s1;
> > > +
> > > + while ('-' == *s2)
> > > + ++s2;
> > > +
> > > + if (grub_tolower (*s1) != grub_tolower (*s2))
> >
> > I think you took this code from grub_strncasecmp(). It is missing
> > grub_uint8_t
> > casts here too. However, if you take a look at grub_strcasecmp(), a few
> > lines
> > above, you can see casts in similar place. It is really confusing. I thing
> > the
> > casts has been added to drop sign. If I am right then grub_strncasecmp() and
> > grub_uuidcasecmp() both should be fixed.
>
> Yes, this part was just copied. So I'll add the casts here and to
> grub_strcasecmp(). Should I add the changes to grub_strcasecmp() in
> this patch and add a note in the commit message of the change?
Please fix grub_strcasecmp() in separate patch. However, it can be a
part of this patch series.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
