(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