| Message ID | 20240627170629.2696427-1-vincent.stehle@arm.com |
|---|---|
| State | Accepted |
| Commit | c85a05a5d87ce6f077c41d7e63a4a7953ddb351c |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | bootstd: cros: store partition type in an efi_guid_t | expand |
On Thu, 27 Jun 2024 at 18:06, Vincent Stehlé <vincent.stehle@arm.com> wrote: > > The scan_part() function uses a struct uuid to store the little-endian > partition type GUID, but this structure should be used only to contain a > big-endian UUID. Use an efi_guid_t instead. > > Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > --- > boot/bootmeth_cros.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
Am 27. Juni 2024 19:06:29 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>: >The scan_part() function uses a struct uuid to store the little-endian >partition type GUID, but this structure should be used only to contain a >big-endian UUID. Use an efi_guid_t instead. > >Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com> >Cc: Simon Glass <sjg@chromium.org> >Cc: Tom Rini <trini@konsulko.com> >--- > boot/bootmeth_cros.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/boot/bootmeth_cros.c b/boot/bootmeth_cros.c >index f015f2e1c75..1f83c14aeab 100644 >--- a/boot/bootmeth_cros.c >+++ b/boot/bootmeth_cros.c >@@ -148,7 +148,7 @@ static int scan_part(struct udevice *blk, int partnum, > { > struct blk_desc *desc = dev_get_uclass_plat(blk); > struct vb2_keyblock *hdr; >- struct uuid type; >+ efi_guid_t type; Does Chrome OS only support GPT partitioning? > ulong num_blks; > int ret; > >@@ -161,7 +161,7 @@ static int scan_part(struct udevice *blk, int partnum, > > /* Check for kernel partition type */ > log_debug("part %x: type=%s\n", partnum, info->type_guid); >- if (uuid_str_to_bin(info->type_guid, (u8 *)&type, UUID_STR_FORMAT_GUID)) >+ if (uuid_str_to_bin(info->type_guid, type.b, UUID_STR_FORMAT_GUID)) > return log_msg_ret("typ", -EINVAL); struct disk_partition containing a string which is only needed in the CLI instead of the 16 byte GUID was a bad idea to start with. Shouldn't we replace it or add least add a GUID field instead of first converting to string and than back to GUID? > > if (memcmp(&cros_kern_type, &type, sizeof(type))) You could use the guidcmp() macro here. Best regards Heinrich
Hi, On Thu, 27 Jun 2024 at 20:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 27. Juni 2024 19:06:29 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>: > >The scan_part() function uses a struct uuid to store the little-endian > >partition type GUID, but this structure should be used only to contain a > >big-endian UUID. Use an efi_guid_t instead. > > > >Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com> > >Cc: Simon Glass <sjg@chromium.org> > >Cc: Tom Rini <trini@konsulko.com> > >--- > > boot/bootmeth_cros.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/boot/bootmeth_cros.c b/boot/bootmeth_cros.c > >index f015f2e1c75..1f83c14aeab 100644 > >--- a/boot/bootmeth_cros.c > >+++ b/boot/bootmeth_cros.c > >@@ -148,7 +148,7 @@ static int scan_part(struct udevice *blk, int partnum, > > { > > struct blk_desc *desc = dev_get_uclass_plat(blk); > > struct vb2_keyblock *hdr; > >- struct uuid type; > >+ efi_guid_t type; > > Does Chrome OS only support GPT partitioning? Indeed. > > > ulong num_blks; > > int ret; > > > >@@ -161,7 +161,7 @@ static int scan_part(struct udevice *blk, int partnum, > > > > /* Check for kernel partition type */ > > log_debug("part %x: type=%s\n", partnum, info->type_guid); > >- if (uuid_str_to_bin(info->type_guid, (u8 *)&type, UUID_STR_FORMAT_GUID)) > >+ if (uuid_str_to_bin(info->type_guid, type.b, UUID_STR_FORMAT_GUID)) > > return log_msg_ret("typ", -EINVAL); > > struct disk_partition containing a string which is only needed in the CLI instead of the 16 byte GUID was a bad idea to start with. Shouldn't we replace it or add least add a GUID field instead of first converting to string and than back to GUID? Yes I agree, it would be nice to fix that. > > > > > if (memcmp(&cros_kern_type, &type, sizeof(type))) > > You could use the guidcmp() macro here. Regards, Simon
On Thu, Jun 27, 2024 at 09:28:04PM +0200, Heinrich Schuchardt wrote: > Hi Heinrich, Thanks for your review. My comments below. Best regards, Vincent. > > Am 27. Juni 2024 19:06:29 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>: > >The scan_part() function uses a struct uuid to store the little-endian > >partition type GUID, but this structure should be used only to contain a > >big-endian UUID. Use an efi_guid_t instead. > > > >Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com> > >Cc: Simon Glass <sjg@chromium.org> > >Cc: Tom Rini <trini@konsulko.com> > >--- > > boot/bootmeth_cros.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/boot/bootmeth_cros.c b/boot/bootmeth_cros.c > >index f015f2e1c75..1f83c14aeab 100644 > >--- a/boot/bootmeth_cros.c > >+++ b/boot/bootmeth_cros.c > >@@ -148,7 +148,7 @@ static int scan_part(struct udevice *blk, int partnum, > > { > > struct blk_desc *desc = dev_get_uclass_plat(blk); > > struct vb2_keyblock *hdr; > >- struct uuid type; > >+ efi_guid_t type; > > Does Chrome OS only support GPT partitioning? > > > ulong num_blks; > > int ret; > > > >@@ -161,7 +161,7 @@ static int scan_part(struct udevice *blk, int partnum, > > > > /* Check for kernel partition type */ > > log_debug("part %x: type=%s\n", partnum, info->type_guid); > >- if (uuid_str_to_bin(info->type_guid, (u8 *)&type, UUID_STR_FORMAT_GUID)) > >+ if (uuid_str_to_bin(info->type_guid, type.b, UUID_STR_FORMAT_GUID)) > > return log_msg_ret("typ", -EINVAL); > > struct disk_partition containing a string which is only needed in the CLI instead of the 16 byte GUID was a bad idea to start with. Shouldn't we replace it or add least add a GUID field instead of first converting to string and than back to GUID? I had a quick look and it seems that converting all those UUIDs from strings to binary would indeed impact many places; let's separate this longer-term effort from this change if you agree. > > > > > if (memcmp(&cros_kern_type, &type, sizeof(type))) > > You could use the guidcmp() macro here. Thanks for the tip; I will send a v2 series. > > Best regards > > Heinrich >
diff --git a/boot/bootmeth_cros.c b/boot/bootmeth_cros.c index f015f2e1c75..1f83c14aeab 100644 --- a/boot/bootmeth_cros.c +++ b/boot/bootmeth_cros.c @@ -148,7 +148,7 @@ static int scan_part(struct udevice *blk, int partnum, { struct blk_desc *desc = dev_get_uclass_plat(blk); struct vb2_keyblock *hdr; - struct uuid type; + efi_guid_t type; ulong num_blks; int ret; @@ -161,7 +161,7 @@ static int scan_part(struct udevice *blk, int partnum, /* Check for kernel partition type */ log_debug("part %x: type=%s\n", partnum, info->type_guid); - if (uuid_str_to_bin(info->type_guid, (u8 *)&type, UUID_STR_FORMAT_GUID)) + if (uuid_str_to_bin(info->type_guid, type.b, UUID_STR_FORMAT_GUID)) return log_msg_ret("typ", -EINVAL); if (memcmp(&cros_kern_type, &type, sizeof(type)))
The scan_part() function uses a struct uuid to store the little-endian partition type GUID, but this structure should be used only to contain a big-endian UUID. Use an efi_guid_t instead. Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com> Cc: Simon Glass <sjg@chromium.org> Cc: Tom Rini <trini@konsulko.com> --- boot/bootmeth_cros.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)