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

[Laszlo Ersek <lersek@redhat.com>]

On 12/07/18 18:04, Philippe Mathieu-Daudé wrote:
> 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é <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c         | 22 ++++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h | 22 ++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 50525ec1dd..f3232fcb16 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -842,6 +842,28 @@ 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 *ptr = NULL;
> +    gsize contents_len = 0;
> +
> +
> +    if (g_file_get_contents(host_path, &ptr, &contents_len, &gerr)) {
> +        fw_cfg_add_file(s, filename, ptr, contents_len);

Can you rename "ptr" to "data" or "contents"?

> +    } else {
> +        error_report("failed to read file %s, %s", host_path, gerr->message);

OK... most functions in this file that populate fw_cfg and can fail
(from external data) still use error_report() + a NULL retval, so this
looks consistent.

> +        g_error_free(gerr);
> +        return NULL;
> +    }
> +    if (len) {
> +        *len = contents_len;
> +    }
> +
> +    return ptr;
> +}
> +
>  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 28630b2f26..8de8d63433 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -166,6 +166,28 @@ 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 contains the

s/will contains/will contain/

> + * total length read from the host file. The data referenced by the starting
> + * pointer is only linked, NOT copied, into the data structure of the fw_cfg
> + * device.

Please drop the last sentence; it does not apply here. There is no
starting pointer passed to this function.

... Should we perhaps replace the last sentence by stating that the data
read from the host filesystem is owned by the new fw_cfg entry?

> + * 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 file content, or NULL if an error occured.
> + */
> +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
> 

Hmmmm. If you do return the pointer to the data just read, directly
exposing the allocation address to the caller, then we could in theory
make this new interface consistent with the rest, and make the *caller*
own the data. Then the currently proposed documentation would be sort-of
accurate (the new fw_cfg entry would not own the data just read -- the
caller would). It's hard to divine what's the best approach here,
without seeing any call sites.

... Purely from a consistency POV, I think I'm slightly attracted to
make the fw_cfg reference again a non-owning one, and force the caller
of fw_cfg_add_file_from_host() to own the data read.

Thanks
Laszlo