Re: [Qemu-devel] [PATCH v3 2/2] smbios: Set system manufacturer, product & version by default

[Eduardo Habkost]

On Wed, Oct 30, 2013 at 09:22:38PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> 
> > On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <address@hidden> writes:
> >> 
> >> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote:
> >> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote:
> >> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, address@hidden wrote:
> >> >> > > From: Markus Armbruster <address@hidden>
> >> >> > > 
> >> >> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product 
> >> >> > > Bochs,
> >> >> > > no version.  Best SeaBIOS can do, but we can provide better 
> >> >> > > defaults:
> >> >> > > manufacturer QEMU, product & version taken from QEMUMachine desc and
> >> >> > > name.
> >> >> > > 
> >> >> > > Take care to do this only for new machine types, of course.
> >> >> > > 
> >> >> > > Signed-off-by: Markus Armbruster <address@hidden>
> >> >> > 
> >> >> > I feel applying this one would be a mistake.
> >> >> > 
> >> >> > Machine desc is for human readers.
> >> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> >> >> > but if we add a variant with IDE compatibility mode we will
> >> >> > likely want to
> >> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> >> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> >> >> > 2009)".
> >> >> > 
> >> >> > In other words we want the ability to tweak
> >> >> > description retroactively, and exposing it to guest will
> >> >> > break this ability.
> >> >> > 
> >> >> > So we really need a new field not tied to the human description.
> >> >> > 
> >> >> 
> >> >> You have a point, but if we do that one day, then we can add a new
> >> >> smbios-specific field and set it for each of the existing machine-types
> >> >> so they keep the same ABI. This patch doesn't make us unable to do that
> >> >> in the future.
> >> >
> >> > We'll likely forget and just break guest ABI.
> >> > So we really need a unit test for this, too.
> >> 
> >> More tests are good, but we I think we got bigger fish to fry than
> >> writing tests to catch changes that are so obviously foolish as messing
> >> with old machine type's QEMUMachine.
> >
> > You "messed with" old machine type's QEMUMachine in your cleanup
> > patches too, didn't you?
> 
> I've occasionally touched QEMUMachine initializers in cleanup series,
> but nothing as frivolous as changing strings.  And I can't find anything
> as frivolous as that in git.  We *are* careful and conservative there.

Actually, we changed .desc for old machine types before. See commit
a0dba644c139907ccf6735c505fbd254010d6938.

-- 
Eduardo