grub2: commands/cmp.c: grub_cmd_cmp()
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_tgrub_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_tgrub_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
Signature · 30 lines
-----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-----
Quoted 5 lines · Rodrigo Steinmüller Wanderley
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
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 3 lines · Rodrigo Steinmüller Wanderley
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 <[email protected]>
* 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-----
Index: cmp.c
===================================================================
RCS file: /cvsroot/grub/grub2/commands/cmp.c,v
retrieving revision 1.2
cmp.c +6 -8
@@ -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 *statgrub_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 1/1 grub2: commands/cmp.c: grub_cmd_cmp()
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 2 lines · Vincent Pelletier
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 <[email protected]>
* 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-----
Index: commands/cmp.c
===================================================================
RCS file: /cvsroot/grub/grub2/commands/cmp.c,v
retrieving revision 1.2
cmp.c +18 -17
@@ -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 ; }
Vincent Pelletier <[email protected]> writes:
Hi Vincent,
Thanks for your patch. It looks ok to me. A few small comments...
Quoted 6 lines
2005-06-29 Vincent Pelletier <[email protected]>
*
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?
cmp.c grub_cmd_cmp (struct grub_arg_list *stat · line 58
Quoted 10 lines
if (grub_file_size (file1) != grub_file_size (file2))cmp.c +5
-3-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
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 2 lines · Marco Gerards
I think
BUFFER_SIZEwould 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.
Quoted 1 line
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
-----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 <[email protected]>
* 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 1/1 commands/cmp.c: grub_cmd_cmp()
-----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-----
Index: cmp.c
===================================================================
RCS file: /cvsroot/grub/grub2/commands/cmp.c,v
retrieving revision 1.2
cmp.c +36 -31
@@ -23,13 +23,21 @@#include <grub/arg.h> #include <grub/misc.h> #include <grub/file.h> +#include <grub/mm.h> + +#define BUFFER_SIZE 512static 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 *statgrub_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 *statelse { -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; }
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
Signature · 30 lines
-----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-----