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

[Zeeshan Ali (Khattak)]

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?

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

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

Good point and I think I agree with the solution though we need to
figure how to not change the APIs for this.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124