(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".
We have two families of "fw_cfg_add_*" functions, some make deep copies,
some only link (reference) the caller's blob. I think for this use case,
due to the allocation occurring internally (similarly to
fw_cfg_add_i16(), for example), fw_cfg should own the copy.
(I'm assuming that we won't need fw_cfg_modify_file_from_host() for now.)
> Pushing the responsibility to free the file contents on the caller is
> not a nice interface anyway. I feel fw_cfg should take over ownership.
> In the current state of things, that's trivial: document it does, done.
>
> If we ever get around to cleaning up machines and onboard devices
> properly, then fw_cfg.c will have to grow a suitable instance_finalize()
> method, even without your patch. We lack such resource cleanup all over
> the place, but feel free to a TODO comment here anyway.
Regarding "real" ownership (i.e. destroying owned blobs when FwCfg
itself is destroyed): we discussed that in:
[PATCH v2 00/18] fw_cfg: reduce memleaks, add QMP/HMP info +
edk2_add_host_crypto_policy
I think it can be a valuable goal, but it will take a dedicated /
focused series (maybe multiple waves).
> Are there more functions in fw_cfg.h with the same interface issue?
> Besides FWCfgState constructors, the only one returning a pointer is
> fw_cfg_modify_file(). Function comment:
>
> * Replace a NAMED fw_cfg item. If an existing item is found, its callback
> * information will be cleared, and a pointer to its data will be returned
> * to the caller, so that it may be freed if necessary. If an existing item
> * is not found, this call defaults to fw_cfg_add_file(), and NULL is
> * returned to the caller.
>
> Okay, not the same issue, but there's still an issue: this function's
> caller needs to know how the old file contents was added! Remember,
> fw_cfg_add_file() lets you add both malloc'ed and other memory. Needs
> an interface fix or at least a comment pointing out the issue. Separate
> patch, not necessarily in this series.
I think that fw_cfg_modify_file() is fine, for now.
First, it does say "freed if necessary". Second, it is clear that
fw_cfg_modify_file() co-operates with fw_cfg_add_file() -- and
fw_cfg_add_file() never copies, only references, leaving the real
ownership with the caller. Thus, fw_cfg_modify_file() is supposed to be
called only by the one agent that added the file earlier (if the file
exists already), and so they are expected to actually "own" the file (=
remember how to release the old contents).
Obviously if I saw a specific patch for improving the function comment,
I could change my mind; I'm uncertain. I could be biased ATM, due to
reviewing and accepting commits 6cec43e178cde and 9c4a5c55f5c6c earlier.
>
>> + */
>> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
>> + const char *host_path, size_t *len);
>> +
>> /**
>> * fw_cfg_add_file_callback:
>> * @s: fw_cfg device being modified
Thanks
Laszlo
