Re: [libvirt] [Qemu-devel] [qemu RFC v3 2/3] qapi: change the type of TargetInfo.arch from string to enum SysEmuTarget

On 04/23/18 11:50, Markus Armbruster wrote: > Laszlo Ersek <lersek(a)redhat.com&gt; 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(a)redhat.com&gt; >> Cc: David Gibson <dgibson(a)redhat.com&gt; >> Cc: Eric Blake <eblake(a)redhat.com&gt; >> Cc: Gerd Hoffmann <kraxel(a)redhat.com&gt; >> Cc: Kashyap Chamarthy <kchamart(a)redhat.com&gt; >> Cc: Markus Armbruster <armbru(a)redhat.com&gt; >> Cc: Paolo Bonzini <pbonzini(a)redhat.com&gt; >> Cc: Thomas Huth <thuth(a)redhat.com&gt; >> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; >> --- >> >> 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?