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

[Philippe Mathieu-Daudé]

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.

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

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