qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v3 2/2] smbios: Set system manufacturer, product & version by default
Date: Thu, 31 Oct 2013 06:30:16 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux)

"Michael S. Tsirkin" <address@hidden> writes:

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

Yes, it has value, but you're not going to succeed into blackmailing me
to do that work now.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]