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é <phi...@redhat.com> 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é <phi...@redhat.com>
>>> ---
>>> 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
