qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH-for-5.1] hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() ret


From: Markus Armbruster
Subject: Re: [PATCH-for-5.1] hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value
Date: Tue, 21 Jul 2020 10:33:16 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux)

Laszlo Ersek <lersek@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.maydell@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é <philmd@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 <lersek@redhat.com>
>
> Thanks
> Laszlo

Preferably with an improved commit message:
Reviewed-by: Markus Armbruster <armbru@redhat.com>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]