| Message ID | 20180420231246.23130-3-lersek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
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; >> }
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 --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; }
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(-)