On Fri, Aug 25, 2017 at 01:40:07PM +0200, Igor Mammedov wrote: > On Fri, 25 Aug 2017 19:45:38 +1000 > David Gibson wrote: > > > On Fri, Aug 25, 2017 at 09:27:40AM +0200, Igor Mammedov wrote: > > > On Fri, 25 Aug 2017 11:38:19 +1000 > > > David Gibson wrote: > > > > > > > On Thu, Aug 24, 2017 at 10:21:47AM +0200, Igor Mammedov wrote: > > > > > PPC handles -cpu FOO rather incosistently, > > > > > i.e. it does case-insensitive matching of FOO to > > > > > a CPU type (see: ppc_cpu_compare_class_name) but > > > > > handles alias names as case-sensitive, as result: > > > > > > > > > > # qemu-system-ppc64 -M mac99 -cpu g3 > > > > > qemu-system-ppc64: unable to find CPU model ' kN�U' > > > > > > > > > > # qemu-system-ppc64 -cpu 970MP_V1.1 > > > > > qemu-system-ppc64: Unable to find sPAPR CPU Core definition > > > > > > > > > > while > > > > > > > > > > # qemu-system-ppc64 -M mac99 -cpu G3 > > > > > # qemu-system-ppc64 -cpu 970MP_v1.1 > > > > > > > > > > start up just fine. > > > > > > > > > > Considering we can't take case-insensitive matching away, > > > > > make it case-insensitive for all alias/type/core_type > > > > > lookups. > > > > > > > > > > As side effect it allows to remove duplicate core types > > > > > which are the same but use lower-case letters in name. > > > > > > > > > > Signed-off-by: Igor Mammedov > > > > > > > > Hmm. So, I'm certainly good with making the matching more consistent, > > > > and removing extra entries differing only in case. > > > > > > > > However, I don't like capitalizing everything in the model table. The > > > > "canonical" capitalization - as in how it appears in manuals and > > > > marketing info - is quite frequently mixed case. So I think making > > > > everything caps in the table will make it harder to read in the > > > > context of looking at the corresponding documentation. > > > only cpu models => typenames are made caps, the rest > > > > Yes, I know. A number of the CPU model names themselves have mixed > > case. Really. > > > > > incl. description is kept as is in mixed case. > > > It looks like _desc is the thing one would use for > > > 1:1 lookup in spec, while _name isn't direct match consistently > > > (i.e. sometimes it skips spaces and sometimes spaces are replaced by underscore). > > > So keeping _name in mixed case unnecessarily complicates name->type lookup. > > > > > > These mixed case + case-insensitive is what made lookup > > > code over-engineered and overly complex. > > > > I'm fine with making all the lookups case insensitive. I just don't > > want to drop the case on the names in the actual code. > I've tried to find a way but considering that names are statically > 'converted' into typenames and lack of ability to magically > uppercase them with preprocessor, I've gave up on this approach. Ok. > Providing that there is _desc with model name that should be > greppable in spec and complexity of lookup code that mixed-cased > typenames incur, I've opted in favor of simplifying lookup > code at expense uniform typenames (which is inter QEMU thingy). > It is easier to maintain and less chances to make mistake. Yeah, I guess. It just looks weird to have non-standard capsing in the table field. Any chance we could standardize on lowercase rather than upper - for some reason that would seem less odd to me (I guess because C-ish names and identifiers are usually lowercased). > Other targets use exact match for cpu_model so whether > they use mixed casing or not doesn't matter. > > > > As result it evolved (regressed) to inconsistent name handling > > > since it's rather hard to understand what's going on there. > > > > Yeah, I know :/. > > > > > > Can we instead retain mixed case in the table, and capitalize both > > > > sides at the comparison time? > > > it has to be the same-case since PPC has case-insensitive cpu_model > > > matching (sometimes :/). Normalized /same case/ typenames > > > are necessary for object_class_by_name() to work as it does exact match. > > > So it would be possible to replace custom fetching of CPU types > > > and iterating over it with custom comparator with object_class_by_name(). > > > As it's done in 4/6. > > > This patch and 3/6 are only preparatory patches for 4/6. > > > > Ah. Hrm. That's awkward. > It's fine to squash all 3 in 1 patch, but then it becomes huge and > unreview-able, as logic changes are buried within names normalization. Sorry, I meant it's awkward that that object_class_by_name() uses exact and so we'd need a custom iterator to do a case insensitive search. The patch structure is fine. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson