| Message ID | 20190310004749.27029-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fw_cfg: Add edk2_add_host_crypto_policy() | expand |
Philippe Mathieu-Daudé <philmd@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é <philmd@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. 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. 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. > + */ > +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
(it's easiest if I follow up under Markus's review) On 03/11/19 08:15, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@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é <philmd@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
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é <philmd@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é <philmd@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
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(). + */ +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
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> --- 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(+)