[virt-tools-list] [libosinfo] Fix osinfo_list_add_union()

[Christophe Fergeau]

On Tue, Jun 05, 2012 at 05:36:38PM +0300, Zeeshan Ali (Khattak) wrote:
> On Tue, Jun 5, 2012 at 10:54 AM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > On Mon, Jun 04, 2012 at 08:38:14PM +0300, Zeeshan Ali (Khattak) wrote:
> >> On Mon, Jun 4, 2012 at 7:26 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> >> > On Mon, Jun 04, 2012 at 06:44:21PM +0300, Zeeshan Ali (Khattak) wrote:
> >> >> From: "Zeeshan Ali (Khattak)" <zeeshanak at gnome.org>
> >> >>
> >> >> This function was adding the second list elements to new list first so
> >> >> the order of elements in the new list was contrary to what user will
> >> >> expect of this function (and all wrapper/using functions).
> >> >
> >> > This change makes sense to me, but since you looked at this code to fix a
> >> > Boxes bug, you should explain what behaviour you were seeing without this
> >> > patch, what this patch change, and why it's better now (or something in
> >> > that spirit ;)
> >>
> >> If the change makes sense anyways, I don't see the need to waste time
> >> on justifying it.
> >
> > The very fact that I needed to first guess and then explain where this
> > patch is coming from in the paragraph below should be proof enough that
> > explaining why you had to look into that code was required to achieve a
> > proper review of said code.
> > By doing that, you're not only helping the reviewers, but also your future
> > self, if you find a bug in this code 6 months from now, having a log
> > explaining why you made this change a long time ago can be a tremendous
> > help to make sure you are not introducing regressions.
> > That said, after the discussion below saying that it's not an actual fix
> > for the bug you were seeing, this explanation is less needed in this patch,
> > but will be useful in the patch with the true fix.
> 
> So this patch can go in already?

Well, it would be bad if this patch went in, and became a good enough work
around for Boxes, and if the root cause never gets fixed...

> 
> >>
> >> > As I understand it, sound was not working in Windows 7 because the list of
> >> > audio devices contained 2 elements, ich6 and ac97. Boxes arbitrarily picked
> >> > the first element in the list which is ac97, but this one is not working.
> >> > With your change, the list order is changed and ich6 is first. Since it's
> >> > the more specific device (it's specified on the 'win7' node in
> >> > windows.xml), this ordering makes sense even though it's not specified
> >> > anywehere I could find in the documentation.
> >> >
> >> > However, the bigger question (if the above explanation is what you were
> >> > seeing) is why was a non-working audio device present in the device list
> >> > returned by libosinfo?
> >>
> >> Good point! I don't know how to fix this in a non-intrusive and nice
> >> way as win7 does inherit from vista and vista from NT. This actually
> >> brings us to a more generic problem: How do we deal with device
> >> support being dropped in a later version of the OS? Perhaps we could
> >> introduce a new node/API for this:
> >>
> >> <dropped-devices>
> >>       <device id="http://pciids.sourceforge.net/v2.2/pci.ids/8086/2415"/>
> >> <!-- AC97 -->
> >> </dropped-devices>
> >
> >
> > I'm wondering if devices should be inherited at all? There are no
> > guarantees that from one Windows version to the next, older hardware
> > support won't get dropped. Currently Windows 7 inherits all the way back to
> > win95 and picks its supported devices from there, we are lucky that there
> > are few dropped devices in the mean time ;)
> 
> Inheritance of devices (actually anything) is mostly meant to avoid
> lots of duplication. Note that only 'inherits-from' and 'clones'
> relationships imply inheritance of devices, not 'upgrades' so if we
> don't have much to gain from device inheritance in some cases, we
> could simply remove 'inherits-from' and 'clones' relationships.
> 
> > Something that may work is to decide that a given OS can only have one
> > device for a given PROP_CLASS, but I'm not sure how realistic of an
> > assumptoin this is.
> 
> That sounds very artificial. How about we add a 'obsoletes' attribute
> to device nodes? So ICH8 entry in win7 would just obsolete the
> inherited AC97 entry.

Seems artificial too ;) No great idea though :-/

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20120606/f7834b58/attachment.sig>