Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Peter Crosthwaite]

On Wed, Jun 18, 2014 at 12:12 AM, Paolo Bonzini <address@hidden> wrote:
> Il 17/06/2014 16:09, Peter Crosthwaite ha scritto:
>
>>> > +
>>> > +    object_property_add_alias(qdev_get_machine(), "rtc",
>>> > +                              OBJECT(s), NULL, &error_abort);
>>
>> This will fail if anyone wants to add two such devices to a machine
>> model. It seems a bit board specific to assume that this device is
>> only valid as a singleton. Perhaps s/&error_abort/local_err/ and
>> raising a warning explaining that only the first RTC in the system
>> gets the alias?
>
>
> &error_warn? :)  But then, /machine/rtc.tm is the only supported interface,

I have thought about &error_warn in the past, but thought the better
of it. Warnings need user digestable message and I can't think of a
sane implementation where you populate a message for error framework
to raise while maintaining the needed errp == NULL || *errp == NULL
semantic at the same time.

> and it should be the same for all RTC devices so perhaps a NULL error
> pointer is enough.
>

NULL Works for me. But other than source verbosity is there any reason
to not catch the error?:

object_property_add_alias(qdev_get_machine(), "rtc",
                              OBJECT(s), NULL, &local_err);
if (local_err) {
    error_report("WARNING: Multiple RTCs in system, not aliasing %s",
object_get_canonical_path(OBJECT(s)));
   /* NOTE: no exit(1) */
}

Regards,
Peter

>
>> The other options is arrayification using your (hot off the press)
>> "[*]" proposal:
>>
>> +    object_property_add_alias(qdev_get_machine(), "rtc[*]",
>> +                              OBJECT(s), NULL, &error_abort);
>
>
> This could make sense, but would leave /machine/rtc[0] in the common case.
> I prefer raising a warning.
>
> Paolo
>