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

[Peter Maydell]

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.

> 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."

> 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.

thanks
-- PMM