grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a dash-ins


From: Glenn Washburn
Subject: Re: [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner
Date: Wed, 24 Mar 2021 10:29:33 -0500

On Wed, 24 Mar 2021 06:43:34 +0100
Mihai Moldovan <ionic@ionic.de> wrote:

> * On 3/24/21 5:24 AM, Glenn Washburn wrote:
> > Aside from the fact that this doesn't handle negative values of n,
> > this would be fine.
> 
> Neither do the original version or grub_strncasecmp()... and if you
> *can* get n to be negative, I'd be highly impressed!
> 
> Jokes aside, it's unsigned, so there's no way for it to become
> negative. It should probably handle 0 == n in a different way like
> grub_strncasecmp() does, but that's easily added.

Hah, calling with n < 0 has the same effect whether n is signed or not,
so I hadn't noticed it.

> > I don't think its as big a deal as it sounds. There are just two
> > extra arrays on the stack, so not memory management issues.
> 
> Agree.
> 
> 
> > Partly I chose to implement it the way I did by using
> > grub_strncasecmp because its pretty trivial to see just by looking
> > at the patches that I'm not changing any core logic.
> 
> Yeah, that's pretty obvious by it using grub_strncasecmp() in the
> end, but my arguments would be that we don't need two passes over the
> input strings instead of just one and could avoid the copies
> entirely. (Yes, these arguments are clearly over-optimizations.)
> 
> Also, semantically, UUID parsing *is* different from simple string
> parsing, which is also better reflected there.

I agree with this wrt to UUID parsing, but not with regard to UUID
comparison (at least as we use UUID which has no semantic awareness).
It really is just a case insensitive string compare with a limited
character set.

> > But I'm not wedded to my implementation, I just want this
> > functionality. If I get time, I'll make a new version taking this
> > in to account.
> 
> I'm not in any way authoritative, just adding my thoughts. :)
> 
> I don't really care which version gets in, but I figured that since
> it might be a while until this lands in grub proper, I could still
> offer some insights.

Yep, appreciate the feedback. And I think your suggestion makes sense.

Glenn



reply via email to

[Prev in Thread] Current Thread [Next in Thread]