Re: [Qemu-devel] [PATCH 4/6] hw/timer/mc146818rtc: Fix introspection problem

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

> There is currently a funny problem with the "mc146818rtc" device:
> 1) Start QEMU like this:
>    qemu-system-ppc64 -M pseries -S
> 2) At the HMP monitor, enter "info qom-tree". Note that there is an
>    entry for "/rtc (spapr-rtc)".
> 3) Introspect the mc146818rtc device like this:
>    device_add mc146818rtc,help
> 4) Run "info qom-tree" again. The "/rtc" entry is gone now!
>
> The rtc_finalize() function of the mc146818rtc device has two bugs: First,
> it tries to remove a "rtc" property, while the rtc_realizefn() added a
> "rtc-time" property instead. And second, it should have been done in an
> unrealize function, not in a finalize function, to avoid that this causes
> problems during introspection.
>
> But since adding aliases to the global machine state should not be done
> from a device's realize function anyway, let's rather fix this issue
> by moving the creation of the alias to the code that creates the device
> (and thus is run from the machine init functions instead). We can then
> remove the object_property_del() completely. In prep.c, the code for
> setting up the alias is added to the function which deals already with
> the rtc device for the "40p" machine. The "prep" machine is ignored
> here since it is scheduled for removal anyway.

That's okay as long as this patch goes in after the removal.  And then
you don't need this sentence.

> Fixes: 654a36d857ff949e0d1989904b76f53fded9dc83
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  hw/ppc/prep.c          |  3 +++
>  hw/timer/mc146818rtc.c | 12 +++---------
>  2 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 3401570..91a8f42 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -696,6 +696,9 @@ static int prep_set_cmos_checksum(DeviceState *dev, void 
> *opaque)
>          rtc_set_memory(rtc, 0x3e, checksum & 0xff);
>          rtc_set_memory(rtc, 0x2f, checksum >> 8);
>          rtc_set_memory(rtc, 0x3f, checksum >> 8);
> +
> +        object_property_add_alias(qdev_get_machine(), "rtc-time", 
> OBJECT(rtc),
> +                                  "date", NULL);
>      }
>      return 0;
>  }
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 3a14075..a504f03 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -995,9 +995,6 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>  
>      object_property_add_tm(OBJECT(s), "date", rtc_get_date, NULL);
>  
> -    object_property_add_alias(qdev_get_machine(), "rtc-time",
> -                              OBJECT(s), "date", NULL);
> -
>      qdev_init_gpio_out(dev, &s->irq, 1);
>  }
>  
> @@ -1019,6 +1016,9 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int 
> base_year, qemu_irq intercept_irq)
>      }
>      QLIST_INSERT_HEAD(&rtc_devices, s, link);
>  
> +    object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(s),
> +                              "date", NULL);
> +
>      return isadev;
>  }
>  
> @@ -1052,17 +1052,11 @@ static void rtc_class_initfn(ObjectClass *klass, void 
> *data)
>      dc->user_creatable = false;
>  }
>  
> -static void rtc_finalize(Object *obj)
> -{
> -    object_property_del(qdev_get_machine(), "rtc", NULL);
> -}
> -
>  static const TypeInfo mc146818rtc_info = {
>      .name          = TYPE_MC146818_RTC,
>      .parent        = TYPE_ISA_DEVICE,
>      .instance_size = sizeof(RTCState),
>      .class_init    = rtc_class_initfn,
> -    .instance_finalize = rtc_finalize,
>  };
>  
>  static void mc146818rtc_register_types(void)

Let's see...  the device is created in two places, mc146818_rtc_init()
and i82378_realize().  You add the alias in the former, but not the
latter.  Instead, you add it where the i82378 device is created: in
ibm_40p_init().  ppc_prep_init() also creates it, but you intentionally
ignore that one.  Net effect: you add the alias a little later.  Looks
good to me.

Preferably with the superfluous sentence deleted from the commit
message:
Reviewed-by: Markus Armbruster <address@hidden>