MBR centralization
PATCH 1/1 MBR centralization
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
fs/partitions/efi.c +1 -1
@@ -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 ) {
fs/partitions/efi.h +0 -1
@@ -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
fs/partitions/msdos.c +10 -17
@@ -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 , §);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 ;
include/linux/genhd.h +4 -0
@@ -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. */
include/linux/genhd.h line 22
Quoted 4 lines · FabF
include/linux/genhd.h +3
-0+/*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;
}Quoted 8 lines · Andrew Morton
include/linux/genhd.h line 22
Quoted 4 lines · FabF
include/linux/genhd.h +3
-0+/*Master boot record magic numbers*/ +#define MSDOS_MBR_SIGNATURE 0xaa55 +#define MSDOS_MBR (p) le16_to_cpu((u16)*p) == MSDOS_MBR_SIGNATUREI'd make this
Quoted 7 lines
/*
* Check for MSDOS Master Boot Record signature
*/
static inline intmsdos_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
fs/partitions/msdos.c line 392
Quoted 7 lines · FabF
-DOS_EXTENDED_PARTITION washing#2
(we don't want magic numbers, do we ?)
-Remove use of <= 4 to <DOS_EXTENDED_PARTITIONwhere neededfs/partitions/msdos.c +1
-1-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.
Quoted 6 lines · Andries Brouwer
Quoted 4 lines · FabF
-DOS_EXTENDED_PARTITION washing#2
(we don't want magic numbers, do we ?)
-Remove use of <= 4 to <DOS_EXTENDED_PARTITIONwhere needed
Quoted 9 lines
Review
Quoted 2 lines
(quoted diff) +1
-1- 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_PARTITIONis the value written in thesys_indfield 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
fs/partitions/efi.c +1 -1
@@ -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 ) {
fs/partitions/efi.h +0 -1
@@ -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
fs/partitions/msdos.c +8 -15
@@ -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 , §);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 ;
include/linux/genhd.h +3 -0
@@ -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. */
fs/partitions/msdos.c line 90
Quoted 4 lines · FabF
Here's a new version:
fs/partitions/msdos.c +1
-1-if (!msdos_magic_present (data +510 )) +if (!MSDOS_MBR (data+510 ))
Not an improvement.
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 5 lines · Anton Altaparmakov
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 12 lines · Anton Altaparmakov
Quoted 11 lines · Andries Brouwer
Quoted 5 lines · Anton Altaparmakov
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
Quoted 6 lines
Quoted 1 line
p[0] == 0x55 && p[1] == 0xaaI 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 intmsdos_magic_present(unsigned char *p)
{
return (p[0] == MSDOS_LABEL_MAGIC1 && p[1] == MSDOS_LABEL_MAGIC2);
}Quoted 38 lines · Anton Altaparmakov
Quoted 24 lines · Anton Altaparmakov
Quoted 6 lines · FabF
Here's a new version:
-macro simplification (We're calling from struct & buffer so static
fx
conversion seems troublesome)-(p) -MBR patch onlyYour patch is wrong:
diff -Naur
orig/include/linux/genhd.hedited/include/linux/genhd.hinclude/linux/genhd.h 2004-06-24 08:02:45.151489490 +0200 renamed from include/linux/genhd.h 2004-06-16 07:18:59.000000000 +0200 +2
-0@@ -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)) == 0xaa55This 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.Quoted 8 lines
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:
Quoted 1 line
#define
MSDOS_MBR(p)((*(u16*)(p)) ==__constant_cpu_to_le16(0xaa55)
Thanks Anton :) Applied.It worked before but in semi-evaluation.
Regards,
FabF
Quoted 4 lines
Best regards,
Anton
fs/partitions/efi.c +1 -1
@@ -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 ) {
fs/partitions/efi.h +0 -1
@@ -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
fs/partitions/msdos.c +8 -15
@@ -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 , §);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 ;
include/linux/genhd.h +3 -0
@@ -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. */