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

[Michael S. Tsirkin]

On Tue, Aug 27, 2013 at 04:19:39PM -0300, Eduardo Habkost wrote:
> On Tue, Aug 27, 2013 at 07:13:49PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 27, 2013 at 01:09:23PM -0300, Eduardo Habkost wrote:
> > > On Tue, Aug 27, 2013 at 09:51:22AM +0200, Markus Armbruster wrote:
> > > > "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.
> > > 
> > > I just confirmed that on qemu-system-x86_64 the only place where .desc
> > > is used is on the machine_parse() help/error code at vl.c. I don't know
> > > about other architectures.
> > 
> > Generally, one would hope we won't expose random
> > internal stuff to guests :).
> 
> Exactly: we won't expose random internal stuff to guests, but there was
> nothing about the field that made it clear that it was internal.
> 
> In this specific case, somebody could one day suggest adding .desc to
> some SMBIOS field, and the suggestion would even make sense to me.

I'm not sure why, but assuming we do this one day,
we'd need to do this in some way that only affects
new machine types, so we really can't expose
.desc (which old machine types have)
we'd need to expose something new.

> > 
> > > I suggest we explicitly document the field as a human-readable
> > > machine-type description that should be never exposed to the guest (just
> > > in case somebody decides to use .desc on some guest-visible table one
> > > day).
> > 
> > Where would you document this?
> 
> Just above the field declaration on struct QEMUMachine at hw/boards.h.

So it's human readable and I see no reason to expose it to guests,
but I'm still not sure what would be
wrong if some future machine version did exactly that
assuming a compelling reason.


> -- 
> Eduardo