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@xxxxxxxxxx> wrote: > > On Mon, Jun 04, 2012 at 06:44:21PM +0300, Zeeshan Ali (Khattak) wrote: > >> From: "Zeeshan Ali (Khattak)" <zeeshanak@xxxxxxxxx> > >> > >> 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
Attachment:
pgpvmM7x4v98t.pgp
Description: PGP signature
