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@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? 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
Attachment:
pgpcTZmJ3Dcci.pgp
Description: PGP signature
