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

[Glenn Washburn]

On Mon, 22 Mar 2021 23:53:16 +0100
Mihai Moldovan <ionic@ionic.de> wrote:

> * On 3/21/21 9:06 PM, Glenn Washburn wrote:
> > I have another version of grub_uuidcasecmp which is more efficient
> > by not copying the UUID bytes to another buffer to strip out the
> > dashes. [...]
> 
> At first, I didn't understand why you would need to copy the original
> UUIDs in the first place, but then ...
> 
> 
> > However, I'm not sure we're so memory constrained that the added
> > complexity (its not trivial to verify correctness) is worth it
> > here, so I've chosen to submit this simpler version.
> 
> I don't think that memory is so scarce that allocating two tiny
> string buffers on the stack would be problematic, but ...
> 
> 
> > If that assumption isn't valid, I can submit
> > the more complex version. This current version uses an extra buffer
> > copy in grub_uuidcasecmp to strip out dashes so that
> > grub_strncasecmp can be called.
> 
> Do you do all of that just to be able to use grub_strncasecmp()?
> 
> 
> Wouldn't it make more sense to just duplicate a bit of code in this
> case and do away with all additional buffers and complexities?
> 
> 
> Untested, written from the top of my head without any syntax check or
> the like:
> 
> 
> static inline int
> grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n)
> {
>   const char *s1 = uuid1,
>              *s2 = uuid2;
> 
>   /*
>    * Assume that n is the number of valid non-dash characters in a
> UUID
>    * and that both UUIDs are sized the same.
>    */
>   for (grub_size_t i = 0; i < n; ++i)
>   {
>     /* Skip forward to non-dash on both UUIDs. */
>     while ('-' == *s1)
>     {
>       ++s1;
>     }
> 
>     while ('-' == *s2)
>     {
>       ++s2;
>     }
> 
>     if (grub_tolower (*s1) != grub_tolower (*s2))
>     {
>       break;
>     }
> 
>     ++s1;
>     ++s2;
>   }
> 
>   return (int) grub_tolower ((grub_uint8_t) *s1)
>     - (int) grub_tolower ((grub_uint8_t) *s2);
> }

Aside from the fact that this doesn't handle negative values of n, this
would be fine. 

> That is, admittedly, copying the actual part of grub_strncasecmp(),
> but we're doing away with all copies and additional buffers and it's
> different enough to warrant being a different function, I figure.

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. 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. 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.

Glenn