[PATCH 0/7] Miscellaneous error message improvements

[PATCH 0/7] Miscellaneous error message improvements

[Markus Armbruster]

Markus Armbruster (7):
  spapr/pci: Correct "does not support hotplugging error messages
  hmp: Improve sync-profile error message
  qga: Improve guest-exec-status error message
  ui/qmp-cmds: Improve two error messages
  net: Fix a misleading error message
  balloon: Fix a misleading error message
  target/i386/cpu: Improve error message for property "vendor"

 hw/ppc/spapr_pci.c |  4 ++--
 monitor/hmp-cmds.c |  4 ++--
 net/net.c          |  6 +++---
 qga/commands.c     |  2 +-
 system/balloon.c   | 10 +++++-----
 target/i386/cpu.c  |  3 ++-
 ui/ui-qmp-cmds.c   |  5 +++--
 7 files changed, 18 insertions(+), 16 deletions(-)

-- 
2.41.0

[PATCH 1/7] spapr/pci: Correct "does not support hotplugging error messages

[Markus Armbruster]

When dynamic-reconfiguration is off, hot plug / unplug can fail with
"Bus 'spapr-pci-host-bridge' does not support hotplugging".
spapr-pci-host-bridge is a device, not a bus.  Report the name of the
bus it provides instead: 'pci.0'.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/spapr_pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 370c5a90f2..ebb32ad90b 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1551,7 +1551,7 @@ static void spapr_pci_pre_plug(HotplugHandler *plug_handler,
          */
         if (plugged_dev->hotplugged) {
             error_setg(errp, QERR_BUS_NO_HOTPLUG,
-                       object_get_typename(OBJECT(phb)));
+                       phb->parent_obj.bus->qbus.name);
             return;
         }
     }
@@ -1672,7 +1672,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
 
     if (!phb->dr_enabled) {
         error_setg(errp, QERR_BUS_NO_HOTPLUG,
-                   object_get_typename(OBJECT(phb)));
+                   phb->parent_obj.bus->qbus.name);
         return;
     }
 
-- 
2.41.0

Re: [PATCH 1/7] spapr/pci: Correct "does not support hotplugging error messages

[Daniel Henrique Barboza]

On 10/31/23 08:10, Markus Armbruster wrote:
> When dynamic-reconfiguration is off, hot plug / unplug can fail with
> "Bus 'spapr-pci-host-bridge' does not support hotplugging".
> spapr-pci-host-bridge is a device, not a bus.  Report the name of the
> bus it provides instead: 'pci.0'.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Feel free to queue it up. Thanks,


Daniel

>   hw/ppc/spapr_pci.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 370c5a90f2..ebb32ad90b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1551,7 +1551,7 @@ static void spapr_pci_pre_plug(HotplugHandler *plug_handler,
>            */
>           if (plugged_dev->hotplugged) {
>               error_setg(errp, QERR_BUS_NO_HOTPLUG,
> -                       object_get_typename(OBJECT(phb)));
> +                       phb->parent_obj.bus->qbus.name);
>               return;
>           }
>       }
> @@ -1672,7 +1672,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
>   
>       if (!phb->dr_enabled) {
>           error_setg(errp, QERR_BUS_NO_HOTPLUG,
> -                   object_get_typename(OBJECT(phb)));
> +                   phb->parent_obj.bus->qbus.name);
>           return;
>       }
>   

Re: [PATCH 1/7] spapr/pci: Correct "does not support hotplugging error messages

[BALATON Zoltan]

On Wed, 1 Nov 2023, Daniel Henrique Barboza wrote:
> On 10/31/23 08:10, Markus Armbruster wrote:
>> When dynamic-reconfiguration is off, hot plug / unplug can fail with
>> "Bus 'spapr-pci-host-bridge' does not support hotplugging".
>> spapr-pci-host-bridge is a device, not a bus.  Report the name of the
>> bus it provides instead: 'pci.0'.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>
> Feel free to queue it up. Thanks,
>
>
> Daniel
>
>>   hw/ppc/spapr_pci.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 370c5a90f2..ebb32ad90b 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -1551,7 +1551,7 @@ static void spapr_pci_pre_plug(HotplugHandler 
>> *plug_handler,
>>            */
>>           if (plugged_dev->hotplugged) {
>>               error_setg(errp, QERR_BUS_NO_HOTPLUG,
>> -                       object_get_typename(OBJECT(phb)));
>> +                       phb->parent_obj.bus->qbus.name);

I could not find it mentioned in the docs but it was said the parent 
pointer is private and one should not access it but cast to the parent 
object instead. Or here may even use pci_get_bus(pdev) maybe after moving 
the asserts before it to make sure the device is valid. But I don't mind 
so you can commit it as it is if nobody notices.

Regards,
BALATON Zoltan

>>               return;
>>           }
>>       }
>> @@ -1672,7 +1672,7 @@ static void spapr_pci_unplug_request(HotplugHandler 
>> *plug_handler,
>>         if (!phb->dr_enabled) {
>>           error_setg(errp, QERR_BUS_NO_HOTPLUG,
>> -                   object_get_typename(OBJECT(phb)));
>> +                   phb->parent_obj.bus->qbus.name);
>>           return;
>>       }
>> 
>
>

Re: [PATCH 1/7] spapr/pci: Correct "does not support hotplugging error messages

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Wed, 1 Nov 2023, Daniel Henrique Barboza wrote:
>> On 10/31/23 08:10, Markus Armbruster wrote:
>>> When dynamic-reconfiguration is off, hot plug / unplug can fail with
>>> "Bus 'spapr-pci-host-bridge' does not support hotplugging".
>>> spapr-pci-host-bridge is a device, not a bus.  Report the name of the
>>> bus it provides instead: 'pci.0'.
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>
>> Feel free to queue it up. Thanks,
>>
>>
>> Daniel
>>
>>>   hw/ppc/spapr_pci.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 370c5a90f2..ebb32ad90b 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -1551,7 +1551,7 @@ static void spapr_pci_pre_plug(HotplugHandler *plug_handler,
>>>            */
>>>           if (plugged_dev->hotplugged) {
>>>               error_setg(errp, QERR_BUS_NO_HOTPLUG,
>>> -                       object_get_typename(OBJECT(phb)));
>>> +                       phb->parent_obj.bus->qbus.name);
>
> I could not find it mentioned in the docs but it was said the parent pointer is private and one should not access it but cast to the parent object instead. Or here may even use pci_get_bus(pdev) maybe after moving the asserts before it to make sure the device is valid. But I don't mind so you can commit it as it is if nobody notices.

pci_get_bus() returns the bus the device is plugged into as a PCI bus.
We need the bus the device provides.  Besides, @phb is plugged into the
main system bus.  pci_get_bus(PCI_DEVICE(phb)) would pass the main
system bus to PCI_BUS(), which is not good.

I can offer

                            PCI_HOST_BRIDGE(phb)->bus->qbus.name);

Looks like a wash to me, but if maintainers like it better, I'll change
to it.

>
> Regards,
> BALATON Zoltan
>
>>>               return;
>>>           }
>>>       }
>>> @@ -1672,7 +1672,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
>>>         if (!phb->dr_enabled) {
>>>           error_setg(errp, QERR_BUS_NO_HOTPLUG,
>>> -                   object_get_typename(OBJECT(phb)));
>>> +                   phb->parent_obj.bus->qbus.name);
>>>           return;
>>>       }
>>> 
>>
>>

[PATCH 2/7] hmp: Improve sync-profile error message

[Markus Armbruster]

Improve

    (qemu) sync-profile of
    Error: Invalid parameter 'of'

to

    Error: invalid parameter 'of', expecting 'on', 'off', or 'reset'

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/hmp-cmds.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 6c559b48c8..871898ac46 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -24,7 +24,6 @@
 #include "qapi/qapi-commands-control.h"
 #include "qapi/qapi-commands-misc.h"
 #include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/cutils.h"
 #include "hw/intc/intc.h"
 #include "qemu/log.h"
@@ -138,7 +137,8 @@ void hmp_sync_profile(Monitor *mon, const QDict *qdict)
     } else {
         Error *err = NULL;
 
-        error_setg(&err, QERR_INVALID_PARAMETER, op);
+        error_setg(&err, "invalid parameter '%s',"
+                   " expecting 'on', 'off', or 'reset'", op);
         hmp_handle_error(mon, err);
     }
 }
-- 
2.41.0

Re: [PATCH 2/7] hmp: Improve sync-profile error message

[Philippe Mathieu-Daudé]

On 31/10/23 12:10, Markus Armbruster wrote:
> Improve
> 
>      (qemu) sync-profile of
>      Error: Invalid parameter 'of'
> 
> to
> 
>      Error: invalid parameter 'of', expecting 'on', 'off', or 'reset'
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   monitor/hmp-cmds.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

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

Re: [PATCH 2/7] hmp: Improve sync-profile error message

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Improve
> 
>     (qemu) sync-profile of
>     Error: Invalid parameter 'of'
> 
> to
> 
>     Error: invalid parameter 'of', expecting 'on', 'off', or 'reset'
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

> ---
>  monitor/hmp-cmds.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 6c559b48c8..871898ac46 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -24,7 +24,6 @@
>  #include "qapi/qapi-commands-control.h"
>  #include "qapi/qapi-commands-misc.h"
>  #include "qapi/qmp/qdict.h"
> -#include "qapi/qmp/qerror.h"
>  #include "qemu/cutils.h"
>  #include "hw/intc/intc.h"
>  #include "qemu/log.h"
> @@ -138,7 +137,8 @@ void hmp_sync_profile(Monitor *mon, const QDict *qdict)
>      } else {
>          Error *err = NULL;
>  
> -        error_setg(&err, QERR_INVALID_PARAMETER, op);
> +        error_setg(&err, "invalid parameter '%s',"
> +                   " expecting 'on', 'off', or 'reset'", op);
>          hmp_handle_error(mon, err);
>      }
>  }
> -- 
> 2.41.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

[PATCH 3/7] qga: Improve guest-exec-status error message

[Markus Armbruster]

When the PID passed to guest-exec-status does not exist, we report

    "Invalid parameter 'pid'"

Improve this to

    "PID 1234 does not exist"

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

diff --git a/qga/commands.c b/qga/commands.c
index ce172edd2d..88c1c99fe5 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -154,7 +154,7 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **errp)
 
     gei = guest_exec_info_find(pid);
     if (gei == NULL) {
-        error_setg(errp, QERR_INVALID_PARAMETER, "pid");
+        error_setg(errp, "PID " PRId64 " does not exist");
         return NULL;
     }
 
-- 
2.41.0

Re: [PATCH 3/7] qga: Improve guest-exec-status error message

[Konstantin Kostiuk]

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

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

On Tue, Oct 31, 2023 at 1:11 PM Markus Armbruster <armbru@redhat.com> wrote:

> When the PID passed to guest-exec-status does not exist, we report
>
>     "Invalid parameter 'pid'"
>
> Improve this to
>
>     "PID 1234 does not exist"
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qga/commands.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qga/commands.c b/qga/commands.c
> index ce172edd2d..88c1c99fe5 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -154,7 +154,7 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid,
> Error **errp)
>
>      gei = guest_exec_info_find(pid);
>      if (gei == NULL) {
> -        error_setg(errp, QERR_INVALID_PARAMETER, "pid");
> +        error_setg(errp, "PID " PRId64 " does not exist");
>          return NULL;
>      }
>
> --
> 2.41.0
>
>

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

Re: [PATCH 3/7] qga: Improve guest-exec-status error message

[Philippe Mathieu-Daudé]

On 31/10/23 12:10, Markus Armbruster wrote:
> When the PID passed to guest-exec-status does not exist, we report
> 
>      "Invalid parameter 'pid'"
> 
> Improve this to
> 
>      "PID 1234 does not exist"
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/commands.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

[PATCH 4/7] ui/qmp-cmds: Improve two error messages

[Markus Armbruster]

set_password with "protocol": "vnc" supports only "connected": "keep".
Any other value is rejected with

    Invalid parameter 'connected'

Improve this to

    parameter 'connected' must be 'keep' when 'protocol' is 'vnc'

client_migrate_info requires "port" or "tls-port".  When both are
missing, it fails with

    Parameter 'port/tls-port' is missing

Improve this to

    parameter 'port' or 'tls-port' is required

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

diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
index debc07d678..8880691f92 100644
--- a/ui/ui-qmp-cmds.c
+++ b/ui/ui-qmp-cmds.c
@@ -44,7 +44,8 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
         assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
         if (opts->connected != SET_PASSWORD_ACTION_KEEP) {
             /* vnc supports "connected=keep" only */
-            error_setg(errp, QERR_INVALID_PARAMETER, "connected");
+            error_setg(errp, "parameter 'connected' must be 'keep'"
+                       " when 'protocol' is 'vnc'");
             return;
         }
         /*
@@ -195,7 +196,7 @@ void qmp_client_migrate_info(const char *protocol, const char *hostname,
         }
 
         if (!has_port && !has_tls_port) {
-            error_setg(errp, QERR_MISSING_PARAMETER, "port/tls-port");
+            error_setg(errp, "parameter 'port' or 'tls-port' is required");
             return;
         }
 
-- 
2.41.0

Re: [PATCH 4/7] ui/qmp-cmds: Improve two error messages

[Philippe Mathieu-Daudé]

On 31/10/23 12:10, Markus Armbruster wrote:
> set_password with "protocol": "vnc" supports only "connected": "keep".
> Any other value is rejected with
> 
>      Invalid parameter 'connected'
> 
> Improve this to
> 
>      parameter 'connected' must be 'keep' when 'protocol' is 'vnc'
> 
> client_migrate_info requires "port" or "tls-port".  When both are
> missing, it fails with
> 
>      Parameter 'port/tls-port' is missing
> 
> Improve this to
> 
>      parameter 'port' or 'tls-port' is required
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   ui/ui-qmp-cmds.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

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

[PATCH 5/7] net: Fix a misleading error message

[Markus Armbruster]

The error message

    $ qemu-system-x86_64 -netdev user,id=net0,ipv6-net=fec0::0/
    qemu-system-x86_64: -netdev user,id=net0,ipv6-net=fec0::0/: Parameter 'ipv6-prefixlen' expects a number

points to ipv6-prefixlen instead of ipv6-net.  Fix:

    qemu-system-x86_64: -netdev user,id=net0,ipv6-net=fec0::0/: parameter 'ipv6-net' expects a number after '/'

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

diff --git a/net/net.c b/net/net.c
index c0c0cbe99e..8e67a20abc 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1227,7 +1227,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
     int ret = -1;
     Visitor *v = opts_visitor_new(opts);
 
-    /* Parse convenience option format ip6-net=fec0::0[/64] */
+    /* Parse convenience option format ipv6-net=fec0::0[/64] */
     const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
 
     if (ip6_net) {
@@ -1247,8 +1247,8 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
         if (substrings[1] &&
             qemu_strtoul(substrings[1], NULL, 10, &prefix_len))
         {
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       "ipv6-prefixlen", "a number");
+            error_setg(errp,
+                       "parameter 'ipv6-net' expects a number after '/'");
             goto out;
         }
 
-- 
2.41.0

[PATCH 6/7] balloon: Fix a misleading error message

[Markus Armbruster]

The error message

    {"execute": "balloon", "arguments":{"value": -1}}
    {"error": {"class": "GenericError", "desc": "Parameter 'target' expects a size"}}

points to 'target' instead of 'value'.  Fix:

    {"error": {"class": "GenericError", "desc": "Parameter 'value' expects a size"}}

Root cause: qmp_balloon()'s parameter is named @target.  Rename it to
@value to match the QAPI schema.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 system/balloon.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/system/balloon.c b/system/balloon.c
index e0e8969a4b..fda7af832e 100644
--- a/system/balloon.c
+++ b/system/balloon.c
@@ -90,17 +90,17 @@ BalloonInfo *qmp_query_balloon(Error **errp)
     return info;
 }
 
-void qmp_balloon(int64_t target, Error **errp)
+void qmp_balloon(int64_t value, Error **errp)
 {
     if (!have_balloon(errp)) {
         return;
     }
 
-    if (target <= 0) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "target", "a size");
+    if (value <= 0) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "value", "a size");
         return;
     }
 
-    trace_balloon_event(balloon_opaque, target);
-    balloon_event_fn(balloon_opaque, target);
+    trace_balloon_event(balloon_opaque, value);
+    balloon_event_fn(balloon_opaque, value);
 }
-- 
2.41.0

Re: [PATCH 6/7] balloon: Fix a misleading error message

[David Hildenbrand]

On 31.10.23 12:10, Markus Armbruster wrote:
> The error message
> 
>      {"execute": "balloon", "arguments":{"value": -1}}
>      {"error": {"class": "GenericError", "desc": "Parameter 'target' expects a size"}}
> 
> points to 'target' instead of 'value'.  Fix:
> 
>      {"error": {"class": "GenericError", "desc": "Parameter 'value' expects a size"}}
> 
> Root cause: qmp_balloon()'s parameter is named @target.  Rename it to
> @value to match the QAPI schema.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   system/balloon.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/system/balloon.c b/system/balloon.c
> index e0e8969a4b..fda7af832e 100644
> --- a/system/balloon.c
> +++ b/system/balloon.c
> @@ -90,17 +90,17 @@ BalloonInfo *qmp_query_balloon(Error **errp)
>       return info;
>   }
>   
> -void qmp_balloon(int64_t target, Error **errp)
> +void qmp_balloon(int64_t value, Error **errp)
>   {
>       if (!have_balloon(errp)) {
>           return;
>       }
>   
> -    if (target <= 0) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "target", "a size");
> +    if (value <= 0) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "value", "a size");
>           return;
>       }
>   
> -    trace_balloon_event(balloon_opaque, target);
> -    balloon_event_fn(balloon_opaque, target);
> +    trace_balloon_event(balloon_opaque, value);
> +    balloon_event_fn(balloon_opaque, value);
>   }

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 6/7] balloon: Fix a misleading error message

[Michael S. Tsirkin]

On Tue, Oct 31, 2023 at 12:10:58PM +0100, Markus Armbruster wrote:
> The error message
> 
>     {"execute": "balloon", "arguments":{"value": -1}}
>     {"error": {"class": "GenericError", "desc": "Parameter 'target' expects a size"}}
> 
> points to 'target' instead of 'value'.  Fix:
> 
>     {"error": {"class": "GenericError", "desc": "Parameter 'value' expects a size"}}
> 
> Root cause: qmp_balloon()'s parameter is named @target.  Rename it to
> @value to match the QAPI schema.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

feel free to merge

> ---
>  system/balloon.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/system/balloon.c b/system/balloon.c
> index e0e8969a4b..fda7af832e 100644
> --- a/system/balloon.c
> +++ b/system/balloon.c
> @@ -90,17 +90,17 @@ BalloonInfo *qmp_query_balloon(Error **errp)
>      return info;
>  }
>  
> -void qmp_balloon(int64_t target, Error **errp)
> +void qmp_balloon(int64_t value, Error **errp)
>  {
>      if (!have_balloon(errp)) {
>          return;
>      }
>  
> -    if (target <= 0) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "target", "a size");
> +    if (value <= 0) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "value", "a size");
>          return;
>      }
>  
> -    trace_balloon_event(balloon_opaque, target);
> -    balloon_event_fn(balloon_opaque, target);
> +    trace_balloon_event(balloon_opaque, value);
> +    balloon_event_fn(balloon_opaque, value);
>  }
> -- 
> 2.41.0

Re: [PATCH 6/7] balloon: Fix a misleading error message

[Mario Casquero]

This patch has been successfully tested by QE. Start a VM with a
virtio-balloon device and resize it to an invalid value. Check in the
expected error message that now makes reference to 'value' instead of
'target'.

Tested-by: Mario Casquero <mcasquer@redhat.com>

On Tue, Oct 31, 2023 at 12:12 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> The error message
>
>     {"execute": "balloon", "arguments":{"value": -1}}
>     {"error": {"class": "GenericError", "desc": "Parameter 'target' expects a size"}}
>
> points to 'target' instead of 'value'.  Fix:
>
>     {"error": {"class": "GenericError", "desc": "Parameter 'value' expects a size"}}
>
> Root cause: qmp_balloon()'s parameter is named @target.  Rename it to
> @value to match the QAPI schema.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  system/balloon.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/system/balloon.c b/system/balloon.c
> index e0e8969a4b..fda7af832e 100644
> --- a/system/balloon.c
> +++ b/system/balloon.c
> @@ -90,17 +90,17 @@ BalloonInfo *qmp_query_balloon(Error **errp)
>      return info;
>  }
>
> -void qmp_balloon(int64_t target, Error **errp)
> +void qmp_balloon(int64_t value, Error **errp)
>  {
>      if (!have_balloon(errp)) {
>          return;
>      }
>
> -    if (target <= 0) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "target", "a size");
> +    if (value <= 0) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "value", "a size");
>          return;
>      }
>
> -    trace_balloon_event(balloon_opaque, target);
> -    balloon_event_fn(balloon_opaque, target);
> +    trace_balloon_event(balloon_opaque, value);
> +    balloon_event_fn(balloon_opaque, value);
>  }
> --
> 2.41.0
>
>

[PATCH 7/7] target/i386/cpu: Improve error message for property "vendor"

[Markus Armbruster]

Improve

    $ qemu-system-x86_64 -device max-x86_64-cpu,vendor=me
    qemu-system-x86_64: -device max-x86_64-cpu,vendor=me: Property '.vendor' doesn't take value 'me'

to

    qemu-system-x86_64: -device max-x86_64-cpu,vendor=0123456789abc: value of property 'vendor' must consist of excactly 12 characters

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index fc8484cb5e..e708628c16 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5192,7 +5192,8 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
     int i;
 
     if (strlen(value) != CPUID_VENDOR_SZ) {
-        error_setg(errp, QERR_PROPERTY_VALUE_BAD, "", "vendor", value);
+        error_setg(errp, "value of property 'vendor' must consist of"
+                   " excactly " stringify(CPUID_VENDOR_SZ) " characters");
         return;
     }
 
-- 
2.41.0

Re: [PATCH 7/7] target/i386/cpu: Improve error message for property "vendor"

[Philippe Mathieu-Daudé]

On 31/10/23 12:10, Markus Armbruster wrote:
> Improve
> 
>      $ qemu-system-x86_64 -device max-x86_64-cpu,vendor=me
>      qemu-system-x86_64: -device max-x86_64-cpu,vendor=me: Property '.vendor' doesn't take value 'me'
> 
> to
> 
>      qemu-system-x86_64: -device max-x86_64-cpu,vendor=0123456789abc: value of property 'vendor' must consist of excactly 12 characters
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   target/i386/cpu.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index fc8484cb5e..e708628c16 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5192,7 +5192,8 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>       int i;
>   
>       if (strlen(value) != CPUID_VENDOR_SZ) {
> -        error_setg(errp, QERR_PROPERTY_VALUE_BAD, "", "vendor", value);
> +        error_setg(errp, "value of property 'vendor' must consist of"
> +                   " excactly " stringify(CPUID_VENDOR_SZ) " characters");

Typo "exactly" (and in example), otherwise:

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

>           return;
>       }
>   

Re: [PATCH 7/7] target/i386/cpu: Improve error message for property "vendor"

[Markus Armbruster]

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

> On 31/10/23 12:10, Markus Armbruster wrote:
>> Improve
>>      $ qemu-system-x86_64 -device max-x86_64-cpu,vendor=me
>>      qemu-system-x86_64: -device max-x86_64-cpu,vendor=me: Property '.vendor' doesn't take value 'me'
>> to
>>      qemu-system-x86_64: -device max-x86_64-cpu,vendor=0123456789abc: value of property 'vendor' must consist of excactly 12 characters
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   target/i386/cpu.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index fc8484cb5e..e708628c16 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -5192,7 +5192,8 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>>       int i;
>>         if (strlen(value) != CPUID_VENDOR_SZ) {
>> -        error_setg(errp, QERR_PROPERTY_VALUE_BAD, "", "vendor", value);
>> +        error_setg(errp, "value of property 'vendor' must consist of"
>> +                   " excactly " stringify(CPUID_VENDOR_SZ) " characters");
>
> Typo "exactly" (and in example), otherwise:

Fixed.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>>           return;
>>       }
>>   

Thanks!

Re: [PATCH 0/7] Miscellaneous error message improvements

[Markus Armbruster]

Queued.