[PATCH 0/5] hw/core: Remove uses of obsolete QERR_ definitions

[PATCH 0/5] hw/core: Remove uses of obsolete QERR_ definitions

[Philippe Mathieu-Daudé]

QERR_ definitions are obsolete since 2015... Remove their
uses in hw/core/.

Philippe Mathieu-Daudé (5):
  hw/core: Remove use of QERR_UNSUPPORTED
  hw/core: Remove use of QERR_FEATURE_DISABLED
  hw/core: Remove uses of QERR_DEVICE_NO_HOTPLUG
  hw/core: Remove uses of QERR_PROPERTY_VALUE_BAD
  hw/core: Remove uses of QERR_INVALID_PARAMETER_VALUE

 include/qapi/qmp/qerror.h        | 6 ------
 hw/core/machine-qmp-cmds.c       | 3 ++-
 hw/core/nmi.c                    | 2 +-
 hw/core/qdev-properties-system.c | 2 +-
 hw/core/qdev-properties.c        | 2 +-
 hw/core/qdev.c                   | 3 ++-
 monitor/misc.c                   | 3 +--
 softmmu/cpus.c                   | 3 +--
 softmmu/qdev-monitor.c           | 9 ++++-----
 target/i386/cpu.c                | 2 +-
 10 files changed, 14 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH 1/5] hw/core: Remove use of QERR_UNSUPPORTED

[Philippe Mathieu-Daudé]

QERR_UNSUPPORTED definition is obsolete since 2015 (commit
4629ed1e989, "qerror: Finally unused, clean up"). Replace it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/nmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/nmi.c b/hw/core/nmi.c
index 481c4b3c7e5..b4b4a1ed286 100644
--- a/hw/core/nmi.c
+++ b/hw/core/nmi.c
@@ -70,7 +70,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, "This command is not currently supported");
     }
 }
 
-- 
2.31.1

Re: [PATCH 1/5] hw/core: Remove use of QERR_UNSUPPORTED

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> QERR_UNSUPPORTED definition is obsolete since 2015 (commit
> 4629ed1e989, "qerror: Finally unused, clean up"). Replace it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/core/nmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/core/nmi.c b/hw/core/nmi.c
> index 481c4b3c7e5..b4b4a1ed286 100644
> --- a/hw/core/nmi.c
> +++ b/hw/core/nmi.c
> @@ -70,7 +70,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, "This command is not currently supported");
>      }
>  }

I think this error message doesn't quite fit here.

We error out when the QOM composition tree does not contain an object
providing interface "nmi".  We don't tell the user, though.  This isn't
terrible, because the suitable objects are generally created by board
code, so whether the command works generally depends only on the machine
type.  Still, a bit more detail in the error message wouldn't hurt,
would it?  Say "machine does not support NMIs".

[PATCH 2/5] hw/core: Remove use of QERR_FEATURE_DISABLED

[Philippe Mathieu-Daudé]

QERR_FEATURE_DISABLED definition is obsolete since 2015 (commit
4629ed1e989, "qerror: Finally unused, clean up"). Replace it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/machine-qmp-cmds.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 216fdfaf3a0..f2eff433bf0 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -138,7 +138,8 @@ 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,
+                   "The feature 'query-hotpluggable-cpus' is not enabled");
         return NULL;
     }
 
-- 
2.31.1

Re: [PATCH 2/5] hw/core: Remove use of QERR_FEATURE_DISABLED

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> QERR_FEATURE_DISABLED definition is obsolete since 2015 (commit
> 4629ed1e989, "qerror: Finally unused, clean up"). Replace it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/core/machine-qmp-cmds.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 216fdfaf3a0..f2eff433bf0 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -138,7 +138,8 @@ 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,
> +                   "The feature 'query-hotpluggable-cpus' is not enabled");
>          return NULL;
>      }

Again, the error message could use improvement, say "machine does not
support hot-plugging CPUs".

I can't help to wonder why this is an error, though.  What's the
difference to successfully returning an empty list?  David, you added
the command, can you explain?

By the way:

    ##
    # @query-hotpluggable-cpus:
    #
    # TODO: Better documentation; currently there is none.

The TODO is three years old, and the command four.  Can we resolve it,
please?

