| Message ID | 20230207075115.1525-10-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | error: Reduce qerror.h usage a bit more | expand |
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). > } > }
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
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index 87ca83b155..09006e69f7 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -59,9 +59,6 @@ #define QERR_QGA_COMMAND_FAILED \ "Guest agent command failed, error was '%s'" -#define QERR_REPLAY_NOT_SUPPORTED \ - "Record/replay feature is not supported for '%s'" - #define QERR_UNSUPPORTED \ "this feature or command is not currently supported" diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h index 7ec0882b50..6e5ab09f71 100644 --- a/include/sysemu/replay.h +++ b/include/sysemu/replay.h @@ -72,7 +72,7 @@ void replay_start(void); /*! Closes replay log file and frees other resources. */ void replay_finish(void); /*! Adds replay blocker with the specified error description */ -void replay_add_blocker(Error *reason); +void replay_add_blocker(const char *feature); /* Returns name of the replay log file */ const char *replay_get_filename(void); /* 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); 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"); } } @@ -2770,13 +2768,8 @@ void qemu_init(int argc, char **argv) drive_add(IF_PFLASH, -1, optarg, PFLASH_OPTS); break; case QEMU_OPTION_snapshot: - { - Error *blocker = NULL; - snapshot = 1; - error_setg(&blocker, QERR_REPLAY_NOT_SUPPORTED, - "-snapshot"); - replay_add_blocker(blocker); - } + snapshot = 1; + replay_add_blocker("-snapshot"); break; case QEMU_OPTION_numa: opts = qemu_opts_parse_noisily(qemu_find_opts("numa"),
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(-)