[PATCH v2 2/7] disk: part_dos: Align dos_partition_t with struct partition

[Javier Martinez Canillas]

Ilias Apalodimas <ilias.apalodimas at linaro.org> writes:

Hello Ilias,

> Hi Javier
>
> [...]
>
>>         unsigned char end_head;         /* end head                             */
>>         unsigned char end_sector;       /* end sector                           */
>>         unsigned char end_cyl;          /* end cylinder                         */
>> -       unsigned char start4[4];        /* starting sector counting from 0      */
>> -       unsigned char size4[4];         /* nr of sectors in partition           */
>> -} dos_partition_t;
>> +       __le32 start_sect;              /* starting sector counting from 0      */
>> +       __le32 nr_sects;                /* nr of sectors in partition           */
>> +} __packed dos_partition_t;
>
> This always has to be packed and the part_dos.h definition had this
> wrong all along?
>

I wondered exactly the same when working on these patches and did a bit
of code archaeology, so I'm glad that you asked!

It wasn't wrong because it was using unsigned char arrays for the start
and number of sectors struct fields, instead of __le32 types.

Given the alignment of a char array is 1-byte (and all the other fields
are unsigned chars), the structure was naturally aligned.

But I agree that using __le32 and __packed is preferred, and in fact is
how Linux defines this data structure in include/linux/msdos_partition.h:

struct msdos_partition {
        u8 boot_ind;            /* 0x80 - active */
        u8 head;                /* starting head */
        u8 sector;              /* starting sector */
        u8 cyl;                 /* starting cylinder */
        u8 sys_ind;             /* What partition type */
        u8 end_head;            /* end head */
        u8 end_sector;          /* end sector */
        u8 end_cyl;             /* end cylinder */
        __le32 start_sect;      /* starting sector counting from 0 */
        __le32 nr_sects;        /* nr of sectors in partition */
} __packed;

The struct_partition in disk/part_efi.c is said to be taken from Linux's
linux/include/genhd.h, which was the old location before commit 1442f76d4317b
("1442f76d4317 block: move struct partition out of genhd.h").

My guess is that the old data structure defined in part_dos.h was copied from
Linux, and that at the time of the copy it was using unsigned char arrays too.

But the struct definition has been like this since the original Linux git
import, so I couldn't check if my guess was correct.

In any case, now this single data structure definition is aligned with how
both  the Linux kernel and edk2 define it.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat