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

[Laszlo Ersek]

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.
> 
> 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 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()).

FWIW

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo