grub2: commands/cmp.c: grub_cmd_cmp()

grub2: commands/cmp.c: grub_cmd_cmp()

[Rodrigo Steinmüller Wanderley]

Good Morning!

I'm reading grub2 commands in order to understand the syntax to add new
builtins into it.  When reading the cmp commands I found some points I
couldn't understand, think they are bugs, but maybe some of you guys can
prove me wrong...

I will comment the points I didn't understand here:

static grub_err_t
grub_cmd_cmp (struct grub_arg_list *state __attribute__ ((unused)),
	      int argc, char **args)
{
  /* ... */
  file1 = grub_file_open (args[0]);
  if (! file1)
    return grub_errno;

  file2 = grub_file_open (args[1]);
  if (! file2)
    {
      grub_file_close (file2);

*** Shouldn't we be closing file1 here? ***

      return grub_errno;
    }

  /* ... */

      do
	{
	  int i;
	  rd1 = grub_file_read (file1, buf1, 512);
	  rd2 = grub_file_read (file2, buf2, 512);

	  if (rd1 != rd2)

*** We are returning without closing file1 and file2 ***
	    return 0;

	  for (i = 0; i < 512; i++)
	    {
	      if (buf1[i] != buf2[i])
		{
		  grub_printf ("Differ at the offset %d: 0x%x [%s], 0x%x
[%s]\n",			       i + pos, buf1[i], args[0],
			       buf2[i], args[1]);

		  grub_file_close (file1);
		  grub_file_close (file2);

*** I think here is ok, but wouldn't it be better to use a goto and have
*** a single exit point here?
		  return 0;
		}
	    }
	  pos += 512;
	  
	} while (rd2);
    }

  grub_printf ("The files are identical.\n");

  return 0;
}

What I'm thinking is something like (even though this function still
looks somewhat ugly to me):

static grub_err_t
grub_cmd_cmp (struct grub_arg_list *state __attribute__ ((unused)),
	      int argc, char **args)
{
  grub_file_t file1 = 0;
  grub_file_t file2 = 0;
  grub_err_t err = 0;

  if (argc != 2)
    return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");

  grub_printf ("Compare `%s' and `%s':\n", args[0],
	       args[1]);

  file1 = grub_file_open (args[0]);
  if (! file1) {
    err = grub_errno;
    goto finish;
  }

  file2 = grub_file_open (args[1]);
  if (! file2)
    {
      err = grub_errno;
      goto finish;
    }

  if (grub_file_size (file1) != grub_file_size (file2))
    grub_printf ("Differ in size: %d [%s], %d [%s]\n", 
		 grub_file_size (file1), args[0], 
		 grub_file_size (file2), args[1]);
  
  else
    {
      char buf1[512];
      char buf2[512];
      grub_ssize_t rd1, rd2;
      grub_uint32_t pos = 0;
     
      do
	{
	  int i;
	  rd1 = grub_file_read (file1, buf1, 512);
	  rd2 = grub_file_read (file2, buf2, 512);

	  if (rd1 != rd2)
	    goto finish;

	  for (i = 0; i < 512; i++)
	    {
	      if (buf1[i] != buf2[i])
		{
		  grub_printf ("Differ at the offset %d: 0x%x [%s], 0x%x
[%s]\n",			       i + pos, buf1[i], args[0],
			       buf2[i], args[1]);

		  goto finish;
		}
	    }
	  pos += 512;
	  
	} while (rd2);
      grub_printf ("The files are identical.\n");
    }

 finish:
  if (file1)
    grub_file_close (file1);
  if (file2)
    grub_file_close (file2);

  return err;
}

Best Regards,
  Rodrigo Steinmuller Wanderley


-- 
-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: GnuPG v1.4.1 (GNU/Linux)

