Re: [Qemu-devel] [PATCH v3 1/4] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()

[Laszlo Ersek]

On 03/13/19 10:29, Laszlo Ersek wrote:
> (it's easiest if I follow up under Markus's review)
> 
> On 03/11/19 08:15, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <address@hidden> writes:
>>
>>> Add a function to read the full content of file on the host, and add
>>> a new 'file' name item to the fw_cfg device.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>> ---
>>> v2: s/ptr/data, corrected documentation (Laszlo)
>>> v3: inverted the if() logic
>>> ---
>>>  hw/nvram/fw_cfg.c         | 21 +++++++++++++++++++++
>>>  include/hw/nvram/fw_cfg.h | 23 +++++++++++++++++++++++
>>>  2 files changed, 44 insertions(+)
>>>
>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>> index 7fdf04adc9..2a345bfd5c 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -826,6 +826,27 @@ void fw_cfg_add_file(FWCfgState *s,  const char 
>>> *filename,
>>>      fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, 
>>> true);
>>>  }
>>>  
>>> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
>>> +                                const char *host_path, size_t *len)
>>> +{
>>> +    GError *gerr = NULL;
>>> +    gchar *data = NULL;
>>> +    gsize contents_len = 0;
>>> +
>>> +    if (!g_file_get_contents(host_path, &data, &contents_len, &gerr)) {
>>> +        error_report("%s", gerr->message);
>>> +        g_error_free(gerr);
>>> +        return NULL;
>>> +    }
>>> +    fw_cfg_add_file(s, filename, data, contents_len);
>>> +
>>> +    if (len) {
>>> +        *len = contents_len;
>>> +    }
>>> +
>>> +    return data;
>>> +}
>>> +
>>>  void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>>>                          void *data, size_t len)
>>>  {
>>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>>> index f5a6895a74..7c4dbe2a2a 100644
>>> --- a/include/hw/nvram/fw_cfg.h
>>> +++ b/include/hw/nvram/fw_cfg.h
>>> @@ -166,6 +166,29 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, 
>>> uint64_t value);
>>>  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
>>>                       size_t len);
>>>  
>>> +/**
>>> + * fw_cfg_add_file_from_host:
>>> + * @s: fw_cfg device being modified
>>> + * @filename: name of new fw_cfg file item
>>> + * @host_path: path of the host file to read the data from
>>> + * @len: pointer to hold the length of the host file (optional)
>>> + *
>>> + * Read the content of a host file as a raw "blob" then add a new NAMED
>>> + * fw_cfg item of the file size. If @len is provided, it will contain the
>>> + * total length read from the host file. The data read from the host
>>> + * filesystem is owned by the new fw_cfg entry, and is stored into the data
>>> + * structure of the fw_cfg device.
>>> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
>>> + * will be used; also, a new entry will be added to the file directory
>>> + * structure residing at key value FW_CFG_FILE_DIR, containing the item 
>>> name,
>>> + * data size, and assigned selector key value.
>>> + *
>>> + * Returns: pointer to the newly allocated file content, or NULL if an 
>>> error
>>> + * occured. The returned pointer must be freed with g_free().
>>
>> In theory.  In practice, your callers (in PATCH 2) ignore the value.
>>
>> The file contents needs to live as long as FWCfgState (something the
>> function comments in this file neglect to spell out; doc fix patch would
>> be nice, not necessarily in this series).  An FWCfgState generally
>> belongs to a machine (see PATCH 3+4).
>>
>> It looks like we destroy neither the machine (in a quick test,
>> machine_finalize() never gets called), nor its fw_cfg device (if I add
>> an instance_finalize() method to TYPE_FW_CFG_IO, it doesn't get called),
>> so both machine and its FWCfgState live forever.  There's no point in
>> time where the caller can obey the function comment's demand to free
>> without leaving dangling pointers in FWCfgState.
> 
> Basically the only comment I personally have is "please replace the
> (void*) return type with (void), and take ownership of the dynamically
> allocated file content like Markus suggests".
> 
> [...]

Wait a second, I completely missed error conditions here. Can we output
an Error* like we usually do? It would be a first, for fw_cfg
interfaces, but I think it would be better than a naked error_report(),
plus a NULL retval.

I'll defer to Markus though.

Thanks
Laszlo