[PATCH 00/12] error: Reduce qerror.h usage a bit more

[PATCH 00/12] error: Reduce qerror.h usage a bit more

[Markus Armbruster]

This series gets rid of two out of 15 remaining QERR_ macros and
confines use of one more to qga/.  Bonus: better error messages.

Markus Armbruster (12):
  error: Drop superfluous #include "qapi/qmp/qerror.h"
  dump: Improve error message when target doesn't support memory dump
  dump: Assert cpu_get_note_size() can't fail
  hw/core: Improve error message when machine doesn't provide NMIs
  hw/smbios: Dumb down smbios_entry_add() stub
  hw/acpi: Dumb down acpi_table_add() stub
  hw/acpi: Move QMP command to hw/core/
  qga: Drop dangling reference to QERR_QGA_LOGGING_DISABLED
  replay: Simplify setting replay blockers
  hw/core: Improve the query-hotpluggable-cpus error message
  migration/colo: Improve an x-colo-lost-heartbeat error message
  rocker: Tweak stubbed out monitor commands' error messages

 MAINTAINERS                    |  1 -
 include/qapi/qmp/qerror.h      |  6 ------
 include/sysemu/replay.h        |  2 +-
 authz/listfile.c               |  1 -
 backends/cryptodev-vhost.c     |  1 -
 backends/rng.c                 |  1 -
 backends/vhost-user.c          |  1 -
 block/backup.c                 |  1 -
 block/commit.c                 |  1 -
 block/mirror.c                 |  1 -
 block/stream.c                 |  1 -
 dump/dump.c                    |  8 +++-----
 hw/acpi/acpi-stub.c            |  4 +---
 hw/acpi/vmgenid.c              | 18 ------------------
 hw/core/machine-qmp-cmds.c     | 21 +++++++++++++++++++--
 hw/core/machine.c              |  1 -
 hw/core/nmi.c                  |  3 +--
 hw/i386/pc.c                   |  1 -
 hw/i386/x86.c                  |  1 -
 hw/misc/xlnx-zynqmp-apu-ctrl.c |  1 -
 hw/net/rocker/qmp-norocker.c   | 12 +++++-------
 hw/smbios/smbios-stub.c        |  4 +---
 migration/colo-failover.c      |  3 +--
 migration/colo.c               |  1 -
 migration/migration-hmp-cmds.c |  1 -
 qga/commands.c                 |  5 ++---
 qga/main.c                     |  1 -
 replay/replay.c                |  6 +++++-
 replay/stubs-system.c          |  2 +-
 softmmu/qtest.c                |  1 -
 softmmu/rtc.c                  |  5 +----
 softmmu/vl.c                   | 13 +++----------
 stubs/vmgenid.c                | 10 ----------
 target/i386/monitor.c          |  1 -
 target/i386/sev-sysemu-stub.c  |  1 -
 target/i386/sev.c              |  1 -
 util/qemu-config.c             |  1 -
 stubs/meson.build              |  1 -
 38 files changed, 44 insertions(+), 100 deletions(-)
 delete mode 100644 stubs/vmgenid.c

-- 
2.39.0

[PATCH 01/12] error: Drop superfluous #include "qapi/qmp/qerror.h"

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 authz/listfile.c               | 1 -
 backends/cryptodev-vhost.c     | 1 -
 backends/rng.c                 | 1 -
 backends/vhost-user.c          | 1 -
 block/backup.c                 | 1 -
 block/commit.c                 | 1 -
 block/mirror.c                 | 1 -
 block/stream.c                 | 1 -
 hw/core/machine.c              | 1 -
 hw/i386/pc.c                   | 1 -
 hw/i386/x86.c                  | 1 -
 hw/misc/xlnx-zynqmp-apu-ctrl.c | 1 -
 migration/colo.c               | 1 -
 migration/migration-hmp-cmds.c | 1 -
 qga/main.c                     | 1 -
 softmmu/qtest.c                | 1 -
 target/i386/monitor.c          | 1 -
 target/i386/sev-sysemu-stub.c  | 1 -
 target/i386/sev.c              | 1 -
 util/qemu-config.c             | 1 -
 20 files changed, 20 deletions(-)

diff --git a/authz/listfile.c b/authz/listfile.c
index da3a0e69a2..45a60e987d 100644
--- a/authz/listfile.c
+++ b/authz/listfile.c
@@ -30,7 +30,6 @@
 #include "qapi/qapi-visit-authz.h"
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qobject.h"
-#include "qapi/qmp/qerror.h"
 #include "qapi/qobject-input-visitor.h"
 
 
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index 572f87b3be..74ea0ad63d 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -28,7 +28,6 @@
 
 #ifdef CONFIG_VHOST_CRYPTO
 #include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/error-report.h"
 #include "hw/virtio/virtio-crypto.h"
 #include "sysemu/cryptodev-vhost-user.h"
diff --git a/backends/rng.c b/backends/rng.c
index 6c7bf64426..9bbd0c77b6 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -13,7 +13,6 @@
 #include "qemu/osdep.h"
 #include "sysemu/rng.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/module.h"
 #include "qom/object_interfaces.h"
 
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
index 7bfcaef976..0596223ac4 100644
--- a/backends/vhost-user.c
+++ b/backends/vhost-user.c
@@ -13,7 +13,6 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/error-report.h"
 #include "qom/object_interfaces.h"
 #include "sysemu/vhost-user-backend.h"
