On Mon, 2014-09-22 at 15:43 +0200, Laszlo Ersek wrote: > On 09/22/14 14:50, Marcel Apfelbaum wrote: > > On Mon, 2014-09-22 at 15:36 +0300, Michael S. Tsirkin wrote: > >> On Mon, Sep 22, 2014 at 02:29:08PM +0200, Laszlo Ersek wrote: > >>> On 09/22/14 14:04, Andreas Färber wrote: > >>>> Am 22.09.2014 um 13:26 schrieb Laszlo Ersek: > >>>>> Based on the registration order captured in the previous patch, we > >>>>> sort the ad-hoc list printed for > >>>>> > >>>>> qemu-system-XXXX -M \? > >>>> > >>>> Agree that the order is worth sanitizing. I would however argue that > >>>> registration order is not entirely stable either if you think of > >>>> non-PC cases where there's dozens of source files registering one > >>>> machine each. I would therefore propose alphabetical order as we do > >>>> for QOM'ified CPUs. > >>> > >>> I did mention alphabetical order in the message of patch #1 -- in fact > >>> that was what I implemented first: > >>> > >>>> The first idea to restore ordering is to sort the ad-hoc list in > >>>> machine_parse() by "MachineClass.name". Such a name-based ordering > >>>> would have to be reverse, so that more recent versioned machine types > >>>> appear higher on the list than older versioned machine types (eg. with > >>>> qemu-system-x86_64). However, such a reverse sort wreaks havoc between > >>>> non-versioned machine types (such as those of qemu-system-aarch64). > >>> > >>> Simply put, it looks very ugly. Namely, if you sort it in alphabetically > >>> *ascending* order, then for the x86_64 target, you get: > >>> > >>>> isapc ISA-only PC > >>>> none empty machine > >>>> pc Standard PC (i440FX + PIIX, 1996) (alias of > >>>> pc-i440fx-2.2) > >>>> pc-0.10 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.11 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.12 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.13 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.14 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.15 Standard PC (i440FX + PIIX, 1996) > >>>> pc-1.0 Standard PC (i440FX + PIIX, 1996) > >>>> pc-1.1 Standard PC (i440FX + PIIX, 1996) > >>>> pc-1.2 Standard PC (i440FX + PIIX, 1996) > >>>> pc-1.3 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-1.4 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-1.5 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-1.6 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-1.7 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-2.0 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-2.1 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-2.2 Standard PC (i440FX + PIIX, 1996) (default) > >>>> pc-q35-1.4 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-1.5 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-1.6 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-1.7 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-2.0 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-2.1 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-2.2 Standard PC (Q35 + ICH9, 2009) > >>>> q35 Standard PC (Q35 + ICH9, 2009) (alias of pc-q35-2.2) > >>> > >>> which is very bad / unusual. Okay, so let's sort it in alphabetically > >>> descending order: > >>> > >>>> q35 Standard PC (Q35 + ICH9, 2009) (alias of pc-q35-2.2) > >>>> pc-q35-2.2 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-2.1 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-2.0 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-1.7 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-1.6 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-1.5 Standard PC (Q35 + ICH9, 2009) > >>>> pc-q35-1.4 Standard PC (Q35 + ICH9, 2009) > >>>> pc-i440fx-2.2 Standard PC (i440FX + PIIX, 1996) (default) > >>>> pc-i440fx-2.1 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-2.0 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-1.7 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-1.6 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-1.5 Standard PC (i440FX + PIIX, 1996) > >>>> pc-i440fx-1.4 Standard PC (i440FX + PIIX, 1996) > >>>> pc-1.3 Standard PC (i440FX + PIIX, 1996) > >>>> pc-1.2 Standard PC (i440FX + PIIX, 1996) > >>>> pc-1.1 Standard PC (i440FX + PIIX, 1996) > >>>> pc-1.0 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.15 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.14 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.13 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.12 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.11 Standard PC (i440FX + PIIX, 1996) > >>>> pc-0.10 Standard PC (i440FX + PIIX, 1996) > >>>> pc Standard PC (i440FX + PIIX, 1996) (alias of > >>>> pc-i440fx-2.2) > >>>> none empty machine > >>>> isapc ISA-only PC > >>> > >>> Okay, this is certainly bearable. However, let's see what the reverse > >>> alpha sort does to aarch64: > >>> > >>>> z2 Zipit Z2 (PXA27x) > >>>> xilinx-zynq-a9 Xilinx Zynq Platform Baseboard for Cortex-A9 > >>>> virt ARM Virtual Machine > >>>> vexpress-a9 ARM Versatile Express for Cortex-A9 > >>>> vexpress-a15 ARM Versatile Express for Cortex-A15 > >>>> versatilepb ARM Versatile/PB (ARM926EJ-S) > >>>> versatileab ARM Versatile/AB (ARM926EJ-S) > >>>> verdex Gumstix Verdex (PXA270) > >>>> tosa Tosa PDA (PXA255) > >>>> terrier Terrier PDA (PXA270) > >>>> sx1-v1 Siemens SX1 (OMAP310) V1 > >>>> sx1 Siemens SX1 (OMAP310) V2 > >>>> spitz Spitz PDA (PXA270) > >>>> smdkc210 Samsung SMDKC210 board (Exynos4210) > >>>> realview-pbx-a9 ARM RealView Platform Baseboard Explore for > >>>> Cortex-A9 > >>>> realview-pb-a8 ARM RealView Platform Baseboard for Cortex-A8 > >>>> realview-eb-mpcore ARM RealView Emulation Baseboard (ARM11MPCore) > >>>> realview-eb ARM RealView Emulation Baseboard (ARM926EJ-S) > >>>> nuri Samsung NURI board (Exynos4210) > >>>> none empty machine > >>>> n810 Nokia N810 tablet aka. RX-44 (OMAP2420) > >>>> n800 Nokia N800 tablet aka. RX-34 (OMAP2420) > >>>> musicpal Marvell 88w8618 / MusicPal (ARM926EJ-S) > >>>> midway Calxeda Midway (ECX-2000) > >>>> mainstone Mainstone II (PXA27x) > >>>> lm3s811evb Stellaris LM3S811EVB > >>>> lm3s6965evb Stellaris LM3S6965EVB > >>>> kzm ARM KZM Emulation Baseboard (ARM1136) > >>>> integratorcp ARM Integrator/CP (ARM926EJ-S) > >>>> highbank Calxeda Highbank (ECX-1000) > >>>> cubieboard cubietech cubieboard > >>>> connex Gumstix Connex (PXA255) > >>>> collie Collie PDA (SA-1110) > >>>> cheetah Palm Tungsten|E aka. Cheetah PDA (OMAP310) > >>>> canon-a1100 Canon PowerShot A1100 IS > >>>> borzoi Borzoi PDA (PXA270) > >>>> akita Akita PDA (PXA270) > >>> > >>> Ugh? The reverse sort is painfully obvious, and the user wonders why it > >>> is necessary. Why is z2 at the top, and akita at the bottom? Why is n810 > >>> above n800 and not below it? And so on. > >>> > >>> Instead, the patchset restores the behavior that had been visible before > >>> commit 261747f1: programmer-controlled logical order *within* clusters, > >>> and unspecified order *between* clusters. > >> > >> We could add a cluster name in the API. > >> Use that for sort between clusters only. > >> Hmm? > > Since the machines have now QOM types we can leverage the types > > as "clusters", and each machine hierarchy can have its own > > sorting interface to arrange their descendants however is desired. > > > > It will look like: > > 1. Get all machine types that implement "sort" interface. > > 2. Define a sort order between them (a interface property ?) > > 3. Ask for each of them to sort its descendants. > > > > In this way we have maximum control on the output and > > give each cluster a proper way to decide the ordering > > based on the semantics and not alphabetical/registering order. > > Guys -- no offense meant, but this architecture astronautics & > bikeshedding is exactly why I, in my foresight, haven't assigned the > RHBZ in question to myself, and why I had unsubscribed from qemu-devel > months ago (or at least stopped mail delivery, can't recall exactly). > > Here's the situation. Commit 261747f1 has broken some behavior of qemu > that is technically trivial / irrelevant, but a nuisance for users. > Noone to my knowledge had ever complained about the machtype listing > previously to commit 261747f1. Therefore the pre-261747f1 behavior is > reasonable and sound as a user-visible target to restore. The patchset I > posted is a technically simple solution that behaves identically to qemu > prior to 261747f1. The diffstat of the patchset is "20 insertions". > > In the span of three followups in the thread, we've arrived at an > abstract sort interface, just so the user can see a more or less > traditionally ordered machtype list with -M '?'. > > I think this is completely wrong; both specifically for the issue at > hand -- creeping object orientation -- and in general as a development / > community process -- qemu is now basically untouchable for people who > want (or must) spend their time primarily on different things. > > I agree that projects shouldn't take drive-by patches from irresponsible > contributors. At the same time I don't think I would qualify as one. My > patchset is not one that I throw over the wall. This is the solution > that I consider technically good, and what I have time for. > > I guess it would be easier for me to understand this level of > nit-picking / over-engineering if my patchset ran a high risk of > regressions in some area. Whereas I'm only trying to undo an already > in-tree regression, as non-intrusively as possible > > I only wanted to help out Marcel with RHBZ 1145042, because the breakage > in the visible machtype ordering is a consequence of his earlier patch, > and it seemed easy enough to address. If my patchset is not acceptable > for technical reasons, I can live with that (albeit in disagreement); > Marcel or someone else will hopefully fix the issue then. Hi Laszlo, This was only a suggestion, trying to discuss a possible "cluster" solution, and nothing more. It is not related to the patch submission which I find it reasonable and I completely agree that it solves a regression introduced by my patches.
I'll be more careful in the feature to emphasize clearly when I am discussing the patches and when I am only throwing ideas for others to comment. Thanks, Marcel > > Cheers > Laszlo >
