Re: [PATCH 3/4] types: Split aligned and packed guids

[Daniel Kiper]

Adding Ard...

On Sat, Oct 07, 2023 at 05:06:46PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> From 8f4a7c92ffe17dd674e80bd3beacb2dac03d135b Mon Sep 17 00:00:00 2001
> From: Vladimir Serbinenko <phcoder@gmail.com>
> Date: Sun, 13 Aug 2023 09:18:23 +0200
> Subject: [PATCH 3/4] types: Split aligned and packed guids
>
> Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
> ---
>  grub-core/commands/probe.c | 12 ++++++------
>  grub-core/kern/misc.c      | 24 ++++++++++++------------
>  include/grub/efi/api.h     |  2 +-
>  include/grub/types.h       |  8 +++++++-
>  4 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/grub-core/commands/probe.c b/grub-core/commands/probe.c
> index be9637f33..193957c39 100644
> --- a/grub-core/commands/probe.c
> +++ b/grub-core/commands/probe.c
> @@ -119,7 +119,7 @@ grub_cmd_probe (grub_extcmd_context_t ctxt, int argc, 
> char **args)
>         if (grub_strcmp(dev->disk->partition->partmap->name, "gpt") == 0)
>           {
>             struct grub_gpt_partentry entry;
> -           grub_guid_t *guid;
> +           grub_guid_t guid;
>
>             if (grub_disk_read(disk, p->offset, p->index, sizeof(entry), 
> &entry))
>               {
> @@ -129,11 +129,11 @@ grub_cmd_probe (grub_extcmd_context_t ctxt, int argc, 
> char **args)
>                 grub_error_pop ();
>                 return grub_errno;
>               }
> -           guid = &entry.guid;
> -           guid->data1 = grub_le_to_cpu32 (guid->data1);
> -           guid->data2 = grub_le_to_cpu16 (guid->data2);
> -           guid->data3 = grub_le_to_cpu16 (guid->data3);
> -           grub_snprintf (val, sizeof(val), "%pG", guid);
> +           guid = entry.guid;
> +           guid.data1 = grub_le_to_cpu32 (guid.data1);
> +           guid.data2 = grub_le_to_cpu16 (guid.data2);
> +           guid.data3 = grub_le_to_cpu16 (guid.data3);
> +           grub_snprintf (val, sizeof(val), "%pG", &guid);
>           }
>         else if (grub_strcmp(dev->disk->partition->partmap->name, "msdos") == 
> 0)
>           {
> diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
> index b57249acb..d4daacf65 100644
> --- a/grub-core/kern/misc.c
> +++ b/grub-core/kern/misc.c
> @@ -1068,22 +1068,22 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, 
> const char *fmt0,
>         if (*(fmt) == 'G')
>           {
>             ++fmt;
> -           grub_guid_t *guid = (grub_guid_t *)(grub_addr_t) curarg;
> -           write_number (str, &count, max_len, 8, 0, '0', 'x', guid->data1);
> +           grub_packed_guid_t *guid = (grub_packed_guid_t *)(grub_addr_t) 
> curarg;
> +           write_number (str, &count, max_len, 8, 0, '0', 'x', 
> guid->inner.data1);
>             write_char (str, &count, max_len, '-');
> -           write_number (str, &count, max_len, 4, 0, '0', 'x', guid->data2);
> +           write_number (str, &count, max_len, 4, 0, '0', 'x', 
> guid->inner.data2);
>             write_char (str, &count, max_len, '-');
> -           write_number (str, &count, max_len, 4, 0, '0', 'x', guid->data3);
> +           write_number (str, &count, max_len, 4, 0, '0', 'x', 
> guid->inner.data3);
>             write_char (str, &count, max_len, '-');
> -           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->data4[0]);
> -           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->data4[1]);
> +           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->inner.data4[0]);
> +           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->inner.data4[1]);
>             write_char (str, &count, max_len, '-');
> -           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->data4[2]);
> -           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->data4[3]);
> -           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->data4[4]);
> -           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->data4[5]);
> -           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->data4[6]);
> -           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->data4[7]);
> +           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->inner.data4[2]);
> +           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->inner.data4[3]);
> +           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->inner.data4[4]);
> +           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->inner.data4[5]);
> +           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->inner.data4[6]);
> +           write_number (str, &count, max_len, 2, 0, '0', 'x', 
> guid->inner.data4[7]);
>             break;
>           }
>         else
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index d3eaef3fb..0ff956adc 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -1388,7 +1388,7 @@ struct grub_efi_configuration_table
>  {
>    grub_guid_t vendor_guid;
>    void *vendor_table;
> -} GRUB_PACKED;
> +};

If it is correct then it requires an explanation in the commit message.
And what about the other types which have grub_guid_t internally?

>  typedef struct grub_efi_configuration_table grub_efi_configuration_table_t;
>
>  #define GRUB_EFIEMU_SYSTEM_TABLE_SIGNATURE 0x5453595320494249LL
> diff --git a/include/grub/types.h b/include/grub/types.h
> index 0d96006fe..ce7fa898f 100644
> --- a/include/grub/types.h
> +++ b/include/grub/types.h
> @@ -376,7 +376,13 @@ struct grub_guid
>    grub_uint16_t data2;
>    grub_uint16_t data3;
>    grub_uint8_t data4[8];
> -} GRUB_PACKED;
> +};

Why not __attribute__ ((__aligned__ (__alignof__ (grub_uint32_t))))? [1] [2]

>  typedef struct grub_guid grub_guid_t;
>
> +struct grub_packed_guid
> +{
> +  struct grub_guid inner;
> +} __attribute__ ((packed));

s/__attribute__ ((packed))/GRUB_PACKED/

Daniel

[1] https://github.com/torvalds/linux/blob/master/include/linux/efi.h#L60-L78
[2] 
https://github.com/torvalds/linux/commit/494c704f9af0a0cddf593b381ea44320888733e6