diff --git a/block/backup.c b/block/backup.c
index 824d39acaa..5b8863b88c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -22,7 +22,6 @@
 #include "block/block-copy.h"
 #include "block/dirty-bitmap.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/cutils.h"
 #include "sysemu/block-backend.h"
 #include "qemu/bitmap.h"
diff --git a/block/commit.c b/block/commit.c
index 41e3599281..387a720d03 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -18,7 +18,6 @@
 #include "block/block_int.h"
 #include "block/blockjob_int.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
 #include "qemu/memalign.h"
 #include "sysemu/block-backend.h"
diff --git a/block/mirror.c b/block/mirror.c
index ab326b67c9..0f6551cf5b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -21,7 +21,6 @@
 #include "block/dirty-bitmap.h"
 #include "sysemu/block-backend.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
 #include "qemu/bitmap.h"
 #include "qemu/memalign.h"
diff --git a/block/stream.c b/block/stream.c
index 8744ad103f..e77a033233 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -16,7 +16,6 @@
 #include "block/block_int.h"
 #include "block/blockjob_int.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu/ratelimit.h"
 #include "sysemu/block-backend.h"
diff --git a/hw/core/machine.c b/hw/core/machine.c
index f7761baab5..89bf690365 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -13,7 +13,6 @@
 #include "qemu/osdep.h"
 #include "qemu/option.h"
 #include "qemu/accel.h"
-#include "qapi/qmp/qerror.h"
 #include "sysemu/replay.h"
 #include "qemu/units.h"
 #include "hw/boards.h"
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6e592bd969..a7a2ededf9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -92,7 +92,6 @@
 #include "hw/mem/memory-device.h"
 #include "sysemu/replay.h"
 #include "target/i386/cpu.h"
-#include "qapi/qmp/qerror.h"
 #include "e820_memory_layout.h"
 #include "fw_cfg.h"
 #include "trace.h"
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index eaff4227bd..48be7a1c23 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -28,7 +28,6 @@
 #include "qemu/datadir.h"
 #include "qemu/guest-random.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "qapi/qapi-visit-common.h"
 #include "qapi/clone-visitor.h"
 #include "qapi/qapi-visit-machine.h"
diff --git a/hw/misc/xlnx-zynqmp-apu-ctrl.c b/hw/misc/xlnx-zynqmp-apu-ctrl.c
index 20de23cf67..3d2be95e6d 100644
--- a/hw/misc/xlnx-zynqmp-apu-ctrl.c
+++ b/hw/misc/xlnx-zynqmp-apu-ctrl.c
@@ -18,7 +18,6 @@
 #include "hw/register.h"
 
 #include "qemu/bitops.h"
-#include "qapi/qmp/qerror.h"
 
 #include "hw/misc/xlnx-zynqmp-apu-ctrl.h"
 
diff --git a/migration/colo.c b/migration/colo.c
index 232c8d44b1..0716e64689 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -33,7 +33,6 @@
 #include "net/colo.h"
 #include "block/block.h"
 #include "qapi/qapi-events-migration.h"
-#include "qapi/qmp/qerror.h"
 #include "sysemu/cpus.h"
 #include "sysemu/runstate.h"
 #include "net/filter.h"
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index ef25bc8929..72519ea99f 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -23,7 +23,6 @@
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-visit-migration.h"
 #include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qerror.h"
 #include "qapi/string-input-visitor.h"
 #include "qapi/string-output-visitor.h"
 #include "qemu/cutils.h"
diff --git a/qga/main.c b/qga/main.c
index 85b7d6ced5..2b992a55b3 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -24,7 +24,6 @@
 #include "qapi/qmp/qjson.h"
 #include "guest-agent-core.h"
 #include "qga-qapi-init-commands.h"
-#include "qapi/qmp/qerror.h"
 #include "qapi/error.h"
 #include "channel.h"
 #include "qemu/cutils.h"
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index d3e0ab4eda..34bd2a33a7 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -28,7 +28,6 @@
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "qemu/cutils.h"
-#include "qapi/qmp/qerror.h"
 #include "qom/object_interfaces.h"
 #include CONFIG_DEVICES
 #ifdef CONFIG_PSERIES
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index ad5b7b8bb5..6512846327 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -28,7 +28,6 @@
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 #include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qerror.h"
 #include "sysemu/kvm.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc-target.h"
diff --git a/target/i386/sev-sysemu-stub.c b/target/i386/sev-sysemu-stub.c
index 7a29295d1e..96e1c15cc3 100644
--- a/target/i386/sev-sysemu-stub.c
+++ b/target/i386/sev-sysemu-stub.c
@@ -15,7 +15,6 @@
 #include "monitor/monitor.h"
 #include "monitor/hmp-target.h"
 #include "qapi/qapi-commands-misc-target.h"
-#include "qapi/qmp/qerror.h"
 #include "qapi/error.h"
 #include "sev.h"
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 32f7dbac4e..0ec970496e 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -34,7 +34,6 @@
 #include "monitor/monitor.h"
 #include "monitor/hmp-target.h"
 #include "qapi/qapi-commands-misc-target.h"
-#include "qapi/qmp/qerror.h"
 #include "exec/confidential-guest-support.h"
 #include "hw/i386/pc.h"
 #include "exec/address-spaces.h"
diff --git a/util/qemu-config.c b/util/qemu-config.c
index d63f27438d..42076efe1e 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -2,7 +2,6 @@
 #include "block/qdict.h" /* for qdict_extract_subqdict() */
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc.h"
-#include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
 #include "qemu/error-report.h"
-- 
2.39.0

Re: [PATCH 01/12] error: Drop superfluous #include "qapi/qmp/qerror.h"

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  authz/listfile.c               | 1 -
>  backends/cryptodev-vhost.c     | 1 -
>  backends/rng.c                 | 1 -
>  backends/vhost-user.c          | 1 -
>  block/backup.c                 | 1 -
>  block/commit.c                 | 1 -
>  block/mirror.c                 | 1 -
>  block/stream.c                 | 1 -
>  hw/core/machine.c              | 1 -
>  hw/i386/pc.c                   | 1 -
>  hw/i386/x86.c                  | 1 -
>  hw/misc/xlnx-zynqmp-apu-ctrl.c | 1 -
>  migration/colo.c               | 1 -
>  migration/migration-hmp-cmds.c | 1 -
>  qga/main.c                     | 1 -
>  softmmu/qtest.c                | 1 -
>  target/i386/monitor.c          | 1 -
>  target/i386/sev-sysemu-stub.c  | 1 -
>  target/i386/sev.c              | 1 -
>  util/qemu-config.c             | 1 -
>  20 files changed, 20 deletions(-)
>
> diff --git a/authz/listfile.c b/authz/listfile.c

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 01/12] error: Drop superfluous #include "qapi/qmp/qerror.h"

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Tue, Feb 7, 2023 at 10:45 AM Juan Quintela <quintela@redhat.com> wrote:

> Markus Armbruster <armbru@redhat.com> wrote:
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  authz/listfile.c               | 1 -
> >  backends/cryptodev-vhost.c     | 1 -
> >  backends/rng.c                 | 1 -
> >  backends/vhost-user.c          | 1 -
> >  block/backup.c                 | 1 -
> >  block/commit.c                 | 1 -
> >  block/mirror.c                 | 1 -
> >  block/stream.c                 | 1 -
> >  hw/core/machine.c              | 1 -
> >  hw/i386/pc.c                   | 1 -
> >  hw/i386/x86.c                  | 1 -
> >  hw/misc/xlnx-zynqmp-apu-ctrl.c | 1 -
> >  migration/colo.c               | 1 -
> >  migration/migration-hmp-cmds.c | 1 -
> >  qga/main.c                     | 1 -
> >  softmmu/qtest.c                | 1 -
> >  target/i386/monitor.c          | 1 -
> >  target/i386/sev-sysemu-stub.c  | 1 -
> >  target/i386/sev.c              | 1 -
> >  util/qemu-config.c             | 1 -
> >  20 files changed, 20 deletions(-)
> >
> > diff --git a/authz/listfile.c b/authz/listfile.c
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
>

