Re: [PATCH 34/46] qom: Don't handle impossible object_property_get_link() failure

[Markus Armbruster]

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

> On 6/25/20 5:09 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>>> On 6/24/20 6:43 PM, Markus Armbruster wrote:
>>>> Don't handle object_property_get_link() failure that can't happen
>>>> unless the programmer screwed up, pass &error_abort.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>  hw/arm/bcm2835_peripherals.c |  7 +------
>>>>  hw/arm/bcm2836.c             |  7 +------
>>>>  hw/display/bcm2835_fb.c      |  8 +-------
>>>>  hw/dma/bcm2835_dma.c         |  9 +--------
>>>>  hw/gpio/bcm2835_gpio.c       | 15 ++-------------
>>>>  hw/intc/nios2_iic.c          |  8 +-------
>>>>  hw/misc/bcm2835_mbox.c       |  9 +--------
>>>>  hw/misc/bcm2835_property.c   | 17 ++---------------
>>>>  hw/usb/hcd-dwc2.c            |  9 +--------
>>>>  9 files changed, 11 insertions(+), 78 deletions(-)
>>>>
>>>> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
>>>> index 8313410ffe..3c40bda91e 100644
>>>> --- a/hw/arm/bcm2835_peripherals.c
>>>> +++ b/hw/arm/bcm2835_peripherals.c
>>>> @@ -134,12 +134,7 @@ static void bcm2835_peripherals_realize(DeviceState 
>>>> *dev, Error **errp)
>>>>      uint64_t ram_size, vcram_size;
>>>>      int n;
>>>>  
>>>> -    obj = object_property_get_link(OBJECT(dev), "ram", &err);
>>>> -    if (obj == NULL) {
>>>> -        error_setg(errp, "%s: required ram link not found: %s",
>>>> -                   __func__, error_get_pretty(err));
>>>> -        return;
>>>> -    }
>>>> +    obj = object_property_get_link(OBJECT(dev), "ram", &error_abort);
>>> [...]
>>>
>>> Should we now add an assert(errp) in object_property_get_link()?
>>> Basically this would force forks to adapt their code when
>>> rebasing.
>> 
>> Functions should not place additional restrictions @errp arguments
>> without a compelling reason.
>
> My compelling reason is you spent quite some time cleaning, then we'll
> merge old patches based on examples previous your cleanup, and either
> you'll have to clean again, or the codebase will get inconsistent again.

We might also merge patches that ignore errors for perfectly sane
reasons.  We'll then debug the crash, and take out the assertion again.

>> What if you want genuinely don't need the
>> error details when object_property_get_link() fails?  Passing null is
>> better than passing &err only to error_free(err).
>
> So what about:
>
> -- >8 --
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1372,9 +1372,11 @@ void object_property_set_link(Object *obj, Object
> *value,
>  Object *object_property_get_link(Object *obj, const char *name,
>                                   Error **errp)
>  {
> -    char *str = object_property_get_str(obj, name, errp);
> +    char *str;
>      Object *target = NULL;
>
> +    assert(errp == NULL || errp == &error_abort || errp == &error_fatal);
> +    str = object_property_get_str(obj, name, errp);
>      if (str && *str) {
>          target = object_resolve_path(str, NULL);
>          if (!target) {
> ---

Consider an @obj that comes in two flavours, one with and one without
the link.  Code handles both, but we still want the Error object for
tracing purposes:

    linked_obj = object_property_get_link(obj, "some-link-prop", &err);
    if (!linked_obj) {
        char *obj_name = object_get_canonical_path_component(obj);
        trace_frob_get_som_link_prop_failed(obj_name, error_get_pretty(err));
        g_free(obj_name);
        error_free(err);
    }

Such use of Error objects just for tracing exists in the tree already.

>
>> 
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> 
>> Thanks!
>>