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

[Michael S. Tsirkin]

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.
> 
> >> >> 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.
> 
> You want me to test for unlikely developer mistakes that are far easier
> to catch in review than most other guest ABI changes, and far less
> harmful than pretty much any other guest ABI change.  This would
> multiply the size of this mini-series by a significant factor.  I can't
> justify this in good conscience to my (and your) employer.  So this
> isn't going to happen.
>
> If the maintainers agree with you, then I wasted my time.  Sad, but I'd
> rather write off the work I've already done than do much more work of no
> particular value just to save it.

It would be of no particular value *if we only test these strings*.
But testing smbios generally has a lot of value IMHO.

-- 
MST