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

[Markus Armbruster]

"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.