Re: [PATCH 0/2] qapi: Move RTC_CHANGE back out of target schema

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Sat, 25 Sept 2021 at 08:44, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > This patchset moves RTC_CHANGE back to misc.json, effectively
>> > reverting commit 183e4281a30962, which moved the RTC_CHANGE event to
>> > the target schema.  That change was an attempt to make the event
>> > target-specific to improve introspection, but the event isn't really
>> > target-specific: it's machine or device specific.  Putting RTC_CHANGE
>> > in the target schema with an ifdef list reduces maintainability (by
>> > adding an if: list with a long list of targets that needs to be
>> > manually updated as architectures are added or removed or as new
>> > devices gain the RTC_CHANGE functionality) and increases compile time
>> > (by preventing RTC devices which emit the event from being "compile
>> > once" rather than "compile once per target", because
>> > qapi-events-misc-target.h uses TARGET_* ifdefs, which are poisoned in
>> > "compile once" files.)
>> >
>> > Patch 2 fixes a minor documentation issue that I noticed while
>> > I was doing this -- we didn't document that the units used in
>> > the RTC_CHANGE event are seconds.
>>
>> Series
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> I realized that this patchset never got applied -- I think I was
> expecting it to be picked up via a QAPI related tree, and then
> it was a bit close to a release to be put in, or something.
> Anyway, I'm going to resend it in a moment.

Want me to take care of merging v2?

>> An additional patch documenting that not all RTCs implement RTC_CHANGE
>> would be nice.  Listing them would be even nicer.
>
> I disagree that listing them would be nice -- the whole point of
> the series is to avoid having lists that get out of date when we
> add a new RTC implementation or fix the missing-feature in an
> existing one. I can add a sentence to the patch 2 docs change:
> "Note that it is not guaranteed that the RTC in a system implements
> this event, or even that the system has an RTC at all."

For a user, "you can rely on RTC_CHANGE with RTCs x, y, z provided by
machines a, b, c" is definitely nicer than "RTC_CHANGE may or may not
work, good luck", which is in turn nicer than nothing at all.

I think you're arguing for being as nice to users as we can without
having to pay for it in maintenance, which is fair.

>> An additional patch adding @qom-path event argument would be nice.
>
> I don't understand what this would involve, so I'll leave it to you
> if you want it.

Okay.