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

[Laszlo Ersek]

(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".

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