[PATCH] caseless uuid detection, fixed wrong behaviour for strncasecmp, added strcasecmp

[PATCH] caseless uuid detection, fixed wrong behaviour for strncasecmp, added strcasecmp

[Daniel Mierswa]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

Hi list,
during testing I found that the UUID is checked case-dependend in
search.c, which is probably not wanted (I hope).
Also the grub_strncasecmp function returned (int) *s1 - (int) *s2 which
is wrong if you compare it to the C library strncasecmp.
I fixed that and used the same algorithm which is used in grub_strncmp
(Taking a grub_size_t instead of int and checked the decremented value
in the loop). I also added strcasecmp for consistency reasons which is
used by search.c now.
I'd appreciate your your replies.

-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply
know better than you do.
               --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22

[-- Attachment #2: caseless_uuid.patch --]
[-- Type: text/plain, Size: 2649 bytes --]

Index: kern/misc.c
===================================================================
--- kern/misc.c	(revision 1952)
+++ kern/misc.c	(working copy)
@@ -194,7 +194,7 @@
   while (*s1 && *s2)
     {
       if (*s1 != *s2)
-	return (int) *s1 - (int) *s2;
+        break;
       
       s1++;
       s2++;
@@ -212,7 +212,7 @@
   while (*s1 && *s2 && --n)
     {
       if (*s1 != *s2)
-	return (int) *s1 - (int) *s2;
+        break;
       
       s1++;
       s2++;
@@ -222,21 +222,36 @@
 }
 
 int
-grub_strncasecmp (const char *s1, const char *s2, int c)
+grub_strcasecmp (const char *s1, const char *s2)
 {
-  int p = 1;
+  while (*s1 && *s2)
+    {
+      if (grub_tolower (*s1) != grub_tolower (*s2))
+        break;
+      
+      s1++;
+      s2++;
+    }
 
-  while (grub_tolower (*s1) && grub_tolower (*s2) && p < c)
+  return (int) grub_tolower (*s1) - (int) grub_tolower (*s2);
+}
+
+int
+grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
+{
+  if (n == 0)
+    return 0;
+  
+  while (*s1 && *s2 && --n)
     {
       if (grub_tolower (*s1) != grub_tolower (*s2))
-	return (int) grub_tolower (*s1) - (int) grub_tolower (*s2);
+        break;
       
       s1++;
       s2++;
-      p++;
     }
 
-  return (int) *s1 - (int) *s2;
+  return (int) grub_tolower (*s1) - (int) grub_tolower (*s2);
 }
 
 char *
Index: include/grub/misc.h
===================================================================
--- include/grub/misc.h	(revision 1952)
+++ include/grub/misc.h	(working copy)
@@ -45,7 +45,8 @@
 int EXPORT_FUNC(grub_memcmp) (const void *s1, const void *s2, grub_size_t n);
 int EXPORT_FUNC(grub_strcmp) (const char *s1, const char *s2);
 int EXPORT_FUNC(grub_strncmp) (const char *s1, const char *s2, grub_size_t n);
-int EXPORT_FUNC(grub_strncasecmp) (const char *s1, const char *s2, int c);
+int EXPORT_FUNC(grub_strcasecmp) (const char *s1, const char *s2);
+int EXPORT_FUNC(grub_strncasecmp) (const char *s1, const char *s2, grub_size_t n);
 char *EXPORT_FUNC(grub_strchr) (const char *s, int c);
 char *EXPORT_FUNC(grub_strrchr) (const char *s, int c);
 int EXPORT_FUNC(grub_strword) (const char *s, const char *w);
Index: commands/search.c
===================================================================
--- commands/search.c	(revision 1952)
+++ commands/search.c	(working copy)
@@ -115,7 +115,7 @@
 	      (fs->uuid) (dev, &uuid);
 	      if (grub_errno == GRUB_ERR_NONE && uuid)
 		{
-		  if (grub_strcmp (uuid, key) == 0)
+		  if (grub_strcasecmp (uuid, key) == 0)
 		    {
 		      /* Found!  */
 		      count++;

Re: [PATCH] caseless uuid detection, fixed wrong behaviour for strncasecmp, added strcasecmp

[Pavel Roskin]

On Wed, 2009-01-21 at 13:08 +0100, Daniel Mierswa wrote:
> Hi list,
> during testing I found that the UUID is checked case-dependend in
> search.c, which is probably not wanted (I hope).
> Also the grub_strncasecmp function returned (int) *s1 - (int) *s2 which
> is wrong if you compare it to the C library strncasecmp.

I agree, that's definitely wrong.  Good catch!

> I fixed that and used the same algorithm which is used in grub_strncmp
> (Taking a grub_size_t instead of int and checked the decremented value
> in the loop). I also added strcasecmp for consistency reasons which is
> used by search.c now.
> I'd appreciate your your replies.

The patch looks good to me.  I would split changes to commands/search.c
into a separate commit.

Please provide ChangeLog entries for the patches.

-- 
Regards,
Pavel Roskin

Re: [PATCH] caseless uuid detection, fixed wrong behaviour for strncasecmp, added strcasecmp

[Daniel Mierswa]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

Am 01/21/09 18:30, Pavel Roskin schrieb:
> The patch looks good to me.  I would split changes to commands/search.c
> into a separate commit.
> 
> Please provide ChangeLog entries for the patches.
> 
I will comply. Thanks for your quick response.


-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply
know better than you do.
               --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22

[-- Attachment #2: ChangeLog --]
[-- Type: text/plain, Size: 435 bytes --]

2009-01-23  Daniel Mierswa <impulze@impulze.org>
	* kern/misc.c: add strcasecmp for consistency reasons, use grub_size_t
	               instead of int for strfuncs, fix strncasecmp return values,
	               use the same algorithm in str*casecmp and str*cmp
	* include/grub/misc.h: add str{,n}casecmp, use grub_size_t for strncasecmp

2009-01-23  Daniel Mierswa <impulze@impulze.org>
	* commands/search.c: caseless UUID comparing

[-- Attachment #3: grub2_strfuncs.patch --]
[-- Type: text/plain, Size: 2150 bytes --]

Index: kern/misc.c
===================================================================
--- kern/misc.c	(revision 1952)
+++ kern/misc.c	(working copy)
@@ -194,7 +194,7 @@
   while (*s1 && *s2)
     {
       if (*s1 != *s2)
-	return (int) *s1 - (int) *s2;
+        break;
       
       s1++;
       s2++;
@@ -212,7 +212,7 @@
   while (*s1 && *s2 && --n)
     {
       if (*s1 != *s2)
-	return (int) *s1 - (int) *s2;
+        break;
       
       s1++;
       s2++;
@@ -222,21 +222,36 @@
 }
 
 int
-grub_strncasecmp (const char *s1, const char *s2, int c)
+grub_strcasecmp (const char *s1, const char *s2)
 {
-  int p = 1;
+  while (*s1 && *s2)
+    {
+      if (grub_tolower (*s1) != grub_tolower (*s2))
+        break;
+      
+      s1++;
+      s2++;
+    }
 
-  while (grub_tolower (*s1) && grub_tolower (*s2) && p < c)
+  return (int) grub_tolower (*s1) - (int) grub_tolower (*s2);
+}
+
+int
+grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
+{
+  if (n == 0)
+    return 0;
+  
+  while (*s1 && *s2 && --n)
     {
       if (grub_tolower (*s1) != grub_tolower (*s2))
-	return (int) grub_tolower (*s1) - (int) grub_tolower (*s2);
+        break;
       
       s1++;
       s2++;
-      p++;
     }
 
-  return (int) *s1 - (int) *s2;
+  return (int) grub_tolower (*s1) - (int) grub_tolower (*s2);
 }
 
 char *
Index: include/grub/misc.h
===================================================================
--- include/grub/misc.h	(revision 1952)
+++ include/grub/misc.h	(working copy)
@@ -45,7 +45,8 @@
 int EXPORT_FUNC(grub_memcmp) (const void *s1, const void *s2, grub_size_t n);
 int EXPORT_FUNC(grub_strcmp) (const char *s1, const char *s2);
 int EXPORT_FUNC(grub_strncmp) (const char *s1, const char *s2, grub_size_t n);
-int EXPORT_FUNC(grub_strncasecmp) (const char *s1, const char *s2, int c);
+int EXPORT_FUNC(grub_strcasecmp) (const char *s1, const char *s2);
+int EXPORT_FUNC(grub_strncasecmp) (const char *s1, const char *s2, grub_size_t n);
 char *EXPORT_FUNC(grub_strchr) (const char *s, int c);
 char *EXPORT_FUNC(grub_strrchr) (const char *s, int c);
 int EXPORT_FUNC(grub_strword) (const char *s, const char *w);

[-- Attachment #4: grub2_caseless_uuid.patch --]
[-- Type: text/plain, Size: 407 bytes --]

Index: commands/search.c
===================================================================
--- commands/search.c	(revision 1952)
+++ commands/search.c	(working copy)
@@ -115,7 +115,7 @@
 	      (fs->uuid) (dev, &uuid);
 	      if (grub_errno == GRUB_ERR_NONE && uuid)
 		{
-		  if (grub_strcmp (uuid, key) == 0)
+		  if (grub_strcasecmp (uuid, key) == 0)
 		    {
 		      /* Found!  */
 		      count++;

Re: [PATCH] caseless uuid detection, fixed wrong behaviour for strncasecmp, added strcasecmp

[Pavel Roskin]

On Fri, 2009-01-23 at 10:51 +0100, Daniel Mierswa wrote:
> Am 01/21/09 18:30, Pavel Roskin schrieb:
> > The patch looks good to me.  I would split changes to commands/search.c
> > into a separate commit.
> > 
> > Please provide ChangeLog entries for the patches.
> > 
> I will comply. Thanks for your quick response.

I'll appreciate if you write your Changelog entries according to the GNU
coding standards.  In particular, please don't abbreviate function
names.

> 2009-01-23  Daniel Mierswa <impulze@impulze.org>
> 	* commands/search.c: caseless UUID comparing

We have a very similar function search_fs_uuid() in disk/fs_uuid.c, and
it still uses grub_strcmp().  Should it be using grub_strcasecmp() as
well?  Can we export that function to reuse it in commands/search.c?

-- 
Regards,
Pavel Roskin

Re: [PATCH] caseless uuid detection, fixed wrong behaviour for strncasecmp, added strcasecmp

[Daniel Mierswa]

[-- Attachment #1.1: Type: text/plain, Size: 827 bytes --]

On 26.01.2009 05:41, Pavel Roskin wrote:
> I'll appreciate if you write your Changelog entries according to the GNU
> coding standards.  In particular, please don't abbreviate function
> names.
Ok, second try.

> We have a very similar function search_fs_uuid() in disk/fs_uuid.c, and
> it still uses grub_strcmp().  Should it be using grub_strcasecmp() as
> well?  Can we export that function to reuse it in commands/search.c?
Yes it should use grub_strcasecmp, thanks for noticing. I don't know
exactly if we can reuse that function at another place. I leave that to
someone who knows where this code is executed. :-)


-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply
know better than you do.
               --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22

[-- Attachment #1.2: grub2_caseless_uuid.patch --]
[-- Type: text/plain, Size: 772 bytes --]

Index: commands/search.c
===================================================================
--- commands/search.c	(revision 1954)
+++ commands/search.c	(working copy)
@@ -115,7 +115,7 @@
 	      (fs->uuid) (dev, &uuid);
 	      if (grub_errno == GRUB_ERR_NONE && uuid)
 		{
-		  if (grub_strcmp (uuid, key) == 0)
+		  if (grub_strcasecmp (uuid, key) == 0)
 		    {
 		      /* Found!  */
 		      count++;
Index: disk/fs_uuid.c
===================================================================
--- disk/fs_uuid.c	(revision 1954)
+++ disk/fs_uuid.c	(working copy)
@@ -52,7 +52,7 @@
 		{
 		  (*count)++;
 
-		  if (grub_strcmp (uuid, key) == 0)
+		  if (grub_strcasecmp (uuid, key) == 0)
 		    {
 		      ret = dev;
 		      grub_free (uuid);

[-- Attachment #1.3: ChangeLog --]
[-- Type: text/plain, Size: 484 bytes --]

2009-01-23  Daniel Mierswa <impulze@impulze.org>
	* kern/misc.c: add grub_strcasecmp for consistency reasons, use grub_size_t
	               instead of int for str-functions, fix grub_strncasecmp
	               return values, use the same algorithm in both str-functions
	* include/grub/misc.h: add grub_strcasecmp, use grub_size_t for
	                       grub_strncasecmp

2009-01-23  Daniel Mierswa <impulze@impulze.org>
	* commands/search.c: caseless UUID comparing

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] caseless uuid detection, fixed wrong behaviour for strncasecmp, added strcasecmp

[Pavel Roskin]

On Mon, 2009-01-26 at 19:28 +0100, Daniel Mierswa wrote:
> On 26.01.2009 05:41, Pavel Roskin wrote:
> > I'll appreciate if you write your Changelog entries according to the GNU
> > coding standards.  In particular, please don't abbreviate function
> > names.
> Ok, second try.

I should have told you that I had applied your patch with rewritten
ChangeLog entries.

A quick look at your "second try" shows that you didn't check the GNU
coding standards.  You can find it using Google or another search
engine.  The part dealing with ChangeLogs is here:

http://www.gnu.org/prep/standards/standards.html#Change-Logs

> > We have a very similar function search_fs_uuid() in disk/fs_uuid.c, and
> > it still uses grub_strcmp().  Should it be using grub_strcasecmp() as
> > well?  Can we export that function to reuse it in commands/search.c?
> Yes it should use grub_strcasecmp, thanks for noticing. I don't know
> exactly if we can reuse that function at another place. I leave that to
> someone who knows where this code is executed. :-)

The code is executed when the disk is referenced by its UUID.  OK, I'll
take care of it.

-- 
Regards,
Pavel Roskin

Re: [PATCH] caseless uuid detection, fixed wrong behaviour for strncasecmp, added strcasecmp

[Pavel Roskin]

On Tue, 2009-01-27 at 20:49 -0500, Pavel Roskin wrote:

> > > We have a very similar function search_fs_uuid() in disk/fs_uuid.c, and
> > > it still uses grub_strcmp().  Should it be using grub_strcasecmp() as
> > > well?  Can we export that function to reuse it in commands/search.c?

P.S. The reuse would be impractical.  fs_uuid is a separate module, not
the core.  Reusing search_fs_uuid() would make the "search" module
depend on "fs_uuid".  In my opinion, it's not worth the trouble to
introduce that dependency.  It's not like we would save a lot of code.

-- 
Regards,
Pavel Roskin

Re: [PATCH] caseless uuid detection, fixed wrong behaviour for strncasecmp, added strcasecmp

[ebik]

On Tue, 27 Jan 2009 23:02:52 -0500
Pavel Roskin <proski@gnu.org> wrote:
> P.S. The reuse would be impractical.  fs_uuid is a separate module,
> not the core.  Reusing search_fs_uuid() would make the "search" module
> depend on "fs_uuid".  In my opinion, it's not worth the trouble to
> introduce that dependency.  It's not like we would save a lot of code. 

Inlining or macro may be considered at that point. (I.e., reusing code
via .h file.) If someone will try to change such function (e.g., to
provide HisNewFs) then he need not to patch two places.

-- 
                                 Tomáš 'ebík' Ebenlendr
                                 http://get.to/ebik
                                 PF 2009.07504223744