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.

thanks
-- PMM

Peter Maydell (2):
  qapi: Move RTC_CHANGE back out of target schema
  qapi: Document the units for the offset argument to RTC_CHANGE

 qapi/misc-target.json | 33 ---------------------------------
 qapi/misc.json        | 22 ++++++++++++++++++++++
 hw/ppc/spapr_rtc.c    |  2 +-
 hw/rtc/mc146818rtc.c  |  2 +-
 4 files changed, 24 insertions(+), 35 deletions(-)

-- 
2.20.1

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>

An additional patch documenting that not all RTCs implement RTC_CHANGE
would be nice.  Listing them would be even nicer.

An additional patch adding @qom-path event argument would be nice.

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

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.