Re: [PATCH v2] hw/core/qdev: Increase qdev_realize() kindness

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Since commit 510ef98dca5, qdev_realize() aborts if bus-less
> device is realized on a bus. Be kind with the developer by
> displaying a hint about what is wrong.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> v2: Use errp (bonzini suggestion)
>
> Paolo, I was tempted to check errp is really &error_abort else
> it is an error! :P
>
>     } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>         error_setg(errp, "%s: Unexpected bus '%s' for bus-less device '%s'",
>                    __func__, DEVICE_GET_CLASS(dev)->bus_type,
>                    object_get_typename(OBJECT(dev)));
>         assert(errp == &error_abort); // <--------------

Don't!

Functions taking an Error **errp parameter to report errors should never
examine the argument.

The Error API is for separating concerns.  The callee's concern is
detecting errors and failing cleanly.  *Handling* the errors is the
caller's concern.

To simplify common handling patterns, we provide convenience arguments
&error_abort, &error_fatal, and NULL.  Their use is exclusively the
caller's concern.

Examining the argument undermines the separation of concerns.

>         return false;
>     }
> ---
>  hw/core/qdev.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 2131c7f951..9d1530c39d 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -392,8 +392,11 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
>  
>      if (bus) {
>          qdev_set_parent_bus(dev, bus);
> -    } else {
> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
> +        error_setg(errp, "%s: Unexpected bus '%s' for bus-less device '%s'",
> +                   __func__, DEVICE_GET_CLASS(dev)->bus_type,
> +                   object_get_typename(OBJECT(dev)));
> +        return false;
>      }
>  
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);

Scratch __func__.  error_setg() records __FILE__, __LINE__ and __func__,
and error_handle_fatal() prints them.

Always, always, always test your error messages.  If they are impossible
to test, mock up the error.  If mocking up is too much trouble, then
reporting the error nicely is, too.

That said, this one matters to me only insofar as it sets a bad example.
I don't actually care how impossible error messages come out :)