mQGiBEKJ288RBAD43+VlxMx8V0dLbU+f7TsbhknjBYp2sRMP0a8IkHa8z4DgJTRd
XRMB0D05Hp5iE/1cA8t3e+g2J4kQhcj1JgUA6KSpYcj/cX6EKb6xhb/GAEQupaXz
7RYglwf4Sz9WJA3roSLtQuWcCOYR9lys+kifeTE2jnDLzDcuzwa2pEYJbwCg04jF
uyOmiBd09P1Bgq4VOQhYM78D/j0Iyj0QIstssnRPWcg4QL9l5c7Y8rLRH63qfGOi
fakmmY6C1JnW/wm4+2iUOc0/DbM+kKS5yXsiRFW7CDeqXLUEF1NIRvNaHkmfRmQf
shDI8NJCr0ULMbUde3b1U0LKgMRr7uVnVRFb2bPkEFh1mDEaxpy376+2Rpn8uHOu
GvqYBACJzY7EPP0fFQMMxeSyxHA7A/lxmC9/s1YtRgBHTCniYOQIZ+kwbFrU9XQv
ExvMeO2DvYAtDNyCgV/PaUm0yLxCAmxSVxQaAMRkOuMSKatyBggLpJVZKQ4WuayL
3xA+ws2+F2ozC/LHK9DodkGen35lP286QyPXOV2WciE4YciU3bQsUm9kcmlnbyBT
IFdhbmRlcmxleSA8cndhbmRlcmxleUBuYXRhbG5ldC5icj6IZAQTEQIAJAUCQonb
zwIbAwUJAeEzgAYLCQgHAwIDFQIDAxYCAQIeAQIXgAAKCRBuM/JKbknmQugDAKC6
ZfWsa8qone19+oppGBkrX028QACfUfLi9rSs/qxmE77b0P+xa2IrWN25Ag0EQonb
2xAIANBkeWLFcVSxSCsCQEH8HJ80VhQO18Sy80MpXebf9sj1gwUATZJ/OcxYYw46
ZrFwNk9raTRULprAcqR5ORKk3TNZ6ZnEl337PZZS5FnELwsHXTm+KVKF3bE2nnB5
/25SzPwkidsyk8Pe3HYM9/r4dwHNOXE3i0nYsweC/aUE8yg/3Ipweu9K1cj+XbSM
IpDydOmBpvVhIvv+VOIoevXxgm2hrD7LQ7jnfBaj/bV9GY/tJyl50nWgMM7csaAg
+4H1lG5/FvzNOgudmhzAdMk5lyTMLyRj6wiYkvckvBCXFaC04FgseylRj72NZilQ
xIstJWNomiATkC6uHYtOKExZ1xcAAwUIALTSG+l21w/W3L9iuEi8QK91n7LyHoO/
OJpYbj73sJWsui7qG63os8aR+KgbdbKNFGDwkyYfbfildYDd+TOkFWkbT64vq4Wv
t51Pl2dB0+0cnO/xqRnbxt4II7SBwg5t1u/MHahaULoTcTYslN+bW9FuB9I22ZiJ
pzFddDWjWApggNQIEapCd+XiuYnED6rV+n0GcmZxpb9Iz0mak7SPCZvN3QzPCI/6
k2YZlt92I/k4E2GU9NVM/1mXkTgqVgwOwlunPW6JYgcv/3n2Ly1eMNJQioWGRSnZ
wQyVx7FvBUqMGLrWHTw3+FQRDd6B6pQ2Y4uL0W4LskQrXm97hhW5NuKITwQYEQIA
DwUCQonb2wIbDAUJAeEzgAAKCRBuM/JKbknmQuXzAKDTa3d+h15/KHHupI6AMkNr
YKRP3ACggEq09XZBLGulCU2e6+/I0j4iN3U=
=JhUw
-----END PGP PUBLIC KEY BLOCK-----

Re: grub2: commands/cmp.c: grub_cmd_cmp()

[Hollis Blanchard]

