Re: [PATCH 12/17] qdev: Clean up qdev_connect_gpio_out_named()

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 4/28/20 6:34 PM, Markus Armbruster wrote:
> Both qdev_connect_gpio_out_named() and device_set_realized() put
> objects without a parent into the "/machine/unattached/" orphanage.
> 
> qdev_connect_gpio_out_named() needs a lengthy comment to explain how
> it works.  It exploits that object_property_add_child() can fail only
> when we got a parent already, and ignoring that error does what we
> want.  True.  If it failed due to "duplicate property", we'd be in
> trouble, but that would be a programming error.
> 
> device_set_realized() is cleaner: it checks whether we need a parent,
> then calls object_property_add_child(), aborting on failure.  No need
> for a comment, and programming errors get caught.

I suppose it was not that obvious at the time of that comment :)

commit 615c4895703164134379b68214130dd502721174
Author: Andreas Färber <afaerber@suse.de>
Date:   Wed Jun 18 00:57:08 2014 -0700

     irq: Slim conversion of qemu_irq to QOM

     As a prequel to any big Pin refactoring plans,
     do an in-place conversion of qemu_irq to an Object,
     so that we can reference it in link<> properties.


commit 02757df2ad2d5dfc96482e2cdfa046f439dafc3d
Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Date:   Thu Sep 25 22:20:25 2014 -0700

     qdev: gpio: Re-implement qdev_connect_gpio QOM style

     Re-implement as a link setter. This should allow the
     QOM framework to keep track of ref counts properly etc.

     We need to add a default parent for the connecting
     input in case it's coming from a non-qdev source.
     We simply parent the IRQ to the machine in this case.

> 
> Change qdev_connect_gpio_out_named() to match.
> 
> Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

> ---
>   hw/core/qdev.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c3a6a11b19..888a1de931 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -541,15 +541,12 @@ void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
>   {
>       char *propname = g_strdup_printf("%s[%d]",
>                                        name ? name : "unnamed-gpio-out", n);
> -    if (pin) {
> -        /* We need a name for object_property_set_link to work.  If the
> -         * object has a parent, object_property_add_child will come back
> -         * with an error without doing anything.  If it has none, it will
> -         * never fail.  So we can just call it with a NULL Error pointer.
> -         */
> +    if (pin && !OBJECT(pin)->parent) {
> +        /* We need a name for object_property_set_link to work */
>           object_property_add_child(container_get(qdev_get_machine(),
>                                                   "/unattached"),
> -                                  "non-qdev-gpio[*]", OBJECT(pin), NULL);
> +                                  "non-qdev-gpio[*]", OBJECT(pin),
> +                                  &error_abort);
>       }
>       object_property_set_link(OBJECT(dev), OBJECT(pin), propname, &error_abort);
>       g_free(propname);
>