Re: [PATCH v3 1/8] reset: allow registering handlers that aren't called by snapshot loading

[Markus Armbruster <armbru@redhat.com>]

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

> (Cc'ing Markus for a QMP related question.)
>
> On Fri, 14 Oct 2022 at 03:17, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>
>> Snapshot loading only expects to call deterministic handlers, not
>> non-deterministic ones. So introduce a way of registering handlers that
>> won't be called when reseting for snapshots.
>>
>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 48e85c052c..a0cdb714f7 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -3058,7 +3058,7 @@ bool load_snapshot(const char *name, const char *vmstate,
>>          goto err_drain;
>>      }
>>
>> -    qemu_system_reset(SHUTDOWN_CAUSE_NONE);
>> +    qemu_system_reset(SHUTDOWN_CAUSE_SNAPSHOT_LOAD);
>>      mis->from_src_file = f;
>>
>>      if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
>> diff --git a/qapi/run-state.json b/qapi/run-state.json
>> index 9273ea6516..74ed0ac93c 100644
>> --- a/qapi/run-state.json
>> +++ b/qapi/run-state.json
>> @@ -86,12 +86,14 @@
>>  #                   ignores --no-reboot. This is useful for sanitizing
>>  #                   hypercalls on s390 that are used during kexec/kdump/boot
>>  #
>> +# @snapshot-load: A snapshot is being loaded by the record & replay subsystem.
>> +#
>>  ##
>>  { 'enum': 'ShutdownCause',
>>    # Beware, shutdown_caused_by_guest() depends on enumeration order
>>    'data': [ 'none', 'host-error', 'host-qmp-quit', 'host-qmp-system-reset',
>>              'host-signal', 'host-ui', 'guest-shutdown', 'guest-reset',
>> -            'guest-panic', 'subsystem-reset'] }
>> +            'guest-panic', 'subsystem-reset', 'snapshot-load'] }
>
> Markus: do we need to mark new enum values with some kind of "since n.n"
> version info ?

We do!  Commonly like

    # @snapshot-load: A snapshot is being loaded by the record & replay
    #                 subsystem (since 7.2)

>>  ##
>>  # @StatusInfo:
>> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
>> index 1e68680b9d..03e1ee3a8a 100644
>> --- a/softmmu/runstate.c
>> +++ b/softmmu/runstate.c
>> @@ -441,9 +441,9 @@ void qemu_system_reset(ShutdownCause reason)
>>      cpu_synchronize_all_states();
>>
>>      if (mc && mc->reset) {
>> -        mc->reset(current_machine);
>> +        mc->reset(current_machine, reason);
>>      } else {
>> -        qemu_devices_reset();
>> +        qemu_devices_reset(reason);
>>      }
>>      if (reason && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
>>          qapi_event_send_reset(shutdown_caused_by_guest(reason), reason);
>
> This change means that resets on snapshot-load, which previously
> did not result in sending a QMP event, will now start to do so
> with this new ShutdownCause type. I don't think we want that
> change in behaviour.
>
> (Also, as per the 'Beware' comment in run-state.json, we will
> claim this to be a 'caused by guest' reset, which doesn't seem
> right. If we suppress the sending the event that is moot, though.)
>
> Markus: if we add a new value to the ShutdownCause enumeration,
> how annoying is it if we decide we don't want it later? I guess
> we can just leave it in the enum unused... (In this case we're
> using it for purely internal purposes and it won't ever actually
> wind up in any QMP events.)

Deleting enumeration values is a compatibility issue only if the value
is usable in QMP input.

"Purely internal" means it cannot occur in QMP output, and any attempt
to use it in input fails.  Aside: feels a bit fragile.

Having an enumeration type where some values are like this is mildly
ugly, because introspection still shows the value.

If we care about fragile or mildly ugly, we can mark such values with a
special feature flag, and have the QAPI generator keep them internal
(input, output, introspection).

Does this answer your question?