[PATCH 3/5] hw/core: Remove uses of QERR_DEVICE_NO_HOTPLUG

[Philippe Mathieu-Daudé]

QERR_DEVICE_NO_HOTPLUG definition is obsolete since 2015 (commit
4629ed1e989, "qerror: Finally unused, clean up"). Replace the two
uses and drop the definition.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qapi/qmp/qerror.h | 3 ---
 hw/core/qdev.c            | 3 ++-
 softmmu/qdev-monitor.c    | 2 +-
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 596fce0c54e..f49ae01cdb0 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -26,9 +26,6 @@
 #define QERR_DEVICE_IN_USE \
     "Device '%s' is in use"
 
-#define QERR_DEVICE_NO_HOTPLUG \
-    "Device '%s' does not support hotplugging"
-
 #define QERR_FEATURE_DISABLED \
     "The feature '%s' is not enabled"
 
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 7f06403752d..14375861c36 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -734,7 +734,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     static int unattached_count;
 
     if (dev->hotplugged && !dc->hotpluggable) {
-        error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
+        error_setg(errp, "Device '%s' does not support hotplugging",
+                   object_get_typename(obj));
         return;
     }
 
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 4851de51a5c..35a885a6623 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -896,7 +896,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
     }
 
     if (!dc->hotpluggable) {
-        error_setg(errp, QERR_DEVICE_NO_HOTPLUG,
+        error_setg(errp, "Device '%s' does not support hotplugging",
                    object_get_typename(OBJECT(dev)));
         return;
     }
-- 
2.31.1

Re: [PATCH 3/5] hw/core: Remove uses of QERR_DEVICE_NO_HOTPLUG

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> QERR_DEVICE_NO_HOTPLUG definition is obsolete since 2015 (commit
> 4629ed1e989, "qerror: Finally unused, clean up"). Replace the two
> uses and drop the definition.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qapi/qmp/qerror.h | 3 ---
>  hw/core/qdev.c            | 3 ++-
>  softmmu/qdev-monitor.c    | 2 +-
>  3 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 596fce0c54e..f49ae01cdb0 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -26,9 +26,6 @@
>  #define QERR_DEVICE_IN_USE \
>      "Device '%s' is in use"
>  
> -#define QERR_DEVICE_NO_HOTPLUG \
> -    "Device '%s' does not support hotplugging"
> -
>  #define QERR_FEATURE_DISABLED \
>      "The feature '%s' is not enabled"
>  
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 7f06403752d..14375861c36 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -734,7 +734,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>      static int unattached_count;
>  
>      if (dev->hotplugged && !dc->hotpluggable) {
> -        error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> +        error_setg(errp, "Device '%s' does not support hotplugging",
> +                   object_get_typename(obj));
>          return;
>      }
>  
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 4851de51a5c..35a885a6623 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -896,7 +896,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>      }
>  
>      if (!dc->hotpluggable) {
> -        error_setg(errp, QERR_DEVICE_NO_HOTPLUG,
> +        error_setg(errp, "Device '%s' does not support hotplugging",
>                     object_get_typename(OBJECT(dev)));
>          return;
>      }

When the same error is detected in multiple places, I like to ask myself
whether the code calls for a refactoring.  But I can't see a useful one
here.

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

Re: [PATCH 3/5] hw/core: Remove uses of QERR_DEVICE_NO_HOTPLUG

[Damien Hedde]

On 11/19/21 09:20, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> QERR_DEVICE_NO_HOTPLUG definition is obsolete since 2015 (commit
>> 4629ed1e989, "qerror: Finally unused, clean up"). Replace the two
>> uses and drop the definition.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   include/qapi/qmp/qerror.h | 3 ---
>>   hw/core/qdev.c            | 3 ++-
>>   softmmu/qdev-monitor.c    | 2 +-
>>   3 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
>> index 596fce0c54e..f49ae01cdb0 100644
>> --- a/include/qapi/qmp/qerror.h
>> +++ b/include/qapi/qmp/qerror.h
>> @@ -26,9 +26,6 @@
>>   #define QERR_DEVICE_IN_USE \
>>       "Device '%s' is in use"
>>   
>> -#define QERR_DEVICE_NO_HOTPLUG \
>> -    "Device '%s' does not support hotplugging"
>> -
>>   #define QERR_FEATURE_DISABLED \
>>       "The feature '%s' is not enabled"
>>   
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 7f06403752d..14375861c36 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -734,7 +734,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>       static int unattached_count;
>>   
>>       if (dev->hotplugged && !dc->hotpluggable) {
>> -        error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
>> +        error_setg(errp, "Device '%s' does not support hotplugging",
>> +                   object_get_typename(obj));
>>           return;
>>       }
>>   
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index 4851de51a5c..35a885a6623 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -896,7 +896,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>>       }
>>   
>>       if (!dc->hotpluggable) {
>> -        error_setg(errp, QERR_DEVICE_NO_HOTPLUG,
>> +        error_setg(errp, "Device '%s' does not support hotplugging",
>>                      object_get_typename(OBJECT(dev)));
>>           return;
>>       }
> 
> When the same error is detected in multiple places, I like to ask myself
> whether the code calls for a refactoring.  But I can't see a useful one
> here.
> 

Since the realize check will catch more case (devices created 
recursively by a command or simply by C code), we could remove the 
softmmu/qdev-monitor.c check and rely on realize catching that just after.
In theory failing during realize should have the same result as "not 
trying to create the device in the first place".

--
Damien

[PATCH 4/5] hw/core: Remove uses of QERR_PROPERTY_VALUE_BAD

[Philippe Mathieu-Daudé]

QERR_PROPERTY_VALUE_BAD definition is obsolete since 2015 (commit
4629ed1e989, "qerror: Finally unused, clean up"). Replace the two
uses and drop the definition.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qapi/qmp/qerror.h | 3 ---
 hw/core/qdev-properties.c | 2 +-
 target/i386/cpu.c         | 2 +-
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index f49ae01cdb0..a3f44fc4a1e 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -50,9 +50,6 @@
 #define QERR_PERMISSION_DENIED \
     "Insufficient permission to perform this operation"
 
-#define QERR_PROPERTY_VALUE_BAD \
-    "Property '%s.%s' doesn't take value '%s'"
-
 #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
     "Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64 ", maximum: %" PRId64 ")"
 
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index c34aac6ebc9..dbea4cf8e5e 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -663,7 +663,7 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, Object *obj,
         break;
     default:
     case -EINVAL:
