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

[Laszlo Ersek]

On 07/21/20 10:33, Markus Armbruster wrote:
> 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.

Hmmm... OK. So I guess the problem was that some functions didn't
introduce their own local_err variables (which is always safe to use),
and consequently didn't use explicit propagation to the errp that they
received form their callers. Instead, they just passed on the errp they
received to their callees. And ERRP_GUARD was deemed a better / safer
design than manually converting all such functions to local_err usage /
propagation.

If a function receives an errp, is the function now *required* to use
ERRP_GUARD? Even if the function uses local_err + explicit propagation?

(I think error_prepend() and error_append_hint() should work with
local_err, no?)

Anyway... commit 8b4b52759a7c ("fw_cfg: Use ERRP_GUARD()", 2020-07-10)
indicates that ERRP_GUARD() is not preferred over local_err + manual
propagation. OK.

Side question: how do we intend to catch reintroductions of local_err +
manual propagation?

> 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?

Yes, thanks!

Laszlo