On Jun 29, 2005, at 6:33 AM, Rodrigo Steinmüller Wanderley wrote:
>
> I'm reading grub2 commands in order to understand the syntax to add new
> builtins into it.  When reading the cmp commands I found some points I
> couldn't understand, think they are bugs, but maybe some of you guys 
> can
> prove me wrong...

I agree; these are all bugs. Good eyes.

-Hollis

Re: [Bulk] grub2: commands/cmp.c: grub_cmd_cmp()

[Vincent Pelletier]

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Rodrigo Steinmüller Wanderley wrote:
> When reading the cmp commands I found some points I
> couldn't understand, think they are bugs, but maybe some of you guys can
> prove me wrong...

You are right :). Thanks for your bug report. Please tell me if this
patch is ok for you.
nb: I haven't tested these changes beyond "make", but I think they are
trivial enough to be trusted...

2005-06-29  Vincent Pelletier  <subdino2004@yahoo.fr>

    * commands/cmp.c
      (grub_cmd_cmp): Close the right file at the right time.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFCwrxoFEQoKRQyjtURAkebAJ0d0+or1iWfjjGvDJUWeQTzPXm8JQCgmvum
QFQJwUA+/ff5iNdG1DaKZ+w=
=4HC3
-----END PGP SIGNATURE-----

[-- Attachment #2: cmp.c.diff --]
[-- Type: text/plain, Size: 1265 bytes --]

Index: cmp.c
===================================================================
RCS file: /cvsroot/grub/grub2/commands/cmp.c,v
retrieving revision 1.2
diff -u -p -r1.2 cmp.c
--- cmp.c	4 Apr 2004 13:46:00 -0000	1.2
+++ cmp.c	29 Jun 2005 15:14:59 -0000
@@ -44,7 +44,7 @@ grub_cmd_cmp (struct grub_arg_list *stat
   file2 = grub_file_open (args[1]);
   if (! file2)
     {
-      grub_file_close (file2);
+      grub_file_close (file1);
       return grub_errno;
     }
 
@@ -67,7 +67,7 @@ grub_cmd_cmp (struct grub_arg_list *stat
 	  rd2 = grub_file_read (file2, buf2, 512);
 
 	  if (rd1 != rd2)
-	    return 0;
+	    goto cleanup;
 
 	  for (i = 0; i < 512; i++)
 	    {
@@ -76,10 +76,7 @@ grub_cmd_cmp (struct grub_arg_list *stat
 		  grub_printf ("Differ at the offset %d: 0x%x [%s], 0x%x [%s]\n",
 			       i + pos, buf1[i], args[0],
 			       buf2[i], args[1]);
-
-		  grub_file_close (file1);
-		  grub_file_close (file2);
-		  return 0;
+		  goto cleanup;
 		}
 	    }
 	  pos += 512;
@@ -87,11 +84,12 @@ grub_cmd_cmp (struct grub_arg_list *stat
 	} while (rd2);
     }
 
+  grub_printf ("The files are identical.\n");
+
+cleanup:
   grub_file_close (file1);
   grub_file_close (file2);
 
-  grub_printf ("The files are identical.\n");
-
   return 0;
 }
 

[PATCH] grub2: commands/cmp.c: grub_cmd_cmp()

[Vincent Pelletier]

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Pelletier wrote:
> nb: I haven't tested these changes beyond "make", but I think they are
> trivial enough to be trusted...

My baaad...
There were 2 bugs left :
- -If the file size isn't a multiple of 512 bytes, we would read
non-initialised or non-updated memory (so it only affects results on
files whom size is < 512)
- -"The files are identical." is displayed when file size differ (was
corrected in Wanderley's version)

Reminds me Hagakure :
"Matters of small concern should be considered seriously".

I'm not sure if it is right to add a macro definition for block size,
and wether the name is good.

2005-06-29  Vincent Pelletier  <subdino2004@yahoo.fr>

    * commands/cmp.c
      (grub_cmd_cmp): Close the right file at the right time.  Compare
      only data just read.  Don't report files of different size as
      identical.  (BLOCK_SIZE): New macro.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFCwsVfFEQoKRQyjtURApNOAKC4bO41ca3vX/m64aIy4hIvjIwW1QCgmbyg
AIn3yvP072FMa0LlDFo1CYM=
=o8fq
-----END PGP SIGNATURE-----

[-- Attachment #2: cmp.c.diff --]
[-- Type: text/plain, Size: 2133 bytes --]

Index: commands/cmp.c
===================================================================
RCS file: /cvsroot/grub/grub2/commands/cmp.c,v
retrieving revision 1.2
diff -u -p -r1.2 cmp.c
--- commands/cmp.c	4 Apr 2004 13:46:00 -0000	1.2
+++ commands/cmp.c	29 Jun 2005 15:47:36 -0000
@@ -24,6 +24,8 @@
 #include <grub/misc.h>
 #include <grub/file.h>
 
+#define BLOCK_SIZE 512
+
 static grub_err_t
 grub_cmd_cmp (struct grub_arg_list *state __attribute__ ((unused)),
 	      int argc, char **args)
@@ -44,54 +46,53 @@ grub_cmd_cmp (struct grub_arg_list *stat
   file2 = grub_file_open (args[1]);
   if (! file2)
     {
-      grub_file_close (file2);
+      grub_file_close (file1);
       return grub_errno;
     }
 
   if (grub_file_size (file1) != grub_file_size (file2))
-    grub_printf ("Differ in size: %d [%s], %d [%s]\n", 
-		 grub_file_size (file1), args[0], 
-		 grub_file_size (file2), args[1]);
+    {
+      grub_printf ("Differ in size: %d [%s], %d [%s]\n", 
+		   grub_file_size (file1), args[0], 
+		   grub_file_size (file2), args[1]);
+    }
   
   else
     {
-      char buf1[512];
-      char buf2[512];
+      char buf1[BLOCK_SIZE];
+      char buf2[BLOCK_SIZE];
       grub_ssize_t rd1, rd2;
       grub_uint32_t pos = 0;
      
       do
 	{
 	  int i;
-	  rd1 = grub_file_read (file1, buf1, 512);
-	  rd2 = grub_file_read (file2, buf2, 512);
+	  rd1 = grub_file_read (file1, buf1, BLOCK_SIZE);
+	  rd2 = grub_file_read (file2, buf2, BLOCK_SIZE);
 
 	  if (rd1 != rd2)
-	    return 0;
+	    goto cleanup;
 
-	  for (i = 0; i < 512; i++)
+	  for (i = 0; i < rd1; i++)
 	    {
 	      if (buf1[i] != buf2[i])
 		{
 		  grub_printf ("Differ at the offset %d: 0x%x [%s], 0x%x [%s]\n",
 			       i + pos, buf1[i], args[0],
 			       buf2[i], args[1]);
-
-		  grub_file_close (file1);
-		  grub_file_close (file2);
-		  return 0;
+		  goto cleanup;
 		}
 	    }
-	  pos += 512;
+	  pos += BLOCK_SIZE;
 	  
 	} while (rd2);
+      grub_printf ("The files are identical.\n");
     }
 
+cleanup:
   grub_file_close (file1);
   grub_file_close (file2);
 
-  grub_printf ("The files are identical.\n");
-
   return 0;
 }
 

Re: [Bulk] grub2: commands/cmp.c: grub_cmd_cmp()

[Rodrigo Steinmüller Wanderley]

Hi Vicent,

29/06 as 17:21: Vincent escreveu:

VP> Please tell me if this
VP> patch is ok for you.

Looks fine!  thanks.

Rodrigo Wanderley

-- 
-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: GnuPG v1.4.1 (GNU/Linux)

mQGiBEKJ288RBAD43+VlxMx8V0dLbU+f7TsbhknjBYp2sRMP0a8IkHa8z4DgJTRd
XRMB0D05Hp5iE/1cA8t3e+g2J4kQhcj1JgUA6KSpYcj/cX6EKb6xhb/GAEQupaXz
7RYglwf4Sz9WJA3roSLtQuWcCOYR9lys+kifeTE2jnDLzDcuzwa2pEYJbwCg04jF
uyOmiBd09P1Bgq4VOQhYM78D/j0Iyj0QIstssnRPWcg4QL9l5c7Y8rLRH63qfGOi
fakmmY6C1JnW/wm4+2iUOc0/DbM+kKS5yXsiRFW7CDeqXLUEF1NIRvNaHkmfRmQf
shDI8NJCr0ULMbUde3b1U0LKgMRr7uVnVRFb2bPkEFh1mDEaxpy376+2Rpn8uHOu
GvqYBACJzY7EPP0fFQMMxeSyxHA7A/lxmC9/s1YtRgBHTCniYOQIZ+kwbFrU9XQv
ExvMeO2DvYAtDNyCgV/PaUm0yLxCAmxSVxQaAMRkOuMSKatyBggLpJVZKQ4WuayL
3xA+ws2+F2ozC/LHK9DodkGen35lP286QyPXOV2WciE4YciU3bQsUm9kcmlnbyBT
IFdhbmRlcmxleSA8cndhbmRlcmxleUBuYXRhbG5ldC5icj6IZAQTEQIAJAUCQonb
zwIbAwUJAeEzgAYLCQgHAwIDFQIDAxYCAQIeAQIXgAAKCRBuM/JKbknmQugDAKC6
ZfWsa8qone19+oppGBkrX028QACfUfLi9rSs/qxmE77b0P+xa2IrWN25Ag0EQonb
2xAIANBkeWLFcVSxSCsCQEH8HJ80VhQO18Sy80MpXebf9sj1gwUATZJ/OcxYYw46
ZrFwNk9raTRULprAcqR5ORKk3TNZ6ZnEl337PZZS5FnELwsHXTm+KVKF3bE2nnB5
/25SzPwkidsyk8Pe3HYM9/r4dwHNOXE3i0nYsweC/aUE8yg/3Ipweu9K1cj+XbSM
IpDydOmBpvVhIvv+VOIoevXxgm2hrD7LQ7jnfBaj/bV9GY/tJyl50nWgMM7csaAg
+4H1lG5/FvzNOgudmhzAdMk5lyTMLyRj6wiYkvckvBCXFaC04FgseylRj72NZilQ
xIstJWNomiATkC6uHYtOKExZ1xcAAwUIALTSG+l21w/W3L9iuEi8QK91n7LyHoO/
OJpYbj73sJWsui7qG63os8aR+KgbdbKNFGDwkyYfbfildYDd+TOkFWkbT64vq4Wv
t51Pl2dB0+0cnO/xqRnbxt4II7SBwg5t1u/MHahaULoTcTYslN+bW9FuB9I22ZiJ
pzFddDWjWApggNQIEapCd+XiuYnED6rV+n0GcmZxpb9Iz0mak7SPCZvN3QzPCI/6
k2YZlt92I/k4E2GU9NVM/1mXkTgqVgwOwlunPW6JYgcv/3n2Ly1eMNJQioWGRSnZ
wQyVx7FvBUqMGLrWHTw3+FQRDd6B6pQ2Y4uL0W4LskQrXm97hhW5NuKITwQYEQIA
DwUCQonb2wIbDAUJAeEzgAAKCRBuM/JKbknmQuXzAKDTa3d+h15/KHHupI6AMkNr
YKRP3ACggEq09XZBLGulCU2e6+/I0j4iN3U=
=JhUw
-----END PGP PUBLIC KEY BLOCK-----

Re: [PATCH] grub2: commands/cmp.c: grub_cmd_cmp()

[Marco Gerards]

Vincent Pelletier <subdino2004@yahoo.fr> writes:

Hi Vincent,

Thanks for your patch.  It looks ok to me.  A few small comments...


> 2005-06-29  Vincent Pelletier  <subdino2004@yahoo.fr>
>
>     * commands/cmp.c
>       (grub_cmd_cmp): Close the right file at the right time.  Compare
>       only data just read.  Don't report files of different size as
>       identical.  (BLOCK_SIZE): New macro.

The (BLOCK_SIZE) stuff should be put on a new line.

I think BUFFER_SIZE would be a better name.  Another size can be
used.  Perhaps it is better to use a bigger buffer size?


>  
>    if (grub_file_size (file1) != grub_file_size (file2))
> -    grub_printf ("Differ in size: %d [%s], %d [%s]\n", 
> -		 grub_file_size (file1), args[0], 
> -		 grub_file_size (file2), args[1]);
> +    {
> +      grub_printf ("Differ in size: %d [%s], %d [%s]\n", 
> +		   grub_file_size (file1), args[0], 
> +		   grub_file_size (file2), args[1]);
> +    }

Huh?

Thanks,
Marco

Re: [Bulk] Re: [PATCH] grub2: commands/cmp.c: grub_cmd_cmp()

[Vincent Pelletier]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 947 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Marco Gerards wrote:
> I think BUFFER_SIZE would be a better name.  Another size can be
> used.  Perhaps it is better to use a bigger buffer size?

As we read from a block device, I think the best size would be a
multiple of the block size... But yes, it could be named buffer size.
And it could even be dynamicaly allocated, to prevent using too much
space on stack.

> Huh?

Some remains from a change-and-undo... I'll send a new patch with a
correct changelog :).

Vincent Pelletier
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFCw8j8FEQoKRQyjtURApAVAKCUiOSXjhRmv5Sy/Hkct/9rqZjapwCeNZws
iYms73xiAlh3TMEgfIoctA4=
=+ko/
-----END PGP SIGNATURE-----

	

	
		
___________________________________________________________________________ 
Appel audio GRATUIT partout dans le monde avec le nouveau Yahoo! Messenger 
Téléchargez cette version sur http://fr.messenger.yahoo.com

Re: [Bulk] Re: [Bulk] Re: [PATCH] grub2: commands/cmp.c: grub_cmd_cmp()

[Vincent Pelletier]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 904 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I haven't changed the BUFFER_SIZE (512). Please do if you think it is
appropriate.

2005-06-29  Vincent Pelletier  <subdino2004@yahoo.fr>

    * commands/cmp.c
      (BUFFER_SIZE): New macro.
      (grub_cmd_cmp): Close the right file at the right time.  Compare
      only data just read.  Don't report files of different size as
      identical.  Dynamically allocate buffers.  Move variable
      declarations at the beginning of function.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFCxmozFEQoKRQyjtURAhgIAJ9lR04VeEhMHnnTe1KJVorjRMzCbwCfYXDT
7qkbXSTMF7bGi8gaxS35/RI=
=N0Ed
-----END PGP SIGNATURE-----

	

	
		
___________________________________________________________________________ 
Appel audio GRATUIT partout dans le monde avec le nouveau Yahoo! Messenger 
Téléchargez cette version sur http://fr.messenger.yahoo.com

[PATCH v2] commands/cmp.c: grub_cmd_cmp()

[Vincent Pelletier]

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

(hum)

Vincent Pelletier
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFCxnFnFEQoKRQyjtURAp6rAJ9bk6yTKM+px9Ikh+4ByzT+Q01c/gCeLVJ1
3YHNI5TDbln9u0Aoz5i9R4M=
=jdzU
-----END PGP SIGNATURE-----

[-- Attachment #2: cmp.c_2.diff --]
[-- Type: text/plain, Size: 2819 bytes --]

Index: cmp.c
===================================================================
RCS file: /cvsroot/grub/grub2/commands/cmp.c,v
retrieving revision 1.2
diff -u -p -r1.2 cmp.c
--- cmp.c	4 Apr 2004 13:46:00 -0000	1.2
+++ cmp.c	2 Jul 2005 10:10:40 -0000
@@ -23,13 +23,21 @@
 #include <grub/arg.h>
 #include <grub/misc.h>
 #include <grub/file.h>
+#include <grub/mm.h>
+
+#define BUFFER_SIZE 512
 
 static grub_err_t
 grub_cmd_cmp (struct grub_arg_list *state __attribute__ ((unused)),
 	      int argc, char **args)
 {
-  grub_file_t file1;
-  grub_file_t file2;
+  grub_err_t err;
+  grub_ssize_t rd1, rd2;
+  grub_uint32_t pos;
+  grub_file_t file1 = 0;
+  grub_file_t file2 = 0;
+  char *buf1 = 0;
+  char *buf2 = 0;
 
   if (argc != 2)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "two arguments required");
@@ -37,16 +45,9 @@ grub_cmd_cmp (struct grub_arg_list *stat
   grub_printf ("Compare `%s' and `%s':\n", args[0],
 	       args[1]);
 
-  file1 = grub_file_open (args[0]);
-  if (! file1)
-    return grub_errno;
-
-  file2 = grub_file_open (args[1]);
-  if (! file2)
-    {
-      grub_file_close (file2);
-      return grub_errno;
-    }
+  if (! (file1 = grub_file_open (args[0]) ) ||
+      ! (file2 = grub_file_open (args[1]) ) )
+    goto cleanup;
 
   if (grub_file_size (file1) != grub_file_size (file2))
     grub_printf ("Differ in size: %d [%s], %d [%s]\n", 
@@ -55,44 +56,48 @@ grub_cmd_cmp (struct grub_arg_list *stat
   
   else
     {
-      char buf1[512];
-      char buf2[512];
-      grub_ssize_t rd1, rd2;
-      grub_uint32_t pos = 0;
-     
+      pos = 0;
+
+      if (! (buf1 = (char *) grub_malloc (BUFFER_SIZE) ) ||
+          ! (buf2 = (char *) grub_malloc (BUFFER_SIZE) ) )
+        goto cleanup;
       do
 	{
 	  int i;
-	  rd1 = grub_file_read (file1, buf1, 512);
-	  rd2 = grub_file_read (file2, buf2, 512);
+	  rd1 = grub_file_read (file1, buf1, BUFFER_SIZE);
+	  rd2 = grub_file_read (file2, buf2, BUFFER_SIZE);
 
 	  if (rd1 != rd2)
-	    return 0;
+	    goto cleanup;
 
-	  for (i = 0; i < 512; i++)
+	  for (i = 0; i < rd2; i++)
 	    {
 	      if (buf1[i] != buf2[i])
 		{
 		  grub_printf ("Differ at the offset %d: 0x%x [%s], 0x%x [%s]\n",
 			       i + pos, buf1[i], args[0],
 			       buf2[i], args[1]);
-
-		  grub_file_close (file1);
-		  grub_file_close (file2);
-		  return 0;
+		  goto cleanup;
 		}
 	    }
-	  pos += 512;
+	  pos += BUFFER_SIZE;
 	  
 	} while (rd2);
+      grub_printf ("The files are identical.\n");
     }
 
-  grub_file_close (file1);
-  grub_file_close (file2);
-
-  grub_printf ("The files are identical.\n");
+cleanup:
+  err=grub_errno;
+  if (buf1)
+    grub_free (buf1);
+  if (buf2)
+    grub_free (buf2);
+  if (file1)
+    grub_file_close (file1);
+  if (file2)
+    grub_file_close (file2);
 
-  return 0;
+  return err;
 }
 
 \f