[-- Attachment #2: Type: text/html, Size: 2080 bytes --]

[PATCH 02/12] dump: Improve error message when target doesn't support memory dump

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
We've been trying to reduce their remaining use.

Get rid of a use of QERR_UNSUPPORTED, and improve the rather vague
error message

    (qemu) dump-guest-memory mumble
    Error: this feature or command is not currently supported

to

    Error: guest memory dumping is not supported on this target

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 dump/dump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/dump/dump.c b/dump/dump.c
index 279b07f09b..80620da40d 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1854,7 +1854,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
      */
     ret = cpu_get_dump_info(&s->dump_info, &s->guest_phys_blocks);
     if (ret < 0) {
-        error_setg(errp, QERR_UNSUPPORTED);
+        error_setg(errp,
+                   "guest memory dumping is not supported on this target");
         goto cleanup;
     }
 
-- 
2.39.0

Re: [PATCH 02/12] dump: Improve error message when target doesn't support memory dump

[Philippe Mathieu-Daudé]

On 7/2/23 08:51, Markus Armbruster wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> We've been trying to reduce their remaining use.
> 
> Get rid of a use of QERR_UNSUPPORTED, and improve the rather vague
> error message
> 
>      (qemu) dump-guest-memory mumble
>      Error: this feature or command is not currently supported
> 
> to
> 
>      Error: guest memory dumping is not supported on this target
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   dump/dump.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 279b07f09b..80620da40d 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1854,7 +1854,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>        */
>       ret = cpu_get_dump_info(&s->dump_info, &s->guest_phys_blocks);
>       if (ret < 0) {
> -        error_setg(errp, QERR_UNSUPPORTED);
> +        error_setg(errp,
> +                   "guest memory dumping is not supported on this target");

"Dumping guest memory is not supported on this target"?

>           goto cleanup;
>       }
>   

Re: [PATCH 02/12] dump: Improve error message when target doesn't support memory dump

[Markus Armbruster]

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

> On 7/2/23 08:51, Markus Armbruster wrote:
>> The QERR_ macros are leftovers from the days of "rich" error objects.
>> We've been trying to reduce their remaining use.
>> Get rid of a use of QERR_UNSUPPORTED, and improve the rather vague
>> error message
>>      (qemu) dump-guest-memory mumble
>>      Error: this feature or command is not currently supported
>> to
>>      Error: guest memory dumping is not supported on this target
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   dump/dump.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>> diff --git a/dump/dump.c b/dump/dump.c
>> index 279b07f09b..80620da40d 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -1854,7 +1854,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>>        */
>>       ret = cpu_get_dump_info(&s->dump_info, &s->guest_phys_blocks);
>>       if (ret < 0) {
>> -        error_setg(errp, QERR_UNSUPPORTED);
>> +        error_setg(errp,
>> +                   "guest memory dumping is not supported on this target");
>
> "Dumping guest memory is not supported on this target"?

Sold!

>>           goto cleanup;
>>       }
>>   

Re: [PATCH 02/12] dump: Improve error message when target doesn't support memory dump

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> We've been trying to reduce their remaining use.
>
> Get rid of a use of QERR_UNSUPPORTED, and improve the rather vague
> error message
>
>     (qemu) dump-guest-memory mumble
>     Error: this feature or command is not currently supported
>
> to
>
>     Error: guest memory dumping is not supported on this target
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Both your and philippe message are ok with me.

[PATCH 03/12] dump: Assert cpu_get_note_size() can't fail

[Markus Armbruster]

The only way cpu_get_note_size() can return a negative value is
integer overflow in the non-stub versions, which is a programming
error.  The stub version is not actually reachable, because the
cpu_get_dump_info() stub will fail first.  Use assert().  This gets
rid of another use of QERR_UNSUPPORTED.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 dump/dump.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 80620da40d..6be8a65fe3 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1865,10 +1865,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
 
     s->note_size = cpu_get_note_size(s->dump_info.d_class,
                                      s->dump_info.d_machine, nr_cpus);
-    if (s->note_size < 0) {
-        error_setg(errp, QERR_UNSUPPORTED);
-        goto cleanup;
-    }
+    assert(s->note_size >= 0);
 
     /*
      * The goal of this block is to (a) update the previously guessed
-- 
2.39.0

Re: [PATCH 03/12] dump: Assert cpu_get_note_size() can't fail

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> The only way cpu_get_note_size() can return a negative value is
> integer overflow in the non-stub versions, which is a programming
> error.  The stub version is not actually reachable, because the
> cpu_get_dump_info() stub will fail first.  Use assert().  This gets
> rid of another use of QERR_UNSUPPORTED.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH 04/12] hw/core: Improve error message when machine doesn't provide NMIs

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
We've been trying to reduce their remaining use.

Get rid of a use of QERR_UNSUPPORTED, and improve the rather vague
error message

    (qemu) nmi
    Error: this feature or command is not currently supported

to

    Error: machine does not provide NMIs

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/nmi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/core/nmi.c b/hw/core/nmi.c
index 481c4b3c7e..a7bce8a04a 100644
--- a/hw/core/nmi.c
+++ b/hw/core/nmi.c
@@ -22,7 +22,6 @@
 #include "qemu/osdep.h"
 #include "hw/nmi.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/module.h"
 #include "monitor/monitor.h"
 
@@ -70,7 +69,7 @@ void nmi_monitor_handle(int cpu_index, Error **errp)
     if (ns.handled) {
         error_propagate(errp, ns.err);
     } else {
-        error_setg(errp, QERR_UNSUPPORTED);
+        error_setg(errp, "machine does not provide NMIs");
     }
 }
 
-- 
2.39.0

Re: [PATCH 04/12] hw/core: Improve error message when machine doesn't provide NMIs

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> We've been trying to reduce their remaining use.
>
> Get rid of a use of QERR_UNSUPPORTED, and improve the rather vague
> error message
>
>     (qemu) nmi
>     Error: this feature or command is not currently supported
>
> to
>
>     Error: machine does not provide NMIs
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Much better error message.

[PATCH 05/12] hw/smbios: Dumb down smbios_entry_add() stub

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
We've been trying to reduce their remaining use.

smbios_entry_add() is only ever called on behalf of CLI option
-smbios.  Since qemu-options.hx sets @arch_mask to QEMU_ARCH_I386 |
QEMU_ARCH_ARM, it is reachable only for these targets.  Since they
provide a real smbios_entry_add(), the stub is unreachable.

There's no point in unreachable code keeping QERR_UNSUPPORTED alive.
Dumb it down to g_assert_not_reached().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/smbios/smbios-stub.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/smbios/smbios-stub.c b/hw/smbios/smbios-stub.c
index 64e5ba93ec..e8808adfda 100644
--- a/hw/smbios/smbios-stub.c
+++ b/hw/smbios/smbios-stub.c
@@ -21,11 +21,9 @@
  */
 
 #include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "hw/firmware/smbios.h"
 
 void smbios_entry_add(QemuOpts *opts, Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    g_assert_not_reached();
 }
-- 
2.39.0

Re: [PATCH 05/12] hw/smbios: Dumb down smbios_entry_add() stub

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> We've been trying to reduce their remaining use.
>
> smbios_entry_add() is only ever called on behalf of CLI option
> -smbios.  Since qemu-options.hx sets @arch_mask to QEMU_ARCH_I386 |
> QEMU_ARCH_ARM, it is reachable only for these targets.  Since they
> provide a real smbios_entry_add(), the stub is unreachable.
>
> There's no point in unreachable code keeping QERR_UNSUPPORTED alive.
> Dumb it down to g_assert_not_reached().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH 06/12] hw/acpi: Dumb down acpi_table_add() stub

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
We've been trying to reduce their remaining use.