-        error_setg(errp, QERR_PROPERTY_VALUE_BAD,
+        error_setg(errp, "Property '%s.%s' doesn't take value '%s'",
                    object_get_typename(obj), name, value);
         break;
     case -ENOENT:
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index fc3ed80ef1e..bc63b80e5bd 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4469,7 +4469,7 @@ 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, "Property '.vendor' doesn't take value '%s'", value);
         return;
     }
 
-- 
2.31.1

Re: [PATCH 4/5] hw/core: Remove uses of QERR_PROPERTY_VALUE_BAD

[Damien Hedde]

On 10/30/21 01:01, Philippe Mathieu-Daudé wrote:
> QERR_PROPERTY_VALUE_BAD definition is obsolete since 2015 (commit
> 4629ed1e989, "qerror: Finally unused, clean up"). Replace the two
> uses and drop the definition.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   include/qapi/qmp/qerror.h | 3 ---
>   hw/core/qdev-properties.c | 2 +-
>   target/i386/cpu.c         | 2 +-
>   3 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index f49ae01cdb0..a3f44fc4a1e 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -50,9 +50,6 @@
>   #define QERR_PERMISSION_DENIED \
>       "Insufficient permission to perform this operation"
>   
> -#define QERR_PROPERTY_VALUE_BAD \
> -    "Property '%s.%s' doesn't take value '%s'"
> -
>   #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
>       "Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64 ", maximum: %" PRId64 ")"
>   
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index c34aac6ebc9..dbea4cf8e5e 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -663,7 +663,7 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, Object *obj,
>           break;
>       default:
>       case -EINVAL:
> -        error_setg(errp, QERR_PROPERTY_VALUE_BAD,
> +        error_setg(errp, "Property '%s.%s' doesn't take value '%s'",
>                      object_get_typename(obj), name, value);
>           break;
>       case -ENOENT:
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index fc3ed80ef1e..bc63b80e5bd 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4469,7 +4469,7 @@ 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, "Property '.vendor' doesn't take value '%s'", value);
>           return;
>       }
>   
> 
Hi,

