On 04/20/18 11:34, Daniel P. Berrangé wrote: > On Fri, Apr 20, 2018 at 10:11:08AM +0200, Laszlo Ersek wrote: >> On 04/19/18 11:12, Daniel P. Berrangé wrote: >>> On Thu, Apr 19, 2018 at 10:39:32AM +0200, Laszlo Ersek wrote: >>>> On 04/19/18 09:56, Daniel P. Berrangé wrote: >>>>> On Thu, Apr 19, 2018 at 09:48:36AM +0200, Markus Armbruster wrote: >>>>>> Laszlo Ersek <lersek@xxxxxxxxxx> writes: >>>>>> >>>>>>> On 04/18/18 10:47, Markus Armbruster wrote: >>>>>>>> Laszlo Ersek <lersek@xxxxxxxxxx> writes: >>>>>> Replacing CpuInfoArch by such an enum will change the discriminator >>>>>> value from "other" to the real architecture, with the obvious >>>>>> compatibility concerns. But we've accepted similar changes twice >>>>>> already: commit 9d0306dfdfb and commit 25fa194b7b1, both v2.12.0-rc0. >>>>>> >>>>>> "other" was a bad idea. Hindsight 20/20. >>>>>> >>>>>> Getting rid of it in one go rather than piecemeal seems like the least >>>>>> bad way out. Too late for 2.12, though. Eric, what do you think? >>>>> >>>>> Given the context in which this "other" value is used, I think it is >>>>> reasonable to kill it and put a full arch list in there. >>>>> >>>>> No app is likely to be accessing the struct under "other" because it >>>>> is just an empty placeholder. >>>> >>>> Commit 9d0306dfdfb added "s390" and "CpuInfoS390", which I guess had the >>>> potential to confuse QMP clients that didn't expect "s390", but >>>> otherwise it didn't mess with preexistent enum values / structures. >>> >>> NB, qemu-system-s390x would previously have returned "other" in >>> this field, and now it returns "s390". So while it didn't >>> remove "other" from the list of things that could potentially >>> exist, it did change what the s390x binary will actually report. >>> >>>> The same applies to commit 25fa194b7b1, just with "riscv" / >>>> "CpuInfoRISCV" substituted. >>>> >>>> Removing "other" might confuse QMP clients that expect it, except >>>> (according to Daniel) no such client exists, probably. >>> >>> When I say removing "other", I imply that we add an explicit arch >>> for all those which we currently are missing. IOW, all qemu-system-XXX >>> binaries which currently report "other" would change to report their >>> respective "XXX" values. >>> >>> So in this way, it is exactly the same as what we did when we >>> introduced "s390" as an option. >>> >>> The only difference is that once we have every binary reporting the >>> correct arch, we can now also remove "other" from the schema itself >>> as it will then be unused. >> >> Can we please translate this into more actionable items for me, because >> I'm getting confused :) >> >> First, if I add "i386" and "x86_64" to the enum list, we'll have all >> three of "i386", "x86_64" and "x86". Is that useful? How will that work? > > Hmm, yes, on closer look this is a big mess as it is. We've been using > generic terms for covering multiple architectures :-( 'x86' for both > i386 and x86_64, 'sparc' for sparc and sparc64, etc. If we try to fix > that we'll be entering a world of backcompat hurt :-( > > Since your schema is likely to end up just being a file in docs/specs, > rather than directly part of our existnig qapi schema, I suggest we just > ignore whats there. Just define an arch enum in your spec which is right, > and let someone else worry about fixing the mess I can't tell you how much I love this idea. :) Thanks! Laszlo > >> Second, assuming I add constants for the ~10 (?) softmmu arches, can I >> still use @CpuInfoOther as the type for the corresponding new members in >> @CpuInfo? What C code changes will be necessary? > > Yes, we could still use the CpuInfoOther struct, since struct names are > invisible to consumers, but as above, lets ignore the mess > > Regards, > Daniel > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
