Re: [RFC PATCH-for-5.2 4/5] qom: Let ObjectPropertyGet functions return a boolean value

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Commits 1c94a35164..7b3cb8037c simplified the error propagation.

The complete series is b6d7e9b66f..a43770df5d.  The part you quoted
omits half of the transformation for qemu-option and QAPI.  The other
half is in

    a5f9b9df25 error: Reduce unnecessary error propagation
    992861fb1e error: Eliminate error_propagate() manually
    af175e85f9 error: Eliminate error_propagate() with Coccinelle, part 2
    668f62ec62 error: Eliminate error_propagate() with Coccinelle, part 1
    dcfe480544 error: Avoid unnecessary error_propagate() after error_setg()

I'd simply point to the complete series.

> Similarly to commit 73ac1aac39 ("qdev: Make functions taking
> Error ** return bool, not void")

I think commit 6fd5bef10b "qom: Make functions taking Error ** return
bool, not void" would be a closer match.

>                                  let the ObjectPropertyGet
> functions return a boolean value, not void.

The conversion simplifies most calls at the cost of slightly
complicating the callee.  The complete series quoted above shows it can
be a good trade:

 276 files changed, 2424 insertions(+), 3567 deletions(-)

> See commit e3fe3988d7 ("error: Document Error API usage rules")
> for rationale.
>
> Cc: armbru@redhat.com
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Sorry I don't see how to split that patch without using
> ugly casts in the middle.
> ---
[...]
>  57 files changed, 325 insertions(+), 281 deletions(-)

This one's different: it converts a callback.  Many more functions than
calls.

It still improves consistency.

It should also help reduce Coverity CHECKED_RETURN false positives in
getters; see below.

>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index e9496ba970..7ba2172932 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -333,9 +333,11 @@ typedef void (ObjectPropertySet)(Object *obj,
>   * @opaque: the object property opaque
>   * @errp: a pointer to an Error that is filled if getting fails.
>   *
> + * Return true on success, false on failure.
> + *
>   * Called when trying to get a property.
>   */
> -typedef void (ObjectPropertyGet)(Object *obj,
> +typedef bool (ObjectPropertyGet)(Object *obj,
>                                   Visitor *v,
>                                   const char *name,
>                                   void *opaque,
[...]
> diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> index 08aede88dc..d59eadd4da 100644
> --- a/target/ppc/compat.c
> +++ b/target/ppc/compat.c
> @@ -238,7 +238,7 @@ int ppc_compat_max_vthreads(PowerPCCPU *cpu)
>      return n_threads;
>  }
>  
> -static void ppc_compat_prop_get(Object *obj, Visitor *v, const char *name,
> +static bool ppc_compat_prop_get(Object *obj, Visitor *v, const char *name,
>                                  void *opaque, Error **errp)
>  {
>      uint32_t compat_pvr = *((uint32_t *)opaque);
> @@ -254,7 +254,7 @@ static void ppc_compat_prop_get(Object *obj, Visitor *v, 
> const char *name,
>          value = compat->name;
>      }
>  
> -    visit_type_str(v, name, (char **)&value, errp);
> +    return visit_type_str(v, name, (char **)&value, errp);

Before the patch, Coverity reports

   CID 1430435:  Error handling issues  (CHECKED_RETURN)
   Calling "visit_type_str" without checking return value (as is done elsewhere 
531 out of 561 times).

False positive; not checking is fine here.  Still, avoiding false
positives is nice, as long as it can be done without impairing
readability.

>  }
>  
>  static void ppc_compat_prop_set(Object *obj, Visitor *v, const char *name,
[...]