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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> 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, armbru@redhat.com wrote:
> >> > > From: Markus Armbruster <armbru@redhat.com>
> >> > > 
> >> > > 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 <armbru@redhat.com>
> >> > 
> >> > 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?

> >> As we are past hard freeze, I think this simple patch is better than a
> >> more complex solution for a problem we still don't have (that can still
> >> be implemented in 1.8).
> >
> > I don't see why we need to rush this into 1.7.
> > Downstreams want their info in smbios for support, branding etc,
> > but I don't see a burning need for this in upstream QEMU.
> > It's kind of nice to have it say "QEMU", but we can afford to
> > delay and do it properly for 1.8.
> 
> Define "properly".  I don't see what I'd like to do differently for 1.8.

With proper tests going in first before we start changing things.
Ideally with better separation between user visible and guest visible
interfaces - though if there was a test to catch guest visible changes,
I would be less worried about this lack of separation.

-- 
MST