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". > > > 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? > > > + break; > > + > > + s1++; > > + s2++; > > + } > > + > > + return (int) grub_tolower ((grub_uint8_t) *s1) > > + - (int) grub_tolower ((grub_uint8_t) *s2); > > ... and these casts suggests again something is wrong above too... > > Daniel Glenn _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel