On Tue, Jun 5, 2012 at 10:54 AM, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@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. 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
