Re: [PATCH 1/5] hw/smbios: support loading OEM strings values from a file

[Philippe Mathieu-Daudé]

On 9/9/20 9:35 AM, Daniel P. Berrangé wrote:
> On Tue, Sep 08, 2020 at 08:24:35PM +0200, Philippe Mathieu-Daudé wrote:
>> Hi Daniel,
>>
>> On 9/8/20 6:54 PM, Daniel P. Berrangé wrote:
>>> Some applications want to pass quite large values for the OEM strings
>>> entries. Rather than having huge strings on the command line, it would
>>> be better to load them from a file, as supported with -fw_cfg.
>>>
>>> This introduces the "valuefile" parameter allowing for:
>>>
>>>   $ echo -n "thisthing" > mydata.txt
>>>   $ qemu-system-x86_64 \
>>>     -smbios type=11,value=something \
>>>     -smbios type=11,valuefile=mydata.txt \
>>>     -smbios type=11,value=somemore \
>>>     ...other args...
>>>
>>> Now in the guest
>>>
>>> $ dmidecide -t 11
>>> Getting SMBIOS data from sysfs.
>>> SMBIOS 2.8 present.
>>>
>>> Handle 0x0E00, DMI type 11, 5 bytes
>>> OEM Strings
>>>     String 1: something
>>>     String 2: thisthing
>>>     String 3: somemore
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>  hw/smbios/smbios.c | 72 +++++++++++++++++++++++++++++++++++++---------
>>>  1 file changed, 59 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
>>> index 7cc950b41c..8450fad285 100644
>>> --- a/hw/smbios/smbios.c
>>> +++ b/hw/smbios/smbios.c
>>> @@ -110,7 +110,7 @@ static struct {
>>>  
>>>  static struct {
>>>      size_t nvalues;
>>> -    const char **values;
>>> +    char **values;
>>>  } type11;
>>>  
>>>  static struct {
>>> @@ -314,6 +314,11 @@ static const QemuOptDesc qemu_smbios_type11_opts[] = {
>>>          .type = QEMU_OPT_STRING,
>>>          .help = "OEM string data",
>>>      },
>>> +    {
>>> +        .name = "path",
>>> +        .type = QEMU_OPT_STRING,
>>> +        .help = "OEM string data from file",
>>> +    },
>>>  };
>>>  
>>>  static const QemuOptDesc qemu_smbios_type17_opts[] = {
>>> @@ -641,6 +646,8 @@ static void smbios_build_type_11_table(void)
>>>  
>>>      for (i = 0; i < type11.nvalues; i++) {
>>>          SMBIOS_TABLE_SET_STR_LIST(11, type11.values[i]);
>>> +        g_free(type11.values[i]);
>>> +        type11.values[i] = NULL;
>>>      }
>>>  
>>>      SMBIOS_BUILD_TABLE_POST;
>>> @@ -940,9 +947,8 @@ static void save_opt(const char **dest, QemuOpts *opts, 
>>> const char *name)
>>>  
>>>  
>>>  struct opt_list {
>>> -    const char *name;
>>>      size_t *ndest;
>>> -    const char ***dest;
>>> +    char ***dest;
>>>  };
>>>  
>>>  static int save_opt_one(void *opaque,
>>> @@ -951,23 +957,61 @@ static int save_opt_one(void *opaque,
>>>  {
>>>      struct opt_list *opt = opaque;
>>>  
>>> -    if (!g_str_equal(name, opt->name)) {
>>> -        return 0;
>>> +    if (g_str_equal(name, "path")) {
>>> +        g_autoptr(GByteArray) data = g_byte_array_new();
>>> +        g_autofree char *buf = g_new(char, 4096);
>>> +        ssize_t ret;
>>> +        int fd = qemu_open(value, O_RDONLY);
>>
>> While not use g_file_get_contents()?
> 
> qemu_open lets mgmt apps pass in pre-opened FDs using /dev/fdset/NN
> syntax.

OK, I was expecting a specific reason from a GLib advocate like you :)

Maybe add a comment on the 'qemu_open()' line? Else one can be tempted
to convert it to g_file_get_contents().

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>