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

[Markus Armbruster]

Cc: Vladimir

Laszlo Ersek <lersek@redhat.com> writes:

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

Not quite :)

Passing @errp directly to avoid error_propagate() is fine.  In fact,
I've grown to dislike use of error_propagate() for several reasons:

1. It's t-e-d-i-o-u-s-l-y verbose.  All that error handling boilerplate
makes it hard to see what the code is trying to get done.

2. It gives me crappy stack backtraces: the backtrace for
error_propagate(&error_abort, local_err) is better than nothing, but the
one I really want is for the error_setg().

3. It annoys me in the debugger by defeating things like break
error_setg_internal if errp == ...

I'm on a quest to kill as many error_propagate() as I can.

We can pass @errp directly unless

a. we need to check for failure by checking whether an error was set, or

b. we want to use error_prepend() or error_append_hint().

We can often avoid (a) by making the function return a distinct error
value.  I finally codified this practice in commit e3fe3988d78, and
updated a substantial amount of code to work that way ("[PATCH v4 00/45]
Less clumsy error checking", commit b6d7e9b66f..a43770df5d).  The
diffstat illustrates the severity of 1.: 275 files changed, 2419
insertions(+), 3558 deletions(-).

ERRP_GUARD() mitigates the remaining propagations: 1. becomes much
better, 2. is addressed fully, 3. remains.

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

You must use ERRP_GUARD() in functions that dereference their @errp
parameter (so that works even when the argument is null) or pass it to
error_prepend() or error_append_hint() (so they get reached even when
the argumentis &error_fatal).

You should use Use ERRP_GUARD() to avoid clumsy error propagation.

You should not use ERRP_GUARD() when propagation is not actually needed.

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

They do.

In fact, ERRP_GUARD() creates local variable under the hood.

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

It's the usual plan

1. Put the rule in writing (done)

2. Eliminate the bad examples (in progress)

We could additionally

3. Make checkpatch.pl flag (some) rule violations, but I go there only
when I know it's necessary.

The plan's success depends on carrying through 2.  That's where many an
ambitious improvement has stalled for us.  I'm confident on this one,
because Vladimir *automated* the conversion to ERRP_GUARD(): commit
8220f3ac74 "scripts: Coccinelle script to use ERRP_GUARD()".

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