Re: [PATCH for-7.2 v2 16/20] device_tree.c: support string props in fdt_format_node()

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 8/8/22 01:36, David Gibson wrote:
> On Fri, Aug 05, 2022 at 06:39:44AM -0300, Daniel Henrique Barboza wrote:
>> To support printing string properties in 'info fdt' we need to determine
>> whether a void data might contain a string.
> 
> Oh... sorry, obviously I hadn't read these later patches when I
> complained about the command not printing property values.
> 
>>
>> We do that by casting the void data to a string array and:
>>
>> - check if the array finishes with a null character
>> - check if all characters are printable
> 
> This won't handle the case of the "string list" several strings tacked
> together, separated by their terminating \0 characters.

Hmmmm how is this printed? Should we concatenate them? Replace the \0
with a whitespace? Or ignore the zero and concatenate them?

E.g. this is a\0string\0list

Should we print it as:

this is astringlist

or

this is a string list ?


Thanks,


Daniel


> 
>>
>> If both conditions are met, we'll consider it to be a string data type
>> and print it accordingly. After this change, 'info fdt' is now able to
>> print string properties. Here's an example with the ARM 'virt' machine:
>>
>> (qemu) info fdt /chosen
>> chosen {
>>      stdout-path = '/pl011@9000000'
>>      rng-seed;
>>      kaslr-seed;
>> }
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   softmmu/device_tree.c | 25 ++++++++++++++++++++++++-
>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
>> index 3fb07b537f..8691c3ccc0 100644
>> --- a/softmmu/device_tree.c
>> +++ b/softmmu/device_tree.c
>> @@ -663,6 +663,24 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp)
>>       error_setg(errp, "Error when saving machine FDT to file %s", filename);
>>   }
>>   
>> +static bool fdt_prop_is_string(const void *data, int size)
>> +{
>> +    const char *str = data;
>> +    int i;
>> +
>> +    if (size <= 0 || str[size - 1] != '\0') {
>> +        return false;
>> +    }
>> +
>> +    for (i = 0; i < size - 1; i++) {
>> +        if (!g_ascii_isprint(str[i])) {
>> +            return false;
>> +        }
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   static void fdt_format_node(GString *buf, int node, int depth)
>>   {
>>       const struct fdt_property *prop = NULL;
>> @@ -681,7 +699,12 @@ static void fdt_format_node(GString *buf, int node, int depth)
>>           prop = fdt_get_property_by_offset(fdt, property, &prop_size);
>>           propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>>   
>> -        g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
>> +        if (fdt_prop_is_string(prop->data, prop_size)) {
>> +            g_string_append_printf(buf, "%*s%s = '%s'\n",
>> +                                   padding, "", propname, prop->data);
> 
> If you're going for dts like output, I'd suggest going all the way.
> That means \" instead of \' and a ';' at the end of the line.
> 
>> +        } else {
>> +            g_string_append_printf(buf, "%*s%s;\n", padding, "", propname);
>> +        }
>>       }
>>   
>>       padding -= 4;
>