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