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

[Christophe Fergeau]

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.

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

Another issue I can see with the current scheme is that it assumes all arch
will use the same devices. While this is reasonably true for x86/x86_64,
this will probably need to be improved if we are ever to support ARM
distros. An additional attribute on the <device> node would probably do the
trick.

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/20120605/9b1903dc/attachment.sig>