acpi_table_add() is only ever called on behalf of CLI option
-acpitable.  Since qemu-options.hx sets @arch_mask to QEMU_ARCH_I386,
it is reachable only for these targets.  Since they provide a real
acpi_table_add(), the stub is unreachable.

There's no point in unreachable code keeping QERR_UNSUPPORTED alive.
Dumb it down to g_assert_not_reached().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/acpi/acpi-stub.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/acpi/acpi-stub.c b/hw/acpi/acpi-stub.c
index 4c9d081ed4..e268ce9b1a 100644
--- a/hw/acpi/acpi-stub.c
+++ b/hw/acpi/acpi-stub.c
@@ -19,11 +19,9 @@
  */
 
 #include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "qapi/qmp/qerror.h"
 #include "hw/acpi/acpi.h"
 
 void acpi_table_add(const QemuOpts *opts, Error **errp)
 {
-    error_setg(errp, QERR_UNSUPPORTED);
+    g_assert_not_reached();
 }
-- 
2.39.0

Re: [PATCH 06/12] hw/acpi: Dumb down acpi_table_add() stub

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> We've been trying to reduce their remaining use.
>
> acpi_table_add() is only ever called on behalf of CLI option
> -acpitable.  Since qemu-options.hx sets @arch_mask to QEMU_ARCH_I386,
> it is reachable only for these targets.  Since they provide a real
> acpi_table_add(), the stub is unreachable.
>
> There's no point in unreachable code keeping QERR_UNSUPPORTED alive.
> Dumb it down to g_assert_not_reached().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH 07/12] hw/acpi: Move QMP command to hw/core/

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
We've been trying to reduce their remaining use.

qmp_query_vm_generation_id() in stubs/vmgenid.c is the last user of
QERR_UNSUPPORTED outside qga/.  Unlike the stubs we just dropped, it
is actually reachable, namely when CONFIG_ACPI_VMGENID is off.  It
always fails like

    (qemu) info vm-generation-id
    Error: this feature or command is not currently supported

Turns out the real qmp_query_vm_generation_id() doesn't actually
depend on CONFIG_ACPI_VMGENID, and fails safely when it's off.  Move
it to hw/core/machine-qmp-cmds.c, and drop the stub.  The error
message becomes

    Error: VM Generation ID device not found

Feels like an improvement to me.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 MAINTAINERS                |  1 -
 hw/acpi/vmgenid.c          | 18 ------------------
 hw/core/machine-qmp-cmds.c | 18 ++++++++++++++++++
 stubs/vmgenid.c            | 10 ----------
 stubs/meson.build          |  1 -
 5 files changed, 18 insertions(+), 30 deletions(-)
 delete mode 100644 stubs/vmgenid.c

diff --git a/MAINTAINERS b/MAINTAINERS
index fa10ecaeb9..1abdd01661 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2267,7 +2267,6 @@ F: hw/acpi/vmgenid.c
 F: include/hw/acpi/vmgenid.h
 F: docs/specs/vmgenid.txt
 F: tests/qtest/vmgenid-test.c
-F: stubs/vmgenid.c
 
 LED
 M: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index 0c9f158ac9..a39315c1b3 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -12,7 +12,6 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "qapi/qapi-commands-machine.h"
 #include "qemu/module.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/aml-build.h"
@@ -244,20 +243,3 @@ static void vmgenid_register_types(void)
 }
 
 type_init(vmgenid_register_types)
-
-GuidInfo *qmp_query_vm_generation_id(Error **errp)
-{
-    GuidInfo *info;
-    VmGenIdState *vms;
-    Object *obj = find_vmgenid_dev();
-
-    if (!obj) {
-        error_setg(errp, "VM Generation ID device not found");
-        return NULL;
-    }
-    vms = VMGENID(obj);
-
-    info = g_malloc0(sizeof(*info));
-    info->guid = qemu_uuid_unparse_strdup(&vms->guid);
-    return info;
-}
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 44b5da8880..a6ed3a63c3 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -8,6 +8,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/acpi/vmgenid.h"
 #include "hw/boards.h"
 #include "hw/intc/intc.h"
 #include "hw/mem/memory-device.h"
@@ -383,3 +384,20 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
 
     return human_readable_text_from_str(buf);
 }
