On Thu, Aug 08, 2024 at 05:37:46PM +0200, Tobias Heider wrote:
> Device tree properties are not explicitly typed but values can take
> multiple forms from strings to arrays and byte-strings.
> grub_fdt_prop_to_string() adds a heuristic to determine the type and
> convert it to a string for printing.
>
> Signed-off-by: Tobias Heider <tobias.hei...@canonical.com>
> ---
> grub-core/lib/fdt.c | 87 ++++++++++++++++++++++++++++++++++++++
> grub-core/loader/efi/fdt.c | 15 +++++--
> include/grub/fdt.h | 3 ++
> 3 files changed, 101 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/lib/fdt.c b/grub-core/lib/fdt.c
> index 73cfa94a2..5ec32ac56 100644
> --- a/grub-core/lib/fdt.c
> +++ b/grub-core/lib/fdt.c
> @@ -1,6 +1,7 @@
> /*
> * GRUB -- GRand Unified Bootloader
> * Copyright (C) 2013 Free Software Foundation, Inc.
> + * Copyright (C) 2024 Canonical, Ltd.
> *
> * GRUB is free software: you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -501,6 +502,92 @@ int grub_fdt_set_prop (void *fdt, unsigned int
> nodeoffset, const char *name,
> return 0;
> }
>
> +static int
s/int/bool/
> +prop_isstring (const unsigned char *str, grub_uint32_t len)
I think you can drop unsigned here.
> +{
> + grub_uint32_t i;
> + int is_nonzero = 0;
Please use bool instead of int.
> + if (!len || str[len - 1])
If you are doing "str[len - 1]" then I think you should put a comment
before function saying that the caller should check len.
> + return 0;
> +
> + for (i = 0; i < len; i++)
> + {
> + if (!str[i])
> + {
> + if (!is_nonzero)
> + return 0;
> +
> + is_nonzero = 0;
> + continue;
> + }
> + if (!grub_isprint (str[i]))
> + return 0;
> +
> + is_nonzero = 1;
> + }
> + return 1;
> +}
> +
> +char *
> +grub_fdt_prop_to_string (const unsigned char *val, grub_uint32_t len)
Again, I think you can drop unsigned here.
> +{
> + char *str;
> + int pos, slen;
> + grub_uint32_t i;
> +
> + if (prop_isstring (val, len))
> + {
> + /* string */
> + return grub_strdup ((const char *)val);
> + }
> + else if (len & 0x3)
Could you define 0x3 as a constant? Or at least add a comment what it means.
> + {
> + /* byte string */
> + grub_printf ("[");
This grub_printf() looks strange. What it does...
> + slen = (len * 3) + 2;
Does len come from trusted source? If yes it requires a comment here.
And I think slen should be defined as grub_size_t.
> + str = grub_malloc (slen);
> + if (str == NULL)
> + return NULL;
> +
> + pos = 0;
> + str[pos++] = '[';
> + for (i = 0; i < len; i++)
> + {
> + pos += grub_snprintf (&str[pos], slen - pos, "%02x", val[i]);
> + if (i != len - 1)
> + str[pos++] = ' ';
> + }
> + str[pos++] = ']';
> + str[pos] = '\0';
> + }
> + else
> + {
> + /* cell list */
> + const grub_uint32_t *cell = (const grub_uint32_t *)val;
Please add space after ")".
> + slen = (len * 11) + 2;
Where do these numbers come from? Please add a comment here and/or
define constants. Similarly it should be explained above.
> + str = grub_malloc (slen);
> + if (str == NULL)
> + return NULL;
> +
> + pos = 0;
> + str[pos++] = '<';
> + for (i = 0, len /= 4; i < len; i++)
> + {
> + pos += grub_snprintf (&str[pos], slen - pos, "0x%08x",
> + grub_be_to_cpu32 (cell[i]));
> + if (i != len - 1)
> + str[pos++] = ' ';
> + }
> + str[pos++] = '>';
> + str[pos] = '\0';
> + }
> +
> + return str;
> +}
> +
> int
> grub_fdt_create_empty_tree (void *fdt, unsigned int size)
> {
> diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> index e510b3491..b43ff730d 100644
> --- a/grub-core/loader/efi/fdt.c
> +++ b/grub-core/loader/efi/fdt.c
> @@ -183,8 +183,10 @@ grub_cmd_fdtdump (grub_extcmd_context_t ctxt,
> char **argv __attribute__ ((unused)))
> {
> struct grub_arg_list *state = ctxt->state;
> - const char *value = NULL;
> + const unsigned char *value = NULL;
Why do you add unsigned here? If it is really needed it should be
explained in the commit message.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
