Laszlo Ersek <ler...@redhat.com> writes:
> On 07/20/20 14:35, Philippe Mathieu-Daudé wrote:
>> Commits b6d7e9b66f..a43770df5d simplified the error propagation.
>> Similarly to commit 6fd5bef10b "qom: Make functions taking Error**
>> return bool, not void", let fw_cfg_add_from_generator() return a
>> boolean value, not void.
>> This allow to simplify parse_fw_cfg() and fixes the error handling
>> issue reported by Coverity (CID 1430396):
>>
>> In parse_fw_cfg():
>>
>> Variable assigned once to a constant guards dead code.
>>
>> Local variable local_err is assigned only once, to a constant
>> value, making it effectively constant throughout its scope.
>> If this is not the intent, examine the logic to see if there
>> is a missing assignment that would make local_err not remain
>> constant.
It's the call of fw_cfg_add_from_generator():
Error *local_err = NULL;
fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
if (local_err) {
error_propagate(errp, local_err);
return -1;
}
return 0;
If it fails, parse_fw_cfg() sets an error and returns 0, which is wrong.
Harmless, because the only caller passes &error_fatal.
Please work this impact assessment into the commit message.
>>
>> Reported-by: Peter Maydell <peter.mayd...@linaro.org>
>> Fixes: Coverity CID 1430396: 'Constant' variable guards dead code (DEADCODE)
>> Fixes: 6552d87c48 ("softmmu/vl: Let -fw_cfg option take a 'gen_id' argument")
>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>> ---
>> include/hw/nvram/fw_cfg.h | 4 +++-
>> hw/nvram/fw_cfg.c | 10 ++++++----
>> softmmu/vl.c | 6 +-----
>> 3 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index 11feae3177..d90857f092 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -302,8 +302,10 @@ void *fw_cfg_modify_file(FWCfgState *s, const char
>> *filename, void *data,
>> * 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: %true on success, %false on error.
>> */
>> -void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
>> +bool fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
>> const char *gen_id, Error **errp);
>>
>> FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 3b1811d3bf..c88aec4341 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -1032,7 +1032,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char
>> *filename,
>> return NULL;
>> }
>>
>> -void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
>> +bool fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
>> const char *gen_id, Error **errp)
>> {
>> ERRP_GUARD();
>> @@ -1044,20 +1044,22 @@ void fw_cfg_add_from_generator(FWCfgState *s, const
>> char *filename,
>> obj = object_resolve_path_component(object_get_objects_root(), gen_id);
>> if (!obj) {
>> error_setg(errp, "Cannot find object ID '%s'", gen_id);
>> - return;
>> + return false;
>> }
>> if (!object_dynamic_cast(obj, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
>> error_setg(errp, "Object ID '%s' is not a '%s' subclass",
>> gen_id, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE);
>> - return;
>> + return false;
>> }
>> klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
>> array = klass->get_data(obj, errp);
>> if (*errp) {
>> - return;
>> + return false;
>> }
>> size = array->len;
>> fw_cfg_add_file(s, filename, g_byte_array_free(array, TRUE), size);
>> +
>> + return true;
>> }
>>
>> static void fw_cfg_machine_reset(void *opaque)
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index f476ef89ed..3416241557 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2070,11 +2070,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts,
>> Error **errp)
>> size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
>> buf = g_memdup(str, size);
>> } else if (nonempty_str(gen_id)) {
>> - Error *local_err = NULL;
>> -
>> - fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
>> - if (local_err) {
>> - error_propagate(errp, local_err);
>> + if (!fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp)) {
>> return -1;
>> }
>> return 0;
The minimally invasive fix would be this one-liner:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index f476ef89ed..46718c1eea 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2072,7 +2072,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts,
Error **errp)
} else if (nonempty_str(gen_id)) {
Error *local_err = NULL;
- fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
+ fw_cfg_add_from_generator(fw_cfg, name, gen_id, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return -1;
I like yours better. We have a long tail of functions taking the
conventional Error **errp parameter to convert from returning void to
bool.
> The retvals seem OK, but I have no idea if this plays nicely with the
> new ERRP_GUARD (which I'm just noticing in fw_cfg_add_from_generator()).
Return values don't interfere with ERRP_GUARD().
Below the hood, ERRP_GUARD() replaces problematic values of @errp by a
pointer to a local variable that is automatically propagated to the
original value. Why is that useful? From error.h's big comment:
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
* - It must not be dereferenced, because it may be null.
* - It should not be passed to error_prepend() or
* error_append_hint(), because that doesn't work with &error_fatal.
* ERRP_GUARD() lifts these restrictions.
fw_cfg_add_from_generator() dereferences @errp to check for failure of
klass->get_data().
If ->get_data() returns null on error and non-null on success, we could
simplify:
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 3b1811d3bf..dfa1f2012a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1035,7 +1035,6 @@ void *fw_cfg_modify_file(FWCfgState *s, const char
*filename,
void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
const char *gen_id, Error **errp)
{
- ERRP_GUARD();
FWCfgDataGeneratorClass *klass;
GByteArray *array;
Object *obj;
@@ -1053,7 +1052,7 @@ void fw_cfg_add_from_generator(FWCfgState *s, const
char *filename,
}
klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
array = klass->get_data(obj, errp);
- if (*errp) {
+ if (!array) {
return;
}
size = array->len;
Clearer now?
> FWIW
>
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
>
> Thanks
> Laszlo
Preferably with an improved commit message:
Reviewed-by: Markus Armbruster <arm...@redhat.com>
