Re: [PATCH 2.6.7-mm1] MBR centralization

Re: [PATCH 2.6.7-mm1] MBR centralization

[Andrew Morton]

FabF <fabian.frederick@skynet.be> wrote:
>
> +/*Master boot record magic numbers*/
> +#define MSDOS_MBR_SIGNATURE 0xaa55
> +#define MSDOS_MBR(p) le16_to_cpu((u16)*p) == MSDOS_MBR_SIGNATURE

I'd make this

/*
 * Check for MSDOS Master Boot Record signature
 */
static inline int msdos_mbr(u16 v)
{
	return le16_to_cpu(v) == 0xaa55;
}

[PATCH 2.6.7-mm1] MBR centralization

[FabF]

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

Andrew,

	Here's a patch for partition management:

	-Centralize msdos mbr detection.(magic detection was made twice : in
efi and msdos)

		-mbr definition moved from efi to genhd
		-msdos partition code mbr magics removed to use the one above
		-adding genhd explicit macro used by both

	-DOS_EXTENDED_PARTITION washing#2
		(we don't want magic numbers, do we ?)
		-Remove use of <= 4 to < DOS_EXTENDED_PARTITION where needed
	-Trivial doc (so trivial indeed).
	-Some code washing

PS: It runs under !EFI (for the least)

Regards,
FabF

[-- Attachment #2: partitions2.diff --]
[-- Type: text/x-patch, Size: 4423 bytes --]

diff -Naur orig/fs/partitions/efi.c edited/fs/partitions/efi.c
--- orig/fs/partitions/efi.c	2004-06-16 07:19:23.000000000 +0200
+++ edited/fs/partitions/efi.c	2004-06-24 01:43:45.000000000 +0200
@@ -145,7 +145,7 @@
 	int i, found = 0, signature = 0;
 	if (!mbr)
 		return 0;
-	signature = (le16_to_cpu(mbr->signature) == MSDOS_MBR_SIGNATURE);
+	signature = MSDOS_MBR(&mbr->signature);
 	for (i = 0; signature && i < 4; i++) {
 		if (mbr->partition_record[i].sys_ind ==
                     EFI_PMBR_OSTYPE_EFI_GPT) {
diff -Naur orig/fs/partitions/efi.h edited/fs/partitions/efi.h
--- orig/fs/partitions/efi.h	2004-06-16 07:19:43.000000000 +0200
+++ edited/fs/partitions/efi.h	2004-06-24 01:20:46.000000000 +0200
@@ -34,7 +34,6 @@
 #include <linux/string.h>
 #include <linux/efi.h>
 
-#define MSDOS_MBR_SIGNATURE 0xaa55
 #define EFI_PMBR_OSTYPE_EFI 0xEF
 #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE
 
diff -Naur orig/fs/partitions/msdos.c edited/fs/partitions/msdos.c
--- orig/fs/partitions/msdos.c	2004-06-16 07:19:37.000000000 +0200
+++ edited/fs/partitions/msdos.c	2004-06-23 22:36:46.496786675 +0200
@@ -50,15 +50,6 @@
 		SYS_IND(p) == LINUX_EXTENDED_PARTITION);
 }
 
-#define MSDOS_LABEL_MAGIC1	0x55
-#define MSDOS_LABEL_MAGIC2	0xAA
-
-static inline int
-msdos_magic_present(unsigned char *p)
-{
-	return (p[0] == MSDOS_LABEL_MAGIC1 && p[1] == MSDOS_LABEL_MAGIC2);
-}
-
 /*
  * Create devices for each logical partition in an extended partition.
  * The logical partitions form a linked list, with each entry being
@@ -87,6 +78,7 @@
 	this_size = first_size;
 
 	while (1) {
+		/*FIXME : Some definition for 100*/
 		if (++loopct > 100)
 			return;
 		if (state->next == state->limit)
@@ -95,7 +87,7 @@
 		if (!data)
 			return;
 
-		if (!msdos_magic_present(data + 510))
+		if (!MSDOS_MBR (data + 510))
 			goto done; 
 
 		p = (struct partition *) (data + 0x1be);
@@ -112,7 +104,7 @@
 		/* 
 		 * First process the data partition(s)
 		 */
-		for (i=0; i<4; i++, p++) {
+		for (i = 0; i < 4; i++, p++) {
 			u32 offs, size, next;
 			if (!NR_SECTS(p) || is_extended_partition(p))
 				continue;
@@ -146,7 +138,7 @@
 		 * It should be a link to the next logical partition.
 		 */
 		p -= 4;
-		for (i=0; i<4; i++, p++)
+		for (i = 0; i < 4; i++, p++)
 			if (NR_SECTS(p) && is_extended_partition(p))
 				break;
 		if (i == 4)
@@ -342,7 +334,7 @@
 	/* The first sector of a Minix partition can have either
 	 * a secondary MBR describing its subpartitions, or
 	 * the normal boot sector. */
-	if (msdos_magic_present (data + 510) &&
+	if (MSDOS_MBR(data + 510) &&
 	    SYS_IND(p) == MINIX_PARTITION) { /* subpartition table present */
 
 		printk(" %s%d: <minix:", state->name, origin);
@@ -385,7 +377,7 @@
 	data = read_dev_sector(bdev, 0, &sect);
 	if (!data)
 		return -1;
-	if (!msdos_magic_present(data + 510)) {
+	if (!MSDOS_MBR(data + 510)) {
 		put_dev_sector(sect);
 		return 0;
 	}
@@ -397,16 +389,16 @@
 	 * is not 0 or 0x80.
 	 */
 	p = (struct partition *) (data + 0x1be);
-	for (slot = 1; slot <= 4; slot++, p++) {
+	for (slot = 1; slot < DOS_EXTENDED_PARTITION; slot++, p++) {
 		if (p->boot_ind != 0 && p->boot_ind != 0x80) {
 			put_dev_sector(sect);
 			return 0;
 		}
 	}
-
+	/*EFI : Extensible Firmware Initiative partitions ?*/
 #ifdef CONFIG_EFI_PARTITION
 	p = (struct partition *) (data + 0x1be);
-	for (slot = 1 ; slot <= 4 ; slot++, p++) {
+	for (slot = 1 ; slot < DOS_EXTENDED_PARTITION; slot++, p++) {
 		/* If this is an EFI GPT disk, msdos should ignore it. */
 		if (SYS_IND(p) == EFI_PMBR_OSTYPE_EFI_GPT) {
 			put_dev_sector(sect);
@@ -433,6 +425,7 @@
 			   extended partition, but leave room for LILO */
 			put_partition(state, slot, start, size == 1 ? 1 : 2);
 			printk(" <");
+			/*parsing extended for logical partitions*/
 			parse_extended(state, bdev, start, size);
 			printk(" >");
 			continue;
diff -Naur orig/include/linux/genhd.h edited/include/linux/genhd.h
--- orig/include/linux/genhd.h	2004-06-16 07:18:59.000000000 +0200
+++ edited/include/linux/genhd.h	2004-06-24 02:00:09.000000000 +0200
@@ -17,6 +17,10 @@
 #include <linux/string.h>
 #include <linux/fs.h>
 
+/*Master boot record magic numbers*/
+#define MSDOS_MBR_SIGNATURE 0xaa55
+#define MSDOS_MBR(p) le16_to_cpu((u16)*p) == MSDOS_MBR_SIGNATURE
+
 enum {
 /* These three have identical behaviour; use the second one if DOS FDISK gets
    confused about extended/logical partitions starting past cylinder 1023. */

Re: [PATCH 2.6.7-mm1] MBR centralization

[Andries Brouwer]

On Wed, Jun 23, 2004 at 11:15:49PM +0200, FabF wrote:
> 
> 	-DOS_EXTENDED_PARTITION washing#2
> 		(we don't want magic numbers, do we ?)
> 		-Remove use of <= 4 to < DOS_EXTENDED_PARTITION where needed

> -       for (slot = 1; slot <= 4; slot++, p++) {
> +       for (slot = 1; slot < DOS_EXTENDED_PARTITION; slot++, p++) {

Maybe you do not know, but the 5 of DOS_EXTENDED_PARTITION is the
value written in the sys_ind field of a partition.

It is totally unrelated to the 4 that is the upper bound of the
loop over the four primary partitions in a DOS-type partition table.

Re: [PATCH 2.6.7-mm1] MBR centralization

[FabF]

On Wed, 2004-06-23 at 23:06, Andrew Morton wrote:
> FabF <fabian.frederick@skynet.be> wrote:
> >
> > +/*Master boot record magic numbers*/
> > +#define MSDOS_MBR_SIGNATURE 0xaa55
> > +#define MSDOS_MBR(p) le16_to_cpu((u16)*p) == MSDOS_MBR_SIGNATURE
> 
> I'd make this
> 
> /*
>  * Check for MSDOS Master Boot Record signature
>  */
> static inline int msdos_mbr(u16 v)
> {
> 	return le16_to_cpu(v) == 0xaa55;
> }
That means I would have to cast from all msdos_partition calls 
e.g. msdos_mbr((u16)*(data+510)) .Is this the right way ?

Regards,
FabF

Re: [PATCH 2.6.7-mm1] MBR centralization

[FabF]

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

On Wed, 2004-06-23 at 23:36, Andries Brouwer wrote:
> On Wed, Jun 23, 2004 at 11:15:49PM +0200, FabF wrote:
> > 
> > 	-DOS_EXTENDED_PARTITION washing#2
> > 		(we don't want magic numbers, do we ?)
> > 		-Remove use of <= 4 to < DOS_EXTENDED_PARTITION where needed
> 
> > -       for (slot = 1; slot <= 4; slot++, p++) {
> > +       for (slot = 1; slot < DOS_EXTENDED_PARTITION; slot++, p++) {
> 
> Maybe you do not know, but the 5 of DOS_EXTENDED_PARTITION is the
> value written in the sys_ind field of a partition.
> 
> It is totally unrelated to the 4 that is the upper bound of the
> loop over the four primary partitions in a DOS-type partition table.
> 

Here's a new version:
	-macro simplification (We're calling from struct & buffer so static fx
conversion seems troublesome)
	-(p)
	-MBR patch only

Someone could check EFI ?

Regards,
FabF

[-- Attachment #2: partitions4.diff --]
[-- Type: text/x-patch, Size: 3918 bytes --]

diff -Naur orig/fs/partitions/efi.c edited/fs/partitions/efi.c
--- orig/fs/partitions/efi.c	2004-06-16 07:19:23.000000000 +0200
+++ edited/fs/partitions/efi.c	2004-06-24 01:43:45.000000000 +0200
@@ -145,7 +145,7 @@
 	int i, found = 0, signature = 0;
 	if (!mbr)
 		return 0;
-	signature = (le16_to_cpu(mbr->signature) == MSDOS_MBR_SIGNATURE);
+	signature = MSDOS_MBR(&mbr->signature);
 	for (i = 0; signature && i < 4; i++) {
 		if (mbr->partition_record[i].sys_ind ==
                     EFI_PMBR_OSTYPE_EFI_GPT) {
diff -Naur orig/fs/partitions/efi.h edited/fs/partitions/efi.h
--- orig/fs/partitions/efi.h	2004-06-16 07:19:43.000000000 +0200
+++ edited/fs/partitions/efi.h	2004-06-24 01:20:46.000000000 +0200
@@ -34,7 +34,6 @@
 #include <linux/string.h>
 #include <linux/efi.h>
 
-#define MSDOS_MBR_SIGNATURE 0xaa55
 #define EFI_PMBR_OSTYPE_EFI 0xEF
 #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE
 
diff -Naur orig/fs/partitions/msdos.c edited/fs/partitions/msdos.c
--- orig/fs/partitions/msdos.c	2004-06-16 07:19:37.000000000 +0200
+++ edited/fs/partitions/msdos.c	2004-06-24 08:00:05.000000000 +0200
@@ -50,15 +50,6 @@
 		SYS_IND(p) == LINUX_EXTENDED_PARTITION);
 }
 
-#define MSDOS_LABEL_MAGIC1	0x55
-#define MSDOS_LABEL_MAGIC2	0xAA
-
-static inline int
-msdos_magic_present(unsigned char *p)
-{
-	return (p[0] == MSDOS_LABEL_MAGIC1 && p[1] == MSDOS_LABEL_MAGIC2);
-}
-
 /*
  * Create devices for each logical partition in an extended partition.
  * The logical partitions form a linked list, with each entry being
@@ -87,6 +78,7 @@
 	this_size = first_size;
 
 	while (1) {
+		/*FIXME : Some definition for 100*/
 		if (++loopct > 100)
 			return;
 		if (state->next == state->limit)
@@ -95,7 +87,7 @@
 		if (!data)
 			return;
 
-		if (!msdos_magic_present(data + 510))
+		if (!MSDOS_MBR (data+510))
 			goto done; 
 
 		p = (struct partition *) (data + 0x1be);
@@ -112,7 +104,7 @@
 		/* 
 		 * First process the data partition(s)
 		 */
-		for (i=0; i<4; i++, p++) {
+		for (i = 0; i < 4; i++, p++) {
 			u32 offs, size, next;
 			if (!NR_SECTS(p) || is_extended_partition(p))
 				continue;
@@ -146,7 +138,7 @@
 		 * It should be a link to the next logical partition.
 		 */
 		p -= 4;
-		for (i=0; i<4; i++, p++)
+		for (i = 0; i < 4; i++, p++)
 			if (NR_SECTS(p) && is_extended_partition(p))
 				break;
 		if (i == 4)
@@ -342,7 +334,7 @@
 	/* The first sector of a Minix partition can have either
 	 * a secondary MBR describing its subpartitions, or
 	 * the normal boot sector. */
-	if (msdos_magic_present (data + 510) &&
+	if (MSDOS_MBR(data + 510) &&
 	    SYS_IND(p) == MINIX_PARTITION) { /* subpartition table present */
 
 		printk(" %s%d: <minix:", state->name, origin);
@@ -385,7 +377,7 @@
 	data = read_dev_sector(bdev, 0, &sect);
 	if (!data)
 		return -1;
-	if (!msdos_magic_present(data + 510)) {
+	if (!MSDOS_MBR(data + 510)) {
 		put_dev_sector(sect);
 		return 0;
 	}
@@ -403,7 +395,7 @@
 			return 0;
 		}
 	}
-
+	/*EFI : Extensible Firmware Initiative partitions ?*/
 #ifdef CONFIG_EFI_PARTITION
 	p = (struct partition *) (data + 0x1be);
 	for (slot = 1 ; slot <= 4 ; slot++, p++) {
@@ -433,6 +425,7 @@
 			   extended partition, but leave room for LILO */
 			put_partition(state, slot, start, size == 1 ? 1 : 2);
 			printk(" <");
+			/*parsing extended for logical partitions*/
 			parse_extended(state, bdev, start, size);
 			printk(" >");
 			continue;
diff -Naur orig/include/linux/genhd.h edited/include/linux/genhd.h
--- orig/include/linux/genhd.h	2004-06-16 07:18:59.000000000 +0200
+++ edited/include/linux/genhd.h	2004-06-24 08:02:45.151489490 +0200
@@ -17,6 +17,9 @@
 #include <linux/string.h>
 #include <linux/fs.h>
 
+/*Master boot record magic numbers*/
+#define MSDOS_MBR(p) le16_to_cpu((u16)*(p)) == 0xaa55
+
 enum {
 /* These three have identical behaviour; use the second one if DOS FDISK gets
    confused about extended/logical partitions starting past cylinder 1023. */

Re: [PATCH 2.6.7-mm1] MBR centralization

[Andries Brouwer]

On Thu, Jun 24, 2004 at 08:07:57AM +0200, FabF wrote:

> Here's a new version:

> -               if (!msdos_magic_present(data + 510))
> +               if (!MSDOS_MBR (data+510))

Not an improvement.

Re: [PATCH 2.6.7-mm1] MBR centralization

[Andries Brouwer]

On Thu, Jun 24, 2004 at 08:38:52AM +0100, Anton Altaparmakov wrote:

> While I am at it, the above macro is even further optimized by moving the 
> endianness conversion to the constant so it is applied at compile time 
> rather than run time like so:
> 
> #define MSDOS_MBR(p)	((*(u16*)(p)) == __constant_cpu_to_le16(0xaa55))

I never understand why people want to do such things.
Cast a character pointer to u16*, possibly do a byteswap, etc.
What one wants is just

	p[0] == 0x55 && p[1] == 0xaa

Re: [PATCH 2.6.7-mm1] MBR centralization

[FabF]

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

On Thu, 2004-06-24 at 09:38, Anton Altaparmakov wrote:
> On Thu, 24 Jun 2004, Anton Altaparmakov wrote:
> > On Thu, 24 Jun 2004, FabF wrote:
> >> Here's a new version:
> >> 	-macro simplification (We're calling from struct & buffer so static 
> >> fx
> >> conversion seems troublesome)
> >> 	-(p)
> >> 	-MBR patch only
> >
> > Your patch is wrong:
> >
> > diff -Naur orig/include/linux/genhd.h edited/include/linux/genhd.h
> > --- orig/include/linux/genhd.h  2004-06-16 07:18:59.000000000 +0200
> > +++ edited/include/linux/genhd.h        2004-06-24 08:02:45.151489490 +0200
> > @@ -17,6 +17,9 @@
> > #include <linux/string.h>
> > #include <linux/fs.h>
> >
> > +/*Master boot record magic numbers*/
> > +#define MSDOS_MBR(p) le16_to_cpu((u16)*(p)) == 0xaa55
> >
> > This macro is completely broken for all p with a type size other than 16 
> > bits.  E.g. if you have "char *p" then *(p) gives you a single byte which you 
> > then cast to u16 so you will never see the 0xaa55.
> >
> > If you want to do it this way you need to cast p to (u16*) first like so:
> >
> > #define MSDOS_MBR(p)	(le16_to_cpup((u16*)(p)) == 0xaa55)
> >
> > Note using _cpup() means you don't have to do the dereferencing yourself 
> > which looks much cleaner IMO.  You do still need the cast as it is missing 
> > from various macros in the kernel (I consider this a bug but this is 
> > orthogonal to this patch).
> 
> While I am at it, the above macro is even further optimized by moving the 
> endianness conversion to the constant so it is applied at compile time 
> rather than run time like so:
> 
> #define MSDOS_MBR(p)	((*(u16*)(p)) == __constant_cpu_to_le16(0xaa55)
Thanks Anton :) Applied.It worked before but in semi-evaluation.

Regards,
FabF

> 
> Best regards,
> 
>  	Anton

[-- Attachment #2: partitions5.diff --]
[-- Type: text/x-patch, Size: 3937 bytes --]

diff -Naur orig/fs/partitions/efi.c edited/fs/partitions/efi.c
--- orig/fs/partitions/efi.c	2004-06-16 07:19:23.000000000 +0200
+++ edited/fs/partitions/efi.c	2004-06-24 01:43:45.000000000 +0200
@@ -145,7 +145,7 @@
 	int i, found = 0, signature = 0;
 	if (!mbr)
 		return 0;
-	signature = (le16_to_cpu(mbr->signature) == MSDOS_MBR_SIGNATURE);
+	signature = MSDOS_MBR(&mbr->signature);
 	for (i = 0; signature && i < 4; i++) {
 		if (mbr->partition_record[i].sys_ind ==
                     EFI_PMBR_OSTYPE_EFI_GPT) {
diff -Naur orig/fs/partitions/efi.h edited/fs/partitions/efi.h
--- orig/fs/partitions/efi.h	2004-06-16 07:19:43.000000000 +0200
+++ edited/fs/partitions/efi.h	2004-06-24 01:20:46.000000000 +0200
@@ -34,7 +34,6 @@
 #include <linux/string.h>
 #include <linux/efi.h>
 
-#define MSDOS_MBR_SIGNATURE 0xaa55
 #define EFI_PMBR_OSTYPE_EFI 0xEF
 #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE
 
diff -Naur orig/fs/partitions/msdos.c edited/fs/partitions/msdos.c
--- orig/fs/partitions/msdos.c	2004-06-16 07:19:37.000000000 +0200
+++ edited/fs/partitions/msdos.c	2004-06-24 08:00:05.000000000 +0200
@@ -50,15 +50,6 @@
 		SYS_IND(p) == LINUX_EXTENDED_PARTITION);
 }
 
-#define MSDOS_LABEL_MAGIC1	0x55
-#define MSDOS_LABEL_MAGIC2	0xAA
-
-static inline int
-msdos_magic_present(unsigned char *p)
-{
-	return (p[0] == MSDOS_LABEL_MAGIC1 && p[1] == MSDOS_LABEL_MAGIC2);
-}
-
 /*
  * Create devices for each logical partition in an extended partition.
  * The logical partitions form a linked list, with each entry being
@@ -87,6 +78,7 @@
 	this_size = first_size;
 
 	while (1) {
+		/*FIXME : Some definition for 100*/
 		if (++loopct > 100)
 			return;
 		if (state->next == state->limit)
@@ -95,7 +87,7 @@
 		if (!data)
 			return;
 
-		if (!msdos_magic_present(data + 510))
+		if (!MSDOS_MBR (data+510))
 			goto done; 
 
 		p = (struct partition *) (data + 0x1be);
@@ -112,7 +104,7 @@
 		/* 
 		 * First process the data partition(s)
 		 */
-		for (i=0; i<4; i++, p++) {
+		for (i = 0; i < 4; i++, p++) {
 			u32 offs, size, next;
 			if (!NR_SECTS(p) || is_extended_partition(p))
 				continue;
@@ -146,7 +138,7 @@
 		 * It should be a link to the next logical partition.
 		 */
 		p -= 4;
-		for (i=0; i<4; i++, p++)
+		for (i = 0; i < 4; i++, p++)
 			if (NR_SECTS(p) && is_extended_partition(p))
 				break;
 		if (i == 4)
@@ -342,7 +334,7 @@
 	/* The first sector of a Minix partition can have either
 	 * a secondary MBR describing its subpartitions, or
 	 * the normal boot sector. */
-	if (msdos_magic_present (data + 510) &&
+	if (MSDOS_MBR(data + 510) &&
 	    SYS_IND(p) == MINIX_PARTITION) { /* subpartition table present */
 
 		printk(" %s%d: <minix:", state->name, origin);
@@ -385,7 +377,7 @@
 	data = read_dev_sector(bdev, 0, &sect);
 	if (!data)
 		return -1;
-	if (!msdos_magic_present(data + 510)) {
+	if (!MSDOS_MBR(data + 510)) {
 		put_dev_sector(sect);
 		return 0;
 	}
@@ -403,7 +395,7 @@
 			return 0;
 		}
 	}
-
+	/*EFI : Extensible Firmware Initiative partitions ?*/
 #ifdef CONFIG_EFI_PARTITION
 	p = (struct partition *) (data + 0x1be);
 	for (slot = 1 ; slot <= 4 ; slot++, p++) {
@@ -433,6 +425,7 @@
 			   extended partition, but leave room for LILO */
 			put_partition(state, slot, start, size == 1 ? 1 : 2);
 			printk(" <");
+			/*parsing extended for logical partitions*/
 			parse_extended(state, bdev, start, size);
 			printk(" >");
 			continue;
diff -Naur orig/include/linux/genhd.h edited/include/linux/genhd.h
--- orig/include/linux/genhd.h	2004-06-16 07:18:59.000000000 +0200
+++ edited/include/linux/genhd.h	2004-06-24 17:11:58.447759035 +0200
@@ -17,6 +17,9 @@
 #include <linux/string.h>
 #include <linux/fs.h>
 
+/*Master boot record magic numbers*/
+#define MSDOS_MBR(p)    ((*(u16*)(p)) == __constant_cpu_to_le16(0xaa55))
+
 enum {
 /* These three have identical behaviour; use the second one if DOS FDISK gets
    confused about extended/logical partitions starting past cylinder 1023. */

Re: [PATCH 2.6.7-mm1] MBR centralization

[Andries Brouwer]

On Fri, Jun 25, 2004 at 09:30:24AM +0100, Anton Altaparmakov wrote:
> On Thu, 2004-06-24 at 14:06, Andries Brouwer wrote:
> > On Thu, Jun 24, 2004 at 08:38:52AM +0100, Anton Altaparmakov wrote:
> > > While I am at it, the above macro is even further optimized by moving the 
> > > endianness conversion to the constant so it is applied at compile time 
> > > rather than run time like so:
> > > 
> > > #define MSDOS_MBR(p)	((*(u16*)(p)) == __constant_cpu_to_le16(0xaa55))
> > 
> > I never understand why people want to do such things.
> > Cast a character pointer to u16*, possibly do a byteswap, etc.
> > What one wants is just
> > 
> > 	p[0] == 0x55 && p[1] == 0xaa
> 
> I would disagree.  Seeing something like "is_msdos_mbr(p)" it is
> immediately obvious what it means while seeing the "p[0] = 0x55 && p[1]
> = 0xaa" it is not obvious at all what it means.  IMO _any_ hardcoded
> value is a very bad thing

Ha Anton - you cannot read.
I complain about this strange casting, and you start talking about hardcoded
values (while you write 0xaa55 yourself). The current code is below - a good
name and no hardcoded constants and no strange casts.

Andries


#define MSDOS_LABEL_MAGIC1      0x55
#define MSDOS_LABEL_MAGIC2      0xAA

static inline int
msdos_magic_present(unsigned char *p)
{
        return (p[0] == MSDOS_LABEL_MAGIC1 && p[1] == MSDOS_LABEL_MAGIC2);
}