diff mbox series

[qemu,RFC,v3,2/3] qapi: change the type of TargetInfo.arch from string to enum SysEmuTarget

Message ID 20180420231246.23130-3-lersek@redhat.com
State New
Headers show
Series qapi: add "firmware.json" | expand

Commit Message

Laszlo Ersek April 20, 2018, 11:12 p.m. UTC 
Now that we have @SysEmuTarget, it makes sense to restict
@TargetInfo.@arch to valid sysemu targets at the schema level.

Cc: "Daniel P. Berrange" <berrange@redhat.com>
Cc: David Gibson <dgibson@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Kashyap Chamarthy <kchamart@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    RFCv3:
    
    - The patch is new in this version. [Markus]

 qapi/misc.json |  6 ++++--
 arch_init.c    | 18 +++++++++++++++++-
 2 files changed, 21 insertions(+), 3 deletions(-)

Comments

Markus Armbruster April 23, 2018, 9:50 a.m. UTC | #1 
Laszlo Ersek <lersek@redhat.com> writes:

> Now that we have @SysEmuTarget, it makes sense to restict
> @TargetInfo.@arch to valid sysemu targets at the schema level.

We could mention that supported targets become visible in QMP
introspection.  But I can't see what QMP clients could do with this
information, so let's not bother.

>
> Cc: "Daniel P. Berrange" <berrange@redhat.com>
> Cc: David Gibson <dgibson@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Kashyap Chamarthy <kchamart@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     RFCv3:
>     
>     - The patch is new in this version. [Markus]
>
>  qapi/misc.json |  6 ++++--
>  arch_init.c    | 18 +++++++++++++++++-
>  2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 5636f4a14997..3b4fbc534089 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -5,6 +5,8 @@
>  # = Miscellanea
>  ##
>  
> +{ 'include': 'common.json' }
> +
>  ##
>  # @qmp_capabilities:
>  #
> @@ -2449,12 +2451,12 @@
>  #
>  # Information describing the QEMU target.
>  #
> -# @arch: the target architecture (eg "x86_64", "i386", etc)
> +# @arch: the target architecture
>  #
>  # Since: 1.2.0
>  ##
>  { 'struct': 'TargetInfo',
> -  'data': { 'arch': 'str' } }
> +  'data': { 'arch': 'SysEmuTarget' } }
>  
>  ##
>  # @query-target:
> diff --git a/arch_init.c b/arch_init.c
> index 6ee07478bd11..4367f30291f8 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -29,6 +29,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/audio/soundhw.h"
>  #include "qapi/qapi-commands-misc.h"
> +#include "qapi/error.h"
>  #include "qemu/config-file.h"
>  #include "qemu/error-report.h"
>  #include "hw/acpi/acpi.h"
> @@ -111,8 +112,23 @@ int xen_available(void)
>  TargetInfo *qmp_query_target(Error **errp)
>  {
>      TargetInfo *info = g_malloc0(sizeof(*info));
> +    Error *local_err = NULL;
>  
> -    info->arch = g_strdup(TARGET_NAME);
> +    /*
> +     * The fallback enum value is irrelevant here (TARGET_NAME is a
> +     * macro and can never be NULL), so simply pass zero. Also, the

We pass -1 in similar cases elsewhere.

> +     * lookup should never fail -- if it fails, then @SysEmuTarget needs
> +     * extending. Catch that with an assertion, but also handle it
> +     * gracefully, in case someone adventurous disables assertions.
> +     */
> +    info->arch = qapi_enum_parse(&SysEmuTarget_lookup, TARGET_NAME, 0,
> +                                 &local_err);
> +    g_assert(!local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        qapi_free_TargetInfo(info);
> +        return NULL;
> +    }

Simpler:

       info->arch = qapi_enum_parse(&SysEmuTarget_lookup, TARGET_NAME, 0,
                                    &error_abort);

>  
>      return info;
>  }
Laszlo Ersek April 23, 2018, 6:42 p.m. UTC | #2 
On 04/23/18 11:50, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> Now that we have @SysEmuTarget, it makes sense to restict
>> @TargetInfo.@arch to valid sysemu targets at the schema level.
> 
> We could mention that supported targets become visible in QMP
> introspection.  But I can't see what QMP clients could do with this
> information, so let's not bother.
> 
>>
>> Cc: "Daniel P. Berrange" <berrange@redhat.com>
>> Cc: David Gibson <dgibson@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Kashyap Chamarthy <kchamart@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     RFCv3:
>>     
>>     - The patch is new in this version. [Markus]
>>
>>  qapi/misc.json |  6 ++++--
>>  arch_init.c    | 18 +++++++++++++++++-
>>  2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index 5636f4a14997..3b4fbc534089 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -5,6 +5,8 @@
>>  # = Miscellanea
>>  ##
>>  
>> +{ 'include': 'common.json' }
>> +
>>  ##
>>  # @qmp_capabilities:
>>  #
>> @@ -2449,12 +2451,12 @@
>>  #
>>  # Information describing the QEMU target.
>>  #
>> -# @arch: the target architecture (eg "x86_64", "i386", etc)
>> +# @arch: the target architecture
>>  #
>>  # Since: 1.2.0
>>  ##
>>  { 'struct': 'TargetInfo',
>> -  'data': { 'arch': 'str' } }
>> +  'data': { 'arch': 'SysEmuTarget' } }
>>  
>>  ##
>>  # @query-target:
>> diff --git a/arch_init.c b/arch_init.c
>> index 6ee07478bd11..4367f30291f8 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -29,6 +29,7 @@
>>  #include "hw/pci/pci.h"
>>  #include "hw/audio/soundhw.h"
>>  #include "qapi/qapi-commands-misc.h"
>> +#include "qapi/error.h"
>>  #include "qemu/config-file.h"
>>  #include "qemu/error-report.h"
>>  #include "hw/acpi/acpi.h"
>> @@ -111,8 +112,23 @@ int xen_available(void)
>>  TargetInfo *qmp_query_target(Error **errp)
>>  {
>>      TargetInfo *info = g_malloc0(sizeof(*info));
>> +    Error *local_err = NULL;
>>  
>> -    info->arch = g_strdup(TARGET_NAME);
>> +    /*
>> +     * The fallback enum value is irrelevant here (TARGET_NAME is a
>> +     * macro and can never be NULL), so simply pass zero. Also, the
> 
> We pass -1 in similar cases elsewhere.
> 
>> +     * lookup should never fail -- if it fails, then @SysEmuTarget needs
>> +     * extending. Catch that with an assertion, but also handle it
>> +     * gracefully, in case someone adventurous disables assertions.
>> +     */
>> +    info->arch = qapi_enum_parse(&SysEmuTarget_lookup, TARGET_NAME, 0,
>> +                                 &local_err);
>> +    g_assert(!local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        qapi_free_TargetInfo(info);
>> +        return NULL;
>> +    }
> 
> Simpler:
> 
>        info->arch = qapi_enum_parse(&SysEmuTarget_lookup, TARGET_NAME, 0,
>                                     &error_abort);

OK, I'll use this form -- but should I use -1 or 0, after all, for
default value?

Thanks,
Laszlo

> 
>>  
>>      return info;
>>  }
Markus Armbruster April 24, 2018, 6:09 a.m. UTC | #3 
Laszlo Ersek <lersek@redhat.com> writes:

> On 04/23/18 11:50, Markus Armbruster wrote:
>> Laszlo Ersek <lersek@redhat.com> writes:
>> 
>>> Now that we have @SysEmuTarget, it makes sense to restict
>>> @TargetInfo.@arch to valid sysemu targets at the schema level.
>> 
>> We could mention that supported targets become visible in QMP
>> introspection.  But I can't see what QMP clients could do with this
>> information, so let's not bother.
>> 
>>>
>>> Cc: "Daniel P. Berrange" <berrange@redhat.com>
>>> Cc: David Gibson <dgibson@redhat.com>
>>> Cc: Eric Blake <eblake@redhat.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Kashyap Chamarthy <kchamart@redhat.com>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Thomas Huth <thuth@redhat.com>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>>     RFCv3:
>>>     
>>>     - The patch is new in this version. [Markus]
>>>
>>>  qapi/misc.json |  6 ++++--
>>>  arch_init.c    | 18 +++++++++++++++++-
>>>  2 files changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qapi/misc.json b/qapi/misc.json
>>> index 5636f4a14997..3b4fbc534089 100644
>>> --- a/qapi/misc.json
>>> +++ b/qapi/misc.json
>>> @@ -5,6 +5,8 @@
>>>  # = Miscellanea
>>>  ##
>>>  
>>> +{ 'include': 'common.json' }
>>> +
>>>  ##
>>>  # @qmp_capabilities:
>>>  #
>>> @@ -2449,12 +2451,12 @@
>>>  #
>>>  # Information describing the QEMU target.
>>>  #
>>> -# @arch: the target architecture (eg "x86_64", "i386", etc)
>>> +# @arch: the target architecture
>>>  #
>>>  # Since: 1.2.0
>>>  ##
>>>  { 'struct': 'TargetInfo',
>>> -  'data': { 'arch': 'str' } }
>>> +  'data': { 'arch': 'SysEmuTarget' } }
>>>  
>>>  ##
>>>  # @query-target:
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 6ee07478bd11..4367f30291f8 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -29,6 +29,7 @@
>>>  #include "hw/pci/pci.h"
>>>  #include "hw/audio/soundhw.h"
>>>  #include "qapi/qapi-commands-misc.h"
>>> +#include "qapi/error.h"
>>>  #include "qemu/config-file.h"
>>>  #include "qemu/error-report.h"
>>>  #include "hw/acpi/acpi.h"
>>> @@ -111,8 +112,23 @@ int xen_available(void)
>>>  TargetInfo *qmp_query_target(Error **errp)
>>>  {
>>>      TargetInfo *info = g_malloc0(sizeof(*info));
>>> +    Error *local_err = NULL;
>>>  
>>> -    info->arch = g_strdup(TARGET_NAME);
>>> +    /*
>>> +     * The fallback enum value is irrelevant here (TARGET_NAME is a
>>> +     * macro and can never be NULL), so simply pass zero. Also, the
>> 
>> We pass -1 in similar cases elsewhere.
>> 
>>> +     * lookup should never fail -- if it fails, then @SysEmuTarget needs
>>> +     * extending. Catch that with an assertion, but also handle it
>>> +     * gracefully, in case someone adventurous disables assertions.
>>> +     */
>>> +    info->arch = qapi_enum_parse(&SysEmuTarget_lookup, TARGET_NAME, 0,
>>> +                                 &local_err);
>>> +    g_assert(!local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +        qapi_free_TargetInfo(info);
>>> +        return NULL;
>>> +    }
>> 
>> Simpler:
>> 
>>        info->arch = qapi_enum_parse(&SysEmuTarget_lookup, TARGET_NAME, 0,
>>                                     &error_abort);
>
> OK, I'll use this form -- but should I use -1 or 0, after all, for
> default value?

I count more than a dozen instances of -1, and none of zero.  Please use
-1.
diff mbox series

Patch

diff --git a/qapi/misc.json b/qapi/misc.json
index 5636f4a14997..3b4fbc534089 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -5,6 +5,8 @@ 
 # = Miscellanea
 ##
 
+{ 'include': 'common.json' }
+
 ##
 # @qmp_capabilities:
 #
@@ -2449,12 +2451,12 @@ 
 #
 # Information describing the QEMU target.
 #
-# @arch: the target architecture (eg "x86_64", "i386", etc)
+# @arch: the target architecture
 #
 # Since: 1.2.0
 ##
 { 'struct': 'TargetInfo',
-  'data': { 'arch': 'str' } }
+  'data': { 'arch': 'SysEmuTarget' } }
 
 ##
 # @query-target:
diff --git a/arch_init.c b/arch_init.c
index 6ee07478bd11..4367f30291f8 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -29,6 +29,7 @@ 
 #include "hw/pci/pci.h"
 #include "hw/audio/soundhw.h"
 #include "qapi/qapi-commands-misc.h"
+#include "qapi/error.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "hw/acpi/acpi.h"
@@ -111,8 +112,23 @@  int xen_available(void)
 TargetInfo *qmp_query_target(Error **errp)
 {
     TargetInfo *info = g_malloc0(sizeof(*info));
+    Error *local_err = NULL;
 
-    info->arch = g_strdup(TARGET_NAME);
+    /*
+     * The fallback enum value is irrelevant here (TARGET_NAME is a
+     * macro and can never be NULL), so simply pass zero. Also, the
+     * lookup should never fail -- if it fails, then @SysEmuTarget needs
+     * extending. Catch that with an assertion, but also handle it
+     * gracefully, in case someone adventurous disables assertions.
+     */
+    info->arch = qapi_enum_parse(&SysEmuTarget_lookup, TARGET_NAME, 0,
+                                 &local_err);
+    g_assert(!local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        qapi_free_TargetInfo(info);
+        return NULL;
+    }
 
     return info;
 }