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
FabF <[email protected]> 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;
}
On Wed, 2004-06-23 at 23:06, Andrew Morton wrote:
> FabF <[email protected]> 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
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.
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
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.
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
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
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);
}