Re: Two Small Patches (x86 VolId & Sun Label Checking)

[ehem+grub]

>From: Vladimir '?-coder/phcoder' Serbinenko <address@hidden>
> On 12/17/2010 06:19 AM, address@hidden wrote:
> > First, what is refered to as "a magic number used by Windows NT" is
> > apparently used as a volume identifier to identify disks. 

> It's windows NT Volume ID. I don't see how it is a misnomer.

Perhaps not a gigantic one, but I would argue the existing name is a
misnomer. "Magic number" generally refers to the various constants used
to flag the various style of disk header, things like 0x504D for Apple,
"RDSK" for Amiga, the 64-bit constant for GPT, or 0xDABE for Sun.

Meanwhile, as you agreed above, it is a volume identifier that needs to
be preserved; something rather different from a magic number. Also, as
I'd further noted, LILO also uses it to identify disks, so it isn't
confined to Windows NT at this point.


> > stage. Second, perhaps minor, but I'm surprised someone chose to break
> > the checksum verification code in grub-core/partmap/sun.c and didn't
> > didn't keep the magic number check with it.

> This seems to be just moving the code around. There are many different
> coding styles. Changing from one of them to another is waste of effort
> unless it's done to uniformise with rest of codebase. I don't have
> strong preference to one or another style so I prefer not to touch anything.

Leaving it alone for now is fine by me. I just found it rather strange to
see two distinct styles in the exact same bit of code. I'd expect either
all of the verification or none of the verification broken into a
distinct function.

I will in fact be implementing breaking detection functions into distinct
functions uniformly, because there is a deeper issue lurking here. Looks
to be pure luck no one ran into an unpleasant bug lurking in the existing
design.

(I can readily provide an illustration of the lurking landmine, later
though)


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         address@hidden PGP F6B23DE0         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
2477\___\_|_/DC21 03A0 5D61 985B <-PGP-> F2BE 6526 ABD2 F6B2\_|_/___/3DE0