maybe we can remove the '.' before vendor in this case.

--
Damien

Re: [PATCH 4/5] hw/core: Remove uses of QERR_PROPERTY_VALUE_BAD

[Philippe Mathieu-Daudé]

On 11/2/21 10:47, Damien Hedde wrote:
> On 10/30/21 01:01, Philippe Mathieu-Daudé wrote:
>> QERR_PROPERTY_VALUE_BAD definition is obsolete since 2015 (commit
>> 4629ed1e989, "qerror: Finally unused, clean up"). Replace the two
>> uses and drop the definition.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   include/qapi/qmp/qerror.h | 3 ---
>>   hw/core/qdev-properties.c | 2 +-
>>   target/i386/cpu.c         | 2 +-
>>   3 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
>> index f49ae01cdb0..a3f44fc4a1e 100644
>> --- a/include/qapi/qmp/qerror.h
>> +++ b/include/qapi/qmp/qerror.h
>> @@ -50,9 +50,6 @@
>>   #define QERR_PERMISSION_DENIED \
>>       "Insufficient permission to perform this operation"
>>   -#define QERR_PROPERTY_VALUE_BAD \
>> -    "Property '%s.%s' doesn't take value '%s'"
>> -
>>   #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
>>       "Property %s.%s doesn't take value %" PRId64 " (minimum: %"
>> PRId64 ", maximum: %" PRId64 ")"
>>   diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index c34aac6ebc9..dbea4cf8e5e 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -663,7 +663,7 @@ void error_set_from_qdev_prop_error(Error **errp,
>> int ret, Object *obj,
>>           break;
>>       default:
>>       case -EINVAL:
>> -        error_setg(errp, QERR_PROPERTY_VALUE_BAD,
>> +        error_setg(errp, "Property '%s.%s' doesn't take value '%s'",
>>                      object_get_typename(obj), name, value);
>>           break;
>>       case -ENOENT:
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index fc3ed80ef1e..bc63b80e5bd 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -4469,7 +4469,7 @@ 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, "Property '.vendor' doesn't take value
>> '%s'", value);
>>           return;
>>       }
>>  
> Hi,
> 
> maybe we can remove the '.' before vendor in this case.

I think so too but have no clue about this are, so will let
the x86 maintainers decide (I have to respin anyway).

Re: [PATCH 4/5] hw/core: Remove uses of QERR_PROPERTY_VALUE_BAD

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> QERR_PROPERTY_VALUE_BAD definition is obsolete since 2015 (commit
> 4629ed1e989, "qerror: Finally unused, clean up"). Replace the two
> uses and drop the definition.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qapi/qmp/qerror.h | 3 ---
>  hw/core/qdev-properties.c | 2 +-
>  target/i386/cpu.c         | 2 +-
>  3 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index f49ae01cdb0..a3f44fc4a1e 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -50,9 +50,6 @@
>  #define QERR_PERMISSION_DENIED \
>      "Insufficient permission to perform this operation"
>  
> -#define QERR_PROPERTY_VALUE_BAD \
> -    "Property '%s.%s' doesn't take value '%s'"
> -
>  #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
>      "Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64 ", maximum: %" PRId64 ")"
>  
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index c34aac6ebc9..dbea4cf8e5e 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -663,7 +663,7 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, Object *obj,
>          break;
>      default:
>      case -EINVAL:
> -        error_setg(errp, QERR_PROPERTY_VALUE_BAD,
> +        error_setg(errp, "Property '%s.%s' doesn't take value '%s'",
>                     object_get_typename(obj), name, value);
>          break;
>      case -ENOENT:
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index fc3ed80ef1e..bc63b80e5bd 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4469,7 +4469,7 @@ 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, "Property '.vendor' doesn't take value '%s'", value);
>          return;
>      }

We error out unless the string has exactly CPUID_VENDOR_SZ characters.
We don't tell the user, though[*].  We should!

If this patch was long, I'd separate the long & mechanical from the
error message improvement.  Since it isn't, I suggest to make the error
message improvement the patch's subject, and include the removal of
QERR_PROPERTY_VALUE_BAD "while there".  You choose how to structure
this.


[*] This is a common issue with error systems that make new error
messages harder than reusing some existing message.

[PATCH 5/5] hw/core: Remove uses of QERR_INVALID_PARAMETER_VALUE

[Philippe Mathieu-Daudé]

QERR_INVALID_PARAMETER_VALUE definition is obsolete since 2015
(commit 4629ed1e989, "qerror: Finally unused, clean up").
Replace the definitions used in hw/core/.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/qdev-properties-system.c | 2 +-
 monitor/misc.c                   | 3 +--
 softmmu/cpus.c                   | 3 +--
 softmmu/qdev-monitor.c           | 7 +++----
 4 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index a91f60567aa..91b322fe372 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -749,7 +749,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name,
             return;
         }
         if (value < -1 || value > 255) {
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+            error_setg(errp, "Parameter '%s' expects %s",
                        name ? name : "null", "a value between -1 and 255");
             return;
         }
diff --git a/monitor/misc.c b/monitor/misc.c
index ffe79668706..5a33458173e 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -125,8 +125,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     if (has_cpu_index) {
         int ret = monitor_set_cpu(&hmp.common, cpu_index);
         if (ret < 0) {
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
-                       "a CPU number");
+            error_setg(errp, "Parameter 'cpu-index' expects a CPU number");
             goto out;
         }
     }
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 071085f840b..0e7f44154fa 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -741,8 +741,7 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
 
     cpu = qemu_get_cpu(cpu_index);
     if (cpu == NULL) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
-                   "a CPU number");
+        error_setg(errp, "Parameter 'cpu-index' expects a CPU number");
         return;
     }
 
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 35a885a6623..27f3a1248ac 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -241,16 +241,15 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
     }
 
     if (object_class_is_abstract(oc)) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
-                   "a non-abstract device type");
+        error_setg(errp,
+                   "Parameter 'driver' expects a non-abstract device type");
         return NULL;
     }
 
     dc = DEVICE_CLASS(oc);
     if (!dc->user_creatable ||
         (phase_check(PHASE_MACHINE_READY) && !dc->hotpluggable)) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
-                   "a pluggable device type");
+        error_setg(errp, "Parameter 'driver' expects a pluggable device type");
         return NULL;
     }
 
-- 
2.31.1

Re: [PATCH 5/5] hw/core: Remove uses of QERR_INVALID_PARAMETER_VALUE

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> QERR_INVALID_PARAMETER_VALUE definition is obsolete since 2015
> (commit 4629ed1e989, "qerror: Finally unused, clean up").
> Replace the definitions used in hw/core/.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/core/qdev-properties-system.c | 2 +-
>  monitor/misc.c                   | 3 +--
>  softmmu/cpus.c                   | 3 +--
>  softmmu/qdev-monitor.c           | 7 +++----
>  4 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index a91f60567aa..91b322fe372 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -749,7 +749,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name,
>              return;
>          }
>          if (value < -1 || value > 255) {
> -            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +            error_setg(errp, "Parameter '%s' expects %s",
>                         name ? name : "null", "a value between -1 and 255");
>              return;
>          }

Not this patch's problem, but here goes anyway: if @name can be null,
the error message is crap.  If it can't, the code is crap.

> diff --git a/monitor/misc.c b/monitor/misc.c
> index ffe79668706..5a33458173e 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -125,8 +125,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>      if (has_cpu_index) {
>          int ret = monitor_set_cpu(&hmp.common, cpu_index);
>          if (ret < 0) {
> -            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
> -                       "a CPU number");
> +            error_setg(errp, "Parameter 'cpu-index' expects a CPU number");
>              goto out;
>          }
>      }
> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> index 071085f840b..0e7f44154fa 100644
> --- a/softmmu/cpus.c
> +++ b/softmmu/cpus.c
> @@ -741,8 +741,7 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>  
>      cpu = qemu_get_cpu(cpu_index);
>      if (cpu == NULL) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
> -                   "a CPU number");
> +        error_setg(errp, "Parameter 'cpu-index' expects a CPU number");
>          return;
>      }
>  
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 35a885a6623..27f3a1248ac 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -241,16 +241,15 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
>      }
>  
>      if (object_class_is_abstract(oc)) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> -                   "a non-abstract device type");
> +        error_setg(errp,
> +                   "Parameter 'driver' expects a non-abstract device type");
>          return NULL;
>      }
>  
>      dc = DEVICE_CLASS(oc);
>      if (!dc->user_creatable ||
>          (phase_check(PHASE_MACHINE_READY) && !dc->hotpluggable)) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> -                   "a pluggable device type");
> +        error_setg(errp, "Parameter 'driver' expects a pluggable device type");
>          return NULL;
>      }

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

Re: [PATCH 0/5] hw/core: Remove uses of obsolete QERR_ definitions

[Damien Hedde]

On 10/30/21 01:01, Philippe Mathieu-Daudé wrote:
> QERR_ definitions are obsolete since 2015... Remove their
> uses in hw/core/.
> 
> Philippe Mathieu-Daudé (5):
>    hw/core: Remove use of QERR_UNSUPPORTED
>    hw/core: Remove use of QERR_FEATURE_DISABLED
>    hw/core: Remove uses of QERR_DEVICE_NO_HOTPLUG
>    hw/core: Remove uses of QERR_PROPERTY_VALUE_BAD
>    hw/core: Remove uses of QERR_INVALID_PARAMETER_VALUE
> 
>   include/qapi/qmp/qerror.h        | 6 ------
>   hw/core/machine-qmp-cmds.c       | 3 ++-
>   hw/core/nmi.c                    | 2 +-
>   hw/core/qdev-properties-system.c | 2 +-
>   hw/core/qdev-properties.c        | 2 +-
>   hw/core/qdev.c                   | 3 ++-
>   monitor/misc.c                   | 3 +--
>   softmmu/cpus.c                   | 3 +--
>   softmmu/qdev-monitor.c           | 9 ++++-----
>   target/i386/cpu.c                | 2 +-
>   10 files changed, 14 insertions(+), 21 deletions(-)
> 

Hi Philippe,

In my patches you just pulled (patch 9). There is a new 
QERR_INVALID_PARAMETER_VALUE in qdev-monitor.c. Maybe it is worth
fixing it as well.

anyway
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>

Thanks,
Damien

Re: [PATCH 0/5] hw/core: Remove uses of obsolete QERR_ definitions

[Philippe Mathieu-Daudé]

On 11/2/21 10:51, Damien Hedde wrote:
> On 10/30/21 01:01, Philippe Mathieu-Daudé wrote:
>> QERR_ definitions are obsolete since 2015... Remove their
>> uses in hw/core/.
>>
>> Philippe Mathieu-Daudé (5):
>>    hw/core: Remove use of QERR_UNSUPPORTED
>>    hw/core: Remove use of QERR_FEATURE_DISABLED
>>    hw/core: Remove uses of QERR_DEVICE_NO_HOTPLUG
>>    hw/core: Remove uses of QERR_PROPERTY_VALUE_BAD
>>    hw/core: Remove uses of QERR_INVALID_PARAMETER_VALUE
>>
>>   include/qapi/qmp/qerror.h        | 6 ------
>>   hw/core/machine-qmp-cmds.c       | 3 ++-
>>   hw/core/nmi.c                    | 2 +-
>>   hw/core/qdev-properties-system.c | 2 +-
>>   hw/core/qdev-properties.c        | 2 +-
>>   hw/core/qdev.c                   | 3 ++-
>>   monitor/misc.c                   | 3 +--
>>   softmmu/cpus.c                   | 3 +--
>>   softmmu/qdev-monitor.c           | 9 ++++-----
>>   target/i386/cpu.c                | 2 +-
>>   10 files changed, 14 insertions(+), 21 deletions(-)
>>
> 
> Hi Philippe,
> 
> In my patches you just pulled (patch 9). There is a new
> QERR_INVALID_PARAMETER_VALUE in qdev-monitor.c. Maybe it is worth
> fixing it as well.

I didn't want to delay your series and miss the soft freeze for this
cleanup, so I decided to let it go and get it merged after the
release, eventually via QAPI or qemu-trivial tree.

> anyway
> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>

Thanks :)

Re: [PATCH 0/5] hw/core: Remove uses of obsolete QERR_ definitions

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> QERR_ definitions are obsolete since 2015... Remove their
> uses in hw/core/.

Eleven down, 220 to go.  Thanks!