Re: [Qemu-devel] [PATCH] pc: reduce duplication, fix PIIX descriptions

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

> We have a lot of code duplication between machine types,
> this increases with each new machine type
> and each new field.
>
> This has already introduced a minor bug: description
> for pc-1.3 says "Standard PC" while description for
> pc-1.4 is "Standard PC (i440FX + PIIX, 1996)"
> which makes you think 1.3 is somehow more standard,
> or newer, while in fact it's a revision of the same PC.

I wouldn't call it a bug.  We're in the habit of never changing old
machine types, so when we created pc-i440fx-1.4 and pc-q35-1.4 with a
.desc that clearly explains the difference, we didn't change the older
versions the PC machine types as well.

That may have been overly cautious.  As long as .desc is not exposed to
the guest, it's not ABI, thus can be changed.

> This patch addresses this issue by using macros, along
> the lines used by PC_COMPAT_X_X - only for
> non-property options.
>
> Note: this conflicts (trivially) with patch
>     [PATCH v3 6/6] hw: Clean up bogus default boot order
> by Markus. Sent separately for ease of review.  If people like this
> approach, I can rebase on top of that patch.

Appreciated.  I'd very much prefer this patch to be rebased on top of
mine rather than the other way round, because mine got a review from
Laszlo.  He wrote my patch "wasn't easy to verify", so I don't want to
ask him for reviewing v5 without a really good reason.

> The approach can extend to non-PC machine types.

Yes, but since these types aren't versioned, there's less food for
de-duplication.  Interested parties are of course quite welcome to
de-duplicate whatever is there.

> Cc: Markus Armbruster <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  hw/i386/pc_piix.c    | 82 
> ++++++++++++++++++++--------------------------------
>  hw/i386/pc_q35.c     | 24 ++++++++-------
>  include/hw/i386/pc.h |  5 ++++
>  3 files changed, 50 insertions(+), 61 deletions(-)

The patch trades a daisy-chain of macros for a few duplicated lines of
data.  Matter of taste.  Not really my thing, but I don't really mind
either.

If it was my thing, I'd probably change the .desc in 1/2, then introduce
the macro chain in 2/2, so that each commit does one thing only.