Re: [PATCH 09/12] replay: Simplify setting replay blockers

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 7/2/23 08:51, Markus Armbruster wrote:
>> replay_add_blocker() takes an Error *.  All callers pass one created
>> like this:
>> 
>>      error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "some feature");
>> 
>> Folding this into replay_add_blocker() simplifies the callers, losing
>> a bit of generality we haven't needed in more than six years.
>> 
>> Since there are no other uses of macro QERR_REPLAY_NOT_SUPPORTED,
>> replace the remaining one by its expansion, and drop the macro.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   include/qapi/qmp/qerror.h |  3 ---
>>   include/sysemu/replay.h   |  2 +-
>>   replay/replay.c           |  6 +++++-
>>   replay/stubs-system.c     |  2 +-
>>   softmmu/rtc.c             |  5 +----
>>   softmmu/vl.c              | 13 +++----------
>>   6 files changed, 11 insertions(+), 20 deletions(-)
>
>
>> diff --git a/replay/replay.c b/replay/replay.c
>> index 9a0dc1cf44..c39156c522 100644
>> --- a/replay/replay.c
>> +++ b/replay/replay.c
>> @@ -376,8 +376,12 @@ void replay_finish(void)
>>       replay_mode = REPLAY_MODE_NONE;
>>   }
>>   
>> -void replay_add_blocker(Error *reason)
>> +void replay_add_blocker(const char *feature)
>>   {
>> +    Error *reason = NULL;
>> +
>> +    error_setg(&reason, "Record/replay feature is not supported for '%s'",
>> +               feature);
>
> Either name 'feature' as 'cli_option' and use '-%s' in format,
>
>>       replay_blockers = g_slist_prepend(replay_blockers, reason);
>>   }
>>   
>> diff --git a/replay/stubs-system.c b/replay/stubs-system.c
>> index 5c262b08f1..50cefdb2d6 100644
>> --- a/replay/stubs-system.c
>> +++ b/replay/stubs-system.c
>> @@ -12,7 +12,7 @@ void replay_input_sync_event(void)
>>       qemu_input_event_sync_impl();
>>   }
>>   
>> -void replay_add_blocker(Error *reason)
>> +void replay_add_blocker(const char *feature)
>>   {
>>   }
>>   void replay_audio_in(size_t *recorded, void *samples, size_t *wpos, size_t 
>> size)
>> diff --git a/softmmu/rtc.c b/softmmu/rtc.c
>> index f7114bed7d..4b2bf75dd6 100644
>> --- a/softmmu/rtc.c
>> +++ b/softmmu/rtc.c
>> @@ -152,11 +152,8 @@ void configure_rtc(QemuOpts *opts)
>>           if (!strcmp(value, "utc")) {
>>               rtc_base_type = RTC_BASE_UTC;
>>           } else if (!strcmp(value, "localtime")) {
>> -            Error *blocker = NULL;
>>               rtc_base_type = RTC_BASE_LOCALTIME;
>> -            error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED,
>> -                      "-rtc base=localtime");
>> -            replay_add_blocker(blocker);
>> +            replay_add_blocker("-rtc base=localtime");
>>           } else {
>>               rtc_base_type = RTC_BASE_DATETIME;
>>               configure_rtc_base_datetime(value);
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 9177d95d4e..9d324fc6cd 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -1855,9 +1855,7 @@ static void qemu_apply_machine_options(QDict *qdict)
>>       }
>>   
>>       if (current_machine->smp.cpus > 1) {
>> -        Error *blocker = NULL;
>> -        error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, "smp");
>> -        replay_add_blocker(blocker);
>> +        replay_add_blocker("smp");
>
> ... or use "-smp" here (yes, pre-existing).
>
>>       }
>>   }

This patch doesn't change error messages.  If we want to improve some,
we should do so in a separate patch.

Let's review the error messages that pass through replay_add_blocker().

0. General format

    qemu-system-x86_64: Record/replay: Record/replay feature is not supported 
for '%s'

   Pretty bad.  Better:

    qemu-system-x86_64: record/replay is not supported %s

   Still neglects to identify the erroneous bit of configuration like we
   do elsewhere, e.g.

    $ qemu-system-x86_64 -device e100
    qemu-system-x86_64: -device e100: 'e100' is not a valid device model name

   Let's not worry about that now.

1. SMP

    $ qemu-system-x86_64 -icount shift=auto,rr=record,rrfile=/dev/null -smp 2
    qemu-system-x86_64: Record/replay: Record/replay feature is not supported 
for 'smp'

   First attempt at improvement:

    qemu-system-x86_64: record/replay is not supported with -smp

   But that's a lie, it works just fine with -smp 1.  So:

    qemu-system-x86_64: record/replay is not supported with multiple CPUs

2. RTC

    $ qemu-system-x86_64 -icount shift=auto,rr=record,rrfile=/dev/null -rtc 
base=localtime
    qemu-system-x86_64: Record/replay: Record/replay feature is not supported 
for '-rtc base=localtime'

   Obvious improvement:

    qemu-system-x86_64: record/replay is not supported with -rtc base=localtime

   Fine, except for the part where we assume where the configuration
   comes from.  Watch this:

    $ cat rtc.cfg
    [rtc]
        base = "localtime"
    $ qemu-system-x86_64 -icount shift=auto,rr=record,rrfile=/dev/null 
-readconfig rtc.cfg
    qemu-system-x86_64: Record/replay: Record/replay feature is not supported 
for '-rtc base=localtime'

   To make sense of this, user needs to make the connection from "-rtc
   base=localtime" to what he actually wrote in the configuration file.
   Let's not worry about that now, either.

3. Snapshot

    $ qemu-system-x86_64 -icount shift=auto,rr=record,rrfile=/dev/null -snapshot
    qemu-system-x86_64: Record/replay: Record/replay feature is not supported 
for '-snapshot'

   Obvious improvement:

    qemu-system-x86_64: record/replay is not supported with -snapshot