[PATCH] r1986 broke FAT detection

[PATCH] r1986 broke FAT detection

[Javier Martín]

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

At r1985, "sudo ./grub-probe -t fs -d /dev/fd0" outputs "fat" with a
freshly-formatted VFAT floppy in the drive. At r1986, it spits "error:
unknown filesystem". The cause is this error, repeated three times:

if (! grub_strncmp(something, "FAT12", 5))
  goto fail;

Strncmp does not return a boolean result (i.e. matches or doesn't), but
an _integer_ that is supposed to establish a comparison order between
strings. Thus, a return value of 0 is actually a match. See why I insist
on treating semantic-ints different than semantic-bools even though the
language does not? The correction is obvious (a patch is attached):

if (0 != grub_strncmp(something, "FAT12",5))
  goto fail;

And I remark the 0 != instead of a simple if (strncmp()) test. BTW, I
think the "FATx" constants should be made into macros or SLT... Magic
constants creep me out.


-- Lazy Oblivious, Rational Disaster -- Habbit

[-- Attachment #1.2: damned_int_bool_identity.patch --]
[-- Type: text/x-patch, Size: 898 bytes --]

Index: fs/fat.c
===================================================================
--- fs/fat.c	(revision 1987)
+++ fs/fat.c	(working copy)
@@ -187,9 +187,9 @@
   if (grub_disk_read (disk, 0, 0, sizeof (bpb), (char *) &bpb))
     goto fail;
 
-  if (! grub_strncmp((const char *) bpb.version_specific.fat12_or_fat16.fstype, "FAT12",5)
-      || ! grub_strncmp((const char *) bpb.version_specific.fat12_or_fat16.fstype, "FAT16",5)
-      || ! grub_strncmp((const char *) bpb.version_specific.fat32.fstype, "FAT32",5))
+  if (0 != grub_strncmp((const char *) bpb.version_specific.fat12_or_fat16.fstype, "FAT12", 5)
+      && 0 != grub_strncmp((const char *) bpb.version_specific.fat12_or_fat16.fstype, "FAT16", 5)
+      && 0 != grub_strncmp((const char *) bpb.version_specific.fat32.fstype, "FAT32", 5))
     goto fail;
   
   /* Get the sizes of logical sectors and clusters.  */

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] r1986 broke FAT detection

[Felix Zielcke]

Am Dienstag, den 10.02.2009, 01:19 +0100 schrieb Javier Martín:
> At r1985, "sudo ./grub-probe -t fs -d /dev/fd0" outputs "fat" with a
> freshly-formatted VFAT floppy in the drive. At r1986, it spits "error:
> unknown filesystem". The cause is this error, repeated three times:
> 
> if (! grub_strncmp(something, "FAT12", 5))
>   goto fail;
> 
> Strncmp does not return a boolean result (i.e. matches or doesn't), but
> an _integer_ that is supposed to establish a comparison order between
> strings. Thus, a return value of 0 is actually a match. See why I insist
> on treating semantic-ints different than semantic-bools even though the
> language does not? The correction is obvious (a patch is attached):

Thanks for your patch. Commited.

-- 
Felix Zielcke

Re: [PATCH] r1986 broke FAT detection

[Javier Martín]

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

El mar, 10-02-2009 a las 10:50 +0100, Felix Zielcke escribió:
> Am Dienstag, den 10.02.2009, 01:19 +0100 schrieb Javier Martín:
> > At r1985, "sudo ./grub-probe -t fs -d /dev/fd0" outputs "fat" with a
> > freshly-formatted VFAT floppy in the drive. At r1986, it spits "error:
> > unknown filesystem". The cause is this error, repeated three times:
> > 
> > if (! grub_strncmp(something, "FAT12", 5))
> >   goto fail;
> > 
> > Strncmp does not return a boolean result (i.e. matches or doesn't), but
> > an _integer_ that is supposed to establish a comparison order between
> > strings. Thus, a return value of 0 is actually a match. See why I insist
> > on treating semantic-ints different than semantic-bools even though the
> > language does not? The correction is obvious (a patch is attached):
> 
> Thanks for your patch. Commited.
> 
You're welcome. I see that nevertheless the "0 != " comparisons were
substituted for standard C int-to-bool-conversion-based comparisons.
Maybe people should know the signature _and_ semantic contract of
strncmp, but frequently they don't (I had to look it up in the
handbook), and while the code that was committed may look like an
"obvious error" to a wanderer (because, of course, comparison functions
should return a semantic-bool, shouldn't they?), the version with the
explicit "0 != " checks at least looks like it was written like that _on
purpose_ (and the actual binary cost should be zero with any sensible
compiler), thus making future developers on bug-fixing quests at least
scratch their heads before proposing the change to the "if (!strncmp)"
error. So, keeping the coding style consistent is important, but I think
a balance with readability is in order. Thus, you are the maintainers
and you know what you're doing, but I think it's not worth to keep the
coding style so strict as to become confusing.


-- Lazy Oblivious, Rational Disaster -- Habbit

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] r1986 broke FAT detection

[Robert Millan]

On Tue, Feb 10, 2009 at 12:44:14PM +0100, Javier Martín wrote:
> > 
> You're welcome. I see that nevertheless the "0 != " comparisons were
> substituted for standard C int-to-bool-conversion-based comparisons.
> Maybe people should know the signature _and_ semantic contract of
> strncmp, but frequently they don't (I had to look it up in the
> handbook), and while the code that was committed may look like an
> "obvious error" to a wanderer (because, of course, comparison functions
> should return a semantic-bool, shouldn't they?), the version with the
> explicit "0 != " checks at least looks like it was written like that _on
> purpose_ (and the actual binary cost should be zero with any sensible
> compiler), thus making future developers on bug-fixing quests at least
> scratch their heads before proposing the change to the "if (!strncmp)"
> error. So, keeping the coding style consistent is important, but I think
> a balance with readability is in order. Thus, you are the maintainers
> and you know what you're doing, but I think it's not worth to keep the
> coding style so strict as to become confusing.

I think you're confusing things.  C has no boolean type.  I know strcmp
gives more info than just a semantic boolean, but in this case it's not
interesting to us.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] r1986 broke FAT detection

[Javier Martín]

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

El sáb, 21-02-2009 a las 14:13 +0100, Robert Millan escribió:
> On Tue, Feb 10, 2009 at 12:44:14PM +0100, Javier Martín wrote:
> > > 
> > You're welcome. I see that nevertheless the "0 != " comparisons were
> > substituted for standard C int-to-bool-conversion-based comparisons.
> > Maybe people should know the signature _and_ semantic contract of
> > strncmp, but frequently they don't (I had to look it up in the
> > handbook), and while the code that was committed may look like an
> > "obvious error" to a wanderer (because, of course, comparison functions
> > should return a semantic-bool, shouldn't they?), the version with the
> > explicit "0 != " checks at least looks like it was written like that _on
> > purpose_ (and the actual binary cost should be zero with any sensible
> > compiler), thus making future developers on bug-fixing quests at least
> > scratch their heads before proposing the change to the "if (!strncmp)"
> > error. So, keeping the coding style consistent is important, but I think
> > a balance with readability is in order. Thus, you are the maintainers
> > and you know what you're doing, but I think it's not worth to keep the
> > coding style so strict as to become confusing.
> 
> I think you're confusing things.  C has no boolean type.  I know strcmp
> gives more info than just a semantic boolean, but in this case it's not
> interesting to us.
> 
My point is that the current code _looks_ confusing: due to the lack of
a proper boolean type in C (and no, C99 _Bool does not count either),
functions that return "semantic-booleans" return int's instead, with the
C convention of 0->false, other->true. This is a common convention, and
there are heaps of functions that work like that. So many, in fact, that
those invocations of strncmp are likely to look odd to someone that does
not have the semantic contract (and not just the formal C signature) in
mind; because under the "usual" convention the code _seems_ to be
checking whether the filesystem is fat12, fat16 _and_ fat32 at once.
However, the return value of strncmp is actually a semantic integer, and
thus the Right Thing (TM, and sorry if I sound preaching) is to perform
the very exact comparison we want, that is "strncmp() == 0".

As I already said, the explicit integer comparison would, given its
relative rarity in normal C code, at least make people scratch their
heads before thinking "hey, this comparison should use 'or's instead of
'and's": in fact, I think that's the advantage of my version - it keeps
the original "x or y or z" logical structure of the comparison. Given
that the cost in binary size would most likely be zero for any average
compiler, I think the change is worth performing.

-- 
-- Lazy, Oblivious, Rational Disaster -- Habbit

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]