+
+GuidInfo *qmp_query_vm_generation_id(Error **errp)
+{
+    GuidInfo *info;
+    VmGenIdState *vms;
+    Object *obj = find_vmgenid_dev();
+
+    if (!obj) {
+        error_setg(errp, "VM Generation ID device not found");
+        return NULL;
+    }
+    vms = VMGENID(obj);
+
+    info = g_malloc0(sizeof(*info));
+    info->guid = qemu_uuid_unparse_strdup(&vms->guid);
+    return info;
+}
diff --git a/stubs/vmgenid.c b/stubs/vmgenid.c
deleted file mode 100644
index bfad656c6c..0000000000
--- a/stubs/vmgenid.c
+++ /dev/null
@@ -1,10 +0,0 @@
-#include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "qapi/qapi-commands-machine.h"
-#include "qapi/qmp/qerror.h"
-
-GuidInfo *qmp_query_vm_generation_id(Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
diff --git a/stubs/meson.build b/stubs/meson.build
index 981585cbdf..7657467a5d 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -45,7 +45,6 @@ stub_ss.add(files('target-get-monitor-def.c'))
 stub_ss.add(files('target-monitor-defs.c'))
 stub_ss.add(files('trace-control.c'))
 stub_ss.add(files('uuid.c'))
-stub_ss.add(files('vmgenid.c'))
 stub_ss.add(files('vmstate.c'))
 stub_ss.add(files('vm-stop.c'))
 stub_ss.add(files('win32-kbd-hook.c'))
-- 
2.39.0

Re: [PATCH 07/12] hw/acpi: Move QMP command to hw/core/

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> We've been trying to reduce their remaining use.
>
> qmp_query_vm_generation_id() in stubs/vmgenid.c is the last user of
> QERR_UNSUPPORTED outside qga/.  Unlike the stubs we just dropped, it
> is actually reachable, namely when CONFIG_ACPI_VMGENID is off.  It
> always fails like
>
>     (qemu) info vm-generation-id
>     Error: this feature or command is not currently supported
>
> Turns out the real qmp_query_vm_generation_id() doesn't actually
> depend on CONFIG_ACPI_VMGENID, and fails safely when it's off.  Move
> it to hw/core/machine-qmp-cmds.c, and drop the stub.  The error
> message becomes
>
>     Error: VM Generation ID device not found
>
> Feels like an improvement to me.

Fully agree.

> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH 08/12] qga: Drop dangling reference to QERR_QGA_LOGGING_DISABLED

[Markus Armbruster]

slog()'s function comment advises to use QERR_QGA_LOGGING_DISABLED.
This macro never existed.  The reference got added in commit
e3d4d25206a "guest agent: add guest agent RPCs/commands" along with
QERR_QGA_LOGGING_FAILED, so maybe that one was meant.  However,
QERR_QGA_LOGGING_FAILED was never actually used, and was removed in
commit d73f0beadb5 "qerror.h: Remove unused error classes".

Drop the dangling reference.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/qga/commands.c b/qga/commands.c
index 360077364e..172826f8f8 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -32,9 +32,8 @@
 #define GUEST_FILE_READ_COUNT_MAX (48 * MiB)
 
 /* Note: in some situations, like with the fsfreeze, logging may be
- * temporarilly disabled. if it is necessary that a command be able
- * to log for accounting purposes, check ga_logging_enabled() beforehand,
- * and use the QERR_QGA_LOGGING_DISABLED to generate an error
+ * temporarily disabled. if it is necessary that a command be able
+ * to log for accounting purposes, check ga_logging_enabled() beforehand.
  */
 void slog(const gchar *fmt, ...)
 {
-- 
2.39.0

Re: [PATCH 08/12] qga: Drop dangling reference to QERR_QGA_LOGGING_DISABLED

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> slog()'s function comment advises to use QERR_QGA_LOGGING_DISABLED.
> This macro never existed.  The reference got added in commit
> e3d4d25206a "guest agent: add guest agent RPCs/commands" along with
> QERR_QGA_LOGGING_FAILED, so maybe that one was meant.  However,
> QERR_QGA_LOGGING_FAILED was never actually used, and was removed in
> commit d73f0beadb5 "qerror.h: Remove unused error classes".
>
> Drop the dangling reference.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 08/12] qga: Drop dangling reference to QERR_QGA_LOGGING_DISABLED

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Tue, Feb 7, 2023 at 10:53 AM Juan Quintela <quintela@redhat.com> wrote:

> Markus Armbruster <armbru@redhat.com> wrote:
> > slog()'s function comment advises to use QERR_QGA_LOGGING_DISABLED.
> > This macro never existed.  The reference got added in commit
> > e3d4d25206a "guest agent: add guest agent RPCs/commands" along with
> > QERR_QGA_LOGGING_FAILED, so maybe that one was meant.  However,
> > QERR_QGA_LOGGING_FAILED was never actually used, and was removed in
> > commit d73f0beadb5 "qerror.h: Remove unused error classes".
> >
> > Drop the dangling reference.
> >
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
>

[-- Attachment #2: Type: text/html, Size: 1371 bytes --]

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

[Markus Armbruster]

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/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"),
-- 
2.39.0

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

[Philippe Mathieu-Daudé]

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

>       }
>   }

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

[PATCH 10/12] hw/core: Improve the query-hotpluggable-cpus error message

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
We've been trying to reduce their remaining use.

Get rid of a use of QERR_FEATURE_DISABLED, and improve the slightly
awkward error message

    (qemu) info hotpluggable-cpus
    Error: The feature 'query-hotpluggable-cpus' is not enabled

to

    Error: machine does not support hot-plugging CPUs

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/machine-qmp-cmds.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index a6ed3a63c3..2d904747c0 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -16,7 +16,6 @@
 #include "qapi/error.h"
 #include "qapi/qapi-builtin-visit.h"
 #include "qapi/qapi-commands-machine.h"
-#include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qobject.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/type-helpers.h"
@@ -141,7 +140,7 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
     MachineClass *mc = MACHINE_GET_CLASS(ms);
 
     if (!mc->has_hotpluggable_cpus) {
-        error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
+        error_setg(errp, "machine does not support hot-plugging CPUs");
         return NULL;
     }
 
-- 
2.39.0

Re: [PATCH 10/12] hw/core: Improve the query-hotpluggable-cpus error message

[Philippe Mathieu-Daudé]

On 7/2/23 08:51, Markus Armbruster wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> We've been trying to reduce their remaining use.
> 
> Get rid of a use of QERR_FEATURE_DISABLED, and improve the slightly
> awkward error message
> 
>      (qemu) info hotpluggable-cpus
>      Error: The feature 'query-hotpluggable-cpus' is not enabled
> 
> to
> 
>      Error: machine does not support hot-plugging CPUs
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/core/machine-qmp-cmds.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index a6ed3a63c3..2d904747c0 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -16,7 +16,6 @@
>   #include "qapi/error.h"
>   #include "qapi/qapi-builtin-visit.h"
>   #include "qapi/qapi-commands-machine.h"
> -#include "qapi/qmp/qerror.h"
>   #include "qapi/qmp/qobject.h"
>   #include "qapi/qobject-input-visitor.h"
>   #include "qapi/type-helpers.h"
> @@ -141,7 +140,7 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
>       MachineClass *mc = MACHINE_GET_CLASS(ms);
>   
>       if (!mc->has_hotpluggable_cpus) {
> -        error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
> +        error_setg(errp, "machine does not support hot-plugging CPUs");

"hotplug CPUs" or "hot-pluggable CPUs"?

>           return NULL;
>       }
>   

Re: [PATCH 10/12] hw/core: Improve the query-hotpluggable-cpus error message

[Markus Armbruster]

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

> On 7/2/23 08:51, Markus Armbruster wrote:
>> The QERR_ macros are leftovers from the days of "rich" error objects.
>> We've been trying to reduce their remaining use.
>> Get rid of a use of QERR_FEATURE_DISABLED, and improve the slightly
>> awkward error message
>>      (qemu) info hotpluggable-cpus
>>      Error: The feature 'query-hotpluggable-cpus' is not enabled
>> to
>>      Error: machine does not support hot-plugging CPUs
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   hw/core/machine-qmp-cmds.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
>> index a6ed3a63c3..2d904747c0 100644
>> --- a/hw/core/machine-qmp-cmds.c
>> +++ b/hw/core/machine-qmp-cmds.c
>> @@ -16,7 +16,6 @@
>>   #include "qapi/error.h"
>>   #include "qapi/qapi-builtin-visit.h"
>>   #include "qapi/qapi-commands-machine.h"
>> -#include "qapi/qmp/qerror.h"
>>   #include "qapi/qmp/qobject.h"
>>   #include "qapi/qobject-input-visitor.h"
>>   #include "qapi/type-helpers.h"
>> @@ -141,7 +140,7 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
>>       MachineClass *mc = MACHINE_GET_CLASS(ms);
>>         if (!mc->has_hotpluggable_cpus) {
>> -        error_setg(errp, QERR_FEATURE_DISABLED, "query-hotpluggable-cpus");
>> +        error_setg(errp, "machine does not support hot-plugging CPUs");
>
> "hotplug CPUs" or "hot-pluggable CPUs"?

I think "hotplug CPUs" is strictly worse than "hotpluggable CPUs",
because "hotpluggable" is an adjective, while "hotplug" is not.

I slightly prefer "does not support hot-plugging CPUs" over "does not
support hot-pluggable CPUs".

As always, I'm happy to take advice from native speakers :)

>
>>           return NULL;
>>       }
>>   

[PATCH 11/12] migration/colo: Improve an x-colo-lost-heartbeat error message

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
We've been trying to reduce their remaining use.

Get rid of a use of QERR_FEATURE_DISABLED, and improve the somewhat
imprecise error message

    (qemu) x_colo_lost_heartbeat
    Error: The feature 'colo' is not enabled

to

    Error: VM is not in COLO mode

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/colo-failover.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/colo-failover.c b/migration/colo-failover.c
index 42453481c4..6cb6f90357 100644
--- a/migration/colo-failover.c
+++ b/migration/colo-failover.c
@@ -17,7 +17,6 @@
 #include "migration.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-migration.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/error-report.h"
 #include "trace.h"
 
@@ -78,7 +77,7 @@ FailoverStatus failover_get_state(void)
 void qmp_x_colo_lost_heartbeat(Error **errp)
 {
     if (get_colo_mode() == COLO_MODE_NONE) {
-        error_setg(errp, QERR_FEATURE_DISABLED, "colo");
+        error_setg(errp, "VM is not in COLO mode");
         return;
     }
 
-- 
2.39.0

Re: [PATCH 11/12] migration/colo: Improve an x-colo-lost-heartbeat error message

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> We've been trying to reduce their remaining use.
>
> Get rid of a use of QERR_FEATURE_DISABLED, and improve the somewhat
> imprecise error message
>
>     (qemu) x_colo_lost_heartbeat
>     Error: The feature 'colo' is not enabled
>
> to
>
>     Error: VM is not in COLO mode
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Althought in this case which error message is better is not so clear.
Colo mode is compiled in, but it is not being "enabled".

Later, Juan.

Re: [PATCH 11/12] migration/colo: Improve an x-colo-lost-heartbeat error message

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> The QERR_ macros are leftovers from the days of "rich" error objects.
>> We've been trying to reduce their remaining use.
>>
>> Get rid of a use of QERR_FEATURE_DISABLED, and improve the somewhat
>> imprecise error message
>>
>>     (qemu) x_colo_lost_heartbeat
>>     Error: The feature 'colo' is not enabled
>>
>> to
>>
>>     Error: VM is not in COLO mode
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> Althought in this case which error message is better is not so clear.
> Colo mode is compiled in, but it is not being "enabled".

In my new error message, I tried to convey that it is about the state of
the VM, not the state of the QEMU binary.

[PATCH 12/12] rocker: Tweak stubbed out monitor commands' error messages

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
We've been trying to reduce their remaining use.

The stubbed out Rocker monitor commands are the last remaining users
of QERR_FEATURE_DISABLED.  They fail like this:

    (qemu) info rocker mumble
    Error: The feature 'rocker' is not enabled

The real rocker commands fail like this when the named object doesn't
exist:

    Error: rocker mumble not found

If that's good enough when Rocker is enabled, then it's good enough
when it's disabled, so replace QERR_FEATURE_DISABLED with that, and
drop the macro.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qerror.h    |  3 ---
 hw/net/rocker/qmp-norocker.c | 12 +++++-------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 09006e69f7..8dd9fcb071 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -29,9 +29,6 @@
 #define QERR_DEVICE_NO_HOTPLUG \
     "Device '%s' does not support hotplugging"
 
-#define QERR_FEATURE_DISABLED \
-    "The feature '%s' is not enabled"
-
 #define QERR_INVALID_PARAMETER \
     "Invalid parameter '%s'"
 
diff --git a/hw/net/rocker/qmp-norocker.c b/hw/net/rocker/qmp-norocker.c
index 5ef4f9324c..f6c1196a24 100644
--- a/hw/net/rocker/qmp-norocker.c
+++ b/hw/net/rocker/qmp-norocker.c
@@ -1,6 +1,5 @@
 /*
- * QMP Target options - Commands handled based on a target config
- *                      versus a host config
+ * QMP command stubs
  *
  * Copyright (c) 2015 David Ahern <dsahern@gmail.com>
  *
@@ -18,17 +17,16 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-rocker.h"
-#include "qapi/qmp/qerror.h"
 
 RockerSwitch *qmp_query_rocker(const char *name, Error **errp)
 {
-    error_setg(errp, QERR_FEATURE_DISABLED, "rocker");
+    error_setg(errp, "rocker %s not found", name);
     return NULL;
 };
 
 RockerPortList *qmp_query_rocker_ports(const char *name, Error **errp)
 {
-    error_setg(errp, QERR_FEATURE_DISABLED, "rocker");
+    error_setg(errp, "rocker %s not found", name);
     return NULL;
 };
 
@@ -37,7 +35,7 @@ RockerOfDpaFlowList *qmp_query_rocker_of_dpa_flows(const char *name,
                                                    uint32_t tbl_id,
                                                    Error **errp)
 {
-    error_setg(errp, QERR_FEATURE_DISABLED, "rocker");
+    error_setg(errp, "rocker %s not found", name);
     return NULL;
 };
 
@@ -46,6 +44,6 @@ RockerOfDpaGroupList *qmp_query_rocker_of_dpa_groups(const char *name,
                                                      uint8_t type,
                                                      Error **errp)
 {
-    error_setg(errp, QERR_FEATURE_DISABLED, "rocker");
+    error_setg(errp, "rocker %s not found", name);
     return NULL;
 };
-- 
2.39.0

Re: [PATCH 12/12] rocker: Tweak stubbed out monitor commands' error messages

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> We've been trying to reduce their remaining use.
>
> The stubbed out Rocker monitor commands are the last remaining users
> of QERR_FEATURE_DISABLED.  They fail like this:
>
>     (qemu) info rocker mumble
>     Error: The feature 'rocker' is not enabled
>
> The real rocker commands fail like this when the named object doesn't
> exist:
>
>     Error: rocker mumble not found
>
> If that's good enough when Rocker is enabled, then it's good enough
> when it's disabled, so replace QERR_FEATURE_DISABLED with that, and
> drop the macro.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 00/12] error: Reduce qerror.h usage a bit more

[Philippe Mathieu-Daudé]

On 7/2/23 08:51, Markus Armbruster wrote:
> This series gets rid of two out of 15 remaining QERR_ macros and
> confines use of one more to qga/.  Bonus: better error messages.
> 
> Markus Armbruster (12):
>    error: Drop superfluous #include "qapi/qmp/qerror.h"
>    dump: Improve error message when target doesn't support memory dump
>    dump: Assert cpu_get_note_size() can't fail
>    hw/core: Improve error message when machine doesn't provide NMIs
>    hw/smbios: Dumb down smbios_entry_add() stub
>    hw/acpi: Dumb down acpi_table_add() stub
>    hw/acpi: Move QMP command to hw/core/
>    qga: Drop dangling reference to QERR_QGA_LOGGING_DISABLED
>    replay: Simplify setting replay blockers
>    hw/core: Improve the query-hotpluggable-cpus error message
>    migration/colo: Improve an x-colo-lost-heartbeat error message
>    rocker: Tweak stubbed out monitor commands' error messages

Series:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>