Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
Peter Maydell peter.mayd...@linaro.org writes:
Reformat the qapi-schema TargetType enumeration so that it has just
one target architecture name per line. This allows patches for
adding new targets to just add a single line, rather than having
to reformat most of the list (resulting in a hard-to-check diff).
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
d15a9c23 is an example of what you get otherwise.
I would much prefer it if we autogenerated this list so you didn't
need to change this file at all to add a new target, but Anthony
is against that; so this is at least an improvement.
I don't object to autogenerating it. I object to autogenerating based
on the selected targets.
The enum should be fixed regardless of what the configure line is.
Regards,
Anthony Liguori
qapi-schema.json | 30 ++
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 199744a..a8d361e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3018,10 +3018,32 @@
# Since: 1.2.0
##
{ 'enum': 'TargetType',
- 'data': [ 'alpha', 'arm', 'cris', 'i386', 'lm32', 'm68k', 'microblazeel',
-'microblaze', 'mips64el', 'mips64', 'mipsel', 'mips', 'moxie',
-'or32', 'ppc64', 'ppcemb', 'ppc', 's390x', 'sh4eb', 'sh4',
-'sparc64', 'sparc', 'unicore32', 'x86_64', 'xtensaeb', 'xtensa'
] }
+ 'data': [ 'alpha',
+'arm',
+'cris',
+'i386',
+'lm32',
+'m68k',
+'microblazeel',
+'microblaze',
+'mips64el',
+'mips64',
+'mipsel',
+'mips',
+'moxie',
+'or32',
+'ppc64',
+'ppcemb',
+'ppc',
+'s390x',
+'sh4eb',
+'sh4',
+'sparc64',
+'sparc',
+'unicore32',
+'x86_64',
+'xtensaeb',
+'xtensa' ] }
##
# @TargetInfo:
--
1.7.9.5
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
Paolo Bonzini pbonz...@redhat.com writes:
Il 20/05/2013 18:21, Peter Maydell ha scritto:
Reformat the qapi-schema TargetType enumeration so that it has just
one target architecture name per line. This allows patches for
adding new targets to just add a single line, rather than having
to reformat most of the list (resulting in a hard-to-check diff).
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
d15a9c23 is an example of what you get otherwise.
I would much prefer it if we autogenerated this list so you didn't
need to change this file at all to add a new target, but Anthony
is against that; so this is at least an improvement.
I have queued a patch for 1.6 that would change this field to a free
string. There is no use of this enum, not even for introspection.
I don't object to this, however..
You
don't need to know what targets were supported in the version that you
compiled from. Only one target is supported in this executable
anyway.
It seems useful to me. One day we may support multiple targets per
executable.
There's no obvious place where all of the possible targets are listed so
from the point of view of someone trying to figure out what the
platforms are while writing a management tool, it seems like a useful
thing to have.
We don't add targets very often... are we optimizing for an uncommon
scenario here?
Regards,
Anthony Liguori
Paolo
qapi-schema.json | 30 ++
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 199744a..a8d361e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3018,10 +3018,32 @@
# Since: 1.2.0
##
{ 'enum': 'TargetType',
- 'data': [ 'alpha', 'arm', 'cris', 'i386', 'lm32', 'm68k', 'microblazeel',
-'microblaze', 'mips64el', 'mips64', 'mipsel', 'mips', 'moxie',
-'or32', 'ppc64', 'ppcemb', 'ppc', 's390x', 'sh4eb', 'sh4',
-'sparc64', 'sparc', 'unicore32', 'x86_64', 'xtensaeb', 'xtensa'
] }
+ 'data': [ 'alpha',
+'arm',
+'cris',
+'i386',
+'lm32',
+'m68k',
+'microblazeel',
+'microblaze',
+'mips64el',
+'mips64',
+'mipsel',
+'mips',
+'moxie',
+'or32',
+'ppc64',
+'ppcemb',
+'ppc',
+'s390x',
+'sh4eb',
+'sh4',
+'sparc64',
+'sparc',
+'unicore32',
+'x86_64',
+'xtensaeb',
+'xtensa' ] }
##
# @TargetInfo:
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
On 22 May 2013 14:12, Anthony Liguori aligu...@us.ibm.com wrote: Peter Maydell peter.mayd...@linaro.org writes: I would much prefer it if we autogenerated this list so you didn't need to change this file at all to add a new target, but Anthony is against that; so this is at least an improvement. I don't object to autogenerating it. I object to autogenerating based on the selected targets. I never suggested that -- what I proposed on IRC was that it could be autogenerated based on the contents of default-configs/. But if we're making it not-an-enum anyway the question is moot. thanks -- PMM
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
Am 22.05.2013 15:15, schrieb Anthony Liguori: Paolo Bonzini pbonz...@redhat.com writes: Il 20/05/2013 18:21, Peter Maydell ha scritto: Reformat the qapi-schema TargetType enumeration so that it has just one target architecture name per line. This allows patches for adding new targets to just add a single line, rather than having to reformat most of the list (resulting in a hard-to-check diff). Signed-off-by: Peter Maydell peter.mayd...@linaro.org --- d15a9c23 is an example of what you get otherwise. I would much prefer it if we autogenerated this list so you didn't need to change this file at all to add a new target, but Anthony is against that; so this is at least an improvement. I have queued a patch for 1.6 that would change this field to a free string. There is no use of this enum, not even for introspection. I don't object to this, however.. You don't need to know what targets were supported in the version that you compiled from. Only one target is supported in this executable anyway. It seems useful to me. One day we may support multiple targets per executable. There's no obvious place where all of the possible targets are listed so from the point of view of someone trying to figure out what the platforms are while writing a management tool, it seems like a useful thing to have. Does today's API allow for returning multiple enum values? I doubt so... We don't add targets very often... are we optimizing for an uncommon scenario here? Well, lately every second release or so. More common is however that people start writing a new target and don't submit it yet (ahem!) while another target gets added, and the current form of rebreaking this block of enum values causes more conflicts than with Peter's proposal where the place to add new values is more likely to differ between targets and becomes more secure to add to. So if we don't go for Paolo's string version, Reviewed-by: Andreas Färber afaer...@suse.de One thought that crossed my mind was whether to put [ and ] on lines of their own to aid adding before alpha and after xtensa, but apart from aarch64 I couldn't think of such fringe target names. ;) Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
On 22 May 2013 14:15, Anthony Liguori aligu...@us.ibm.com wrote: Paolo Bonzini pbonz...@redhat.com writes: You don't need to know what targets were supported in the version that you compiled from. Only one target is supported in this executable anyway. It seems useful to me. One day we may support multiple targets per executable. Why would you care about which architectures the executable supports? What you actually want to know is which machine models are supported; whether board foo happens to be ARM or PPC isn't really very interesting IMHO. -- PMM
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
Andreas Färber afaer...@suse.de writes: Am 22.05.2013 15:15, schrieb Anthony Liguori: Paolo Bonzini pbonz...@redhat.com writes: Il 20/05/2013 18:21, Peter Maydell ha scritto: Reformat the qapi-schema TargetType enumeration so that it has just one target architecture name per line. This allows patches for adding new targets to just add a single line, rather than having to reformat most of the list (resulting in a hard-to-check diff). Signed-off-by: Peter Maydell peter.mayd...@linaro.org --- d15a9c23 is an example of what you get otherwise. I would much prefer it if we autogenerated this list so you didn't need to change this file at all to add a new target, but Anthony is against that; so this is at least an improvement. I have queued a patch for 1.6 that would change this field to a free string. There is no use of this enum, not even for introspection. I don't object to this, however.. You don't need to know what targets were supported in the version that you compiled from. Only one target is supported in this executable anyway. It seems useful to me. One day we may support multiple targets per executable. There's no obvious place where all of the possible targets are listed so from the point of view of someone trying to figure out what the platforms are while writing a management tool, it seems like a useful thing to have. Does today's API allow for returning multiple enum values? I doubt so... Nope. We don't add targets very often... are we optimizing for an uncommon scenario here? Well, lately every second release or so. More common is however that people start writing a new target and don't submit it yet (ahem!) while another target gets added, and the current form of rebreaking this block of enum values causes more conflicts Sounds like a good reason to submit the target upstream to me... than with Peter's proposal where the place to add new values is more likely to differ between targets and becomes more secure to add to. There's no need to keep it in sorted order. We should just add stuff to the end of the enum. Heck, if someone wants to send a patch to randomize the sort order, that'd be fine too :-) Regards, Anthony Liguori So if we don't go for Paolo's string version, Reviewed-by: Andreas Färber afaer...@suse.de One thought that crossed my mind was whether to put [ and ] on lines of their own to aid adding before alpha and after xtensa, but apart from aarch64 I couldn't think of such fringe target names. ;) Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
Peter Maydell peter.mayd...@linaro.org writes: On 22 May 2013 14:15, Anthony Liguori aligu...@us.ibm.com wrote: Paolo Bonzini pbonz...@redhat.com writes: You don't need to know what targets were supported in the version that you compiled from. Only one target is supported in this executable anyway. It seems useful to me. One day we may support multiple targets per executable. Why would you care about which architectures the executable supports? What you actually want to know is which machine models are supported; whether board foo happens to be ARM or PPC isn't really very interesting IMHO. That's a very good point. It was the libvirt folks that requested this. Perhaps they can shed some light on the logic? Regards, Anthony Liguori -- PMM
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
Il 22/05/2013 16:29, Anthony Liguori ha scritto: Peter Maydell peter.mayd...@linaro.org writes: On 22 May 2013 14:15, Anthony Liguori aligu...@us.ibm.com wrote: Paolo Bonzini pbonz...@redhat.com writes: You don't need to know what targets were supported in the version that you compiled from. Only one target is supported in this executable anyway. It seems useful to me. One day we may support multiple targets per executable. Why would you care about which architectures the executable supports? What you actually want to know is which machine models are supported; whether board foo happens to be ARM or PPC isn't really very interesting IMHO. That's a very good point. It was the libvirt folks that requested this. Perhaps they can shed some light on the logic? There is processor-dependent logic in libvirt, for example the CPUID bits. Paolo
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
Il 20/05/2013 18:21, Peter Maydell ha scritto:
Reformat the qapi-schema TargetType enumeration so that it has just
one target architecture name per line. This allows patches for
adding new targets to just add a single line, rather than having
to reformat most of the list (resulting in a hard-to-check diff).
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
d15a9c23 is an example of what you get otherwise.
I would much prefer it if we autogenerated this list so you didn't
need to change this file at all to add a new target, but Anthony
is against that; so this is at least an improvement.
I have queued a patch for 1.6 that would change this field to a free
string. There is no use of this enum, not even for introspection. You
don't need to know what targets were supported in the version that you
compiled from. Only one target is supported in this executable anyway.
Paolo
qapi-schema.json | 30 ++
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 199744a..a8d361e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3018,10 +3018,32 @@
# Since: 1.2.0
##
{ 'enum': 'TargetType',
- 'data': [ 'alpha', 'arm', 'cris', 'i386', 'lm32', 'm68k', 'microblazeel',
-'microblaze', 'mips64el', 'mips64', 'mipsel', 'mips', 'moxie',
-'or32', 'ppc64', 'ppcemb', 'ppc', 's390x', 'sh4eb', 'sh4',
-'sparc64', 'sparc', 'unicore32', 'x86_64', 'xtensaeb', 'xtensa'
] }
+ 'data': [ 'alpha',
+'arm',
+'cris',
+'i386',
+'lm32',
+'m68k',
+'microblazeel',
+'microblaze',
+'mips64el',
+'mips64',
+'mipsel',
+'mips',
+'moxie',
+'or32',
+'ppc64',
+'ppcemb',
+'ppc',
+'s390x',
+'sh4eb',
+'sh4',
+'sparc64',
+'sparc',
+'unicore32',
+'x86_64',
+'xtensaeb',
+'xtensa' ] }
##
# @TargetInfo:
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
On 20 May 2013 17:41, Eric Blake ebl...@redhat.com wrote: Yep, that raises (once again) the question of dynamic introspection - there's a difference between the maximum amount of information known at compile time, and the subset of enum values that are actually usable at runtime. This list is static (all known architectures) It's not really static though for practical purposes. The user still has to be able to cope with I know about architecture foo but it's not in the list (if talking to an older qemu) or the list contains architecture bar which I don't know about (if talking to a newer qemu). I think the only thing we really need to avoid is changing the name of an existing architecture? thanks -- PMM
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
On 05/20/2013 10:47 AM, Peter Maydell wrote: On 20 May 2013 17:41, Eric Blake ebl...@redhat.com wrote: Yep, that raises (once again) the question of dynamic introspection - there's a difference between the maximum amount of information known at compile time, and the subset of enum values that are actually usable at runtime. This list is static (all known architectures) It's not really static though for practical purposes. The user still has to be able to cope with I know about architecture foo but it's not in the list (if talking to an older qemu) or the list contains architecture bar which I don't know about (if talking to a newer qemu). I think the only thing we really need to avoid is changing the name of an existing architecture? I'm not talking about keeping the enum static as in unchanging in all future qemu releases, so much as static in that at the time qemu was compiled, this was the list of architectures qemu knew about. But what good does it do to know what other architectures the compile-time qemu knew about, when what we really care about is what does THIS qemu binary emulate. And Paolo raised the point that what we really care about is static-per-binary - that is, each binary supports exactly one architecture (unless you are doing a LOT of work to support multi-arch emulation from a single binary!). Changing the name of an architecture would be reflected by having a different qemu-FOO binary name, right? If that's the case, then introspection of which architectures are supported is done by listing all the matches to qemu-* in the directory where binaries are installed, and we don't need a QMP enum duplicating what the file system already tells us. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Il 20/05/2013 18:57, Eric Blake ha scritto: Changing the name of an architecture would be reflected by having a different qemu-FOO binary name, right? If that's the case, then introspection of which architectures are supported is done by listing all the matches to qemu-* in the directory where binaries are installed, and we don't need a QMP enum duplicating what the file system already tells us. Yes, that was exactly my point (plus, those binaries might well come from different versions of QEMU). Paolo -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.19 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRmlfnAAoJEBvWZb6bTYbyDq0P/AkeSq6btQY4noFbY7WQwN3k 4+uDUjtwvobsuoqvEKYKMCo6MDyJnfNkgX7S6aNSj+v5+59/tlS0evjgj0N76FGj GC2FcDmmqu3OSt9+2i+pU6gs3eSk6UFbZ9/nxI9hzJ5aaPq6vCr/0l/lR1Ru5op6 W6705VDwp13jhgx+P14OfwI3qLztIP6/mL8Mc58J+7Y+GQ2SCs1CPJgUH4EW76Qg uSOZ6FuNan7Hv+aNdUBXeXRteT4AfzwiGeCi2Z5zxvztUJr9YsOx+TKD/6HbO+1l mLyHsOAEba6Ic9N8t/kZLh8yMK44WMcTX7Xnc583NZOz/njBS3+wJqBQLaggj37N FX0w0lYkcbflqIei9YKK5X2Pex8tOQxHB7NDoGgcXQiOtMC8s3CJ1J91PaPrEprm m9s9PXtZ+AXO4gt1cH5sofdPY5NiIChQOjgmp4TFf90NG1+t0DLIqnJaA0BWKEBl uQbdyV3jUyZOdF0hgGRhrt0S1MuOS2e+cCVj1/qNmoAFMkFKEKevInS3QxH2kSNs wkjBHZ9g7LU8LO4qyBCpp4mxepsrKh9kgGnJDkx6cVULC3jkjbSqeORFYblgp/lm 4ZWc95r1m8XkLaIrxuA5DPO1yuHT/bfZiZp1KiwMRVD3vI877y31nUeSK8jdv3Rp iVzfr9t7+uWYWRF3t516 =81Fx -END PGP SIGNATURE-
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
On 20 May 2013 17:35, Paolo Bonzini pbonz...@redhat.com wrote: Il 20/05/2013 18:21, Peter Maydell ha scritto: Reformat the qapi-schema TargetType enumeration I have queued a patch for 1.6 that would change this field to a free string. There is no use of this enum, not even for introspection. You don't need to know what targets were supported in the version that you compiled from. Only one target is supported in this executable anyway. That sounds like a better solution. Do you have a pointer to the patch? I tried a quick list archive search but probably didn't use the right keywords. thanks -- PMM
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
Il 20/05/2013 18:38, Peter Maydell ha scritto: I have queued a patch for 1.6 that would change this field to a free string. There is no use of this enum, not even for introspection. You don't need to know what targets were supported in the version that you compiled from. Only one target is supported in this executable anyway. That sounds like a better solution. Do you have a pointer to the patch? I tried a quick list archive search but probably didn't use the right keywords. No, I haven't quite tested it enough. I'll send an RFC shortly, as soon as compilation finishes. Paolo
Re: [Qemu-devel] [PATCH] qapi-schema.json: Reformat TargetType enum to one-per-line
On 05/20/2013 10:21 AM, Peter Maydell wrote: Reformat the qapi-schema TargetType enumeration so that it has just one target architecture name per line. This allows patches for adding new targets to just add a single line, rather than having to reformat most of the list (resulting in a hard-to-check diff). Signed-off-by: Peter Maydell peter.mayd...@linaro.org --- d15a9c23 is an example of what you get otherwise. I would much prefer it if we autogenerated this list so you didn't need to change this file at all to add a new target, but Anthony is against that; so this is at least an improvement. Yep, that raises (once again) the question of dynamic introspection - there's a difference between the maximum amount of information known at compile time, and the subset of enum values that are actually usable at runtime. This list is static (all known architectures), depending on what solutions we come up with for dynamic introspection, maybe that solution will also have a way for generating enum types. qapi-schema.json | 30 ++ 1 file changed, 26 insertions(+), 4 deletions(-) At any rate, I see no harm in taking this patch as-is now, while we still spend time thinking about introspection. Reviewed-by: Eric Blake ebl...@redhat.com -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
