On Mar 24, 10 18:28:07 +0100, Soeren Sandmann wrote: > Please note that while pixman is not as strict as the X server in who > can push to the repository, it is not a complete free-for-all. > Committing small, obvious patches that fixes typos or oversights is > fine, but don't commit non-obvious stuff without sending it to the > mailing list and giving people time to comment on it.
Ok, just the comment from Jonathan indicated to me that I should commit. > When we are this close to stable release, we need to be even more > careful about what goes in. Patches committed between now and next > week will likely ship without no testing at all. It definitely did help here. So there's at least minimal testing :) > This patch in particular, I don't think shold ship with no testing at > all. So please revert it, and we can consider it again for 0.19.x. I'm fine with that if you consider it problematic. Given that the situation it changes should actually not occur at all, I doubt there's any fallout, though. > - Why are you seeing an assert in the first place? > > The asserts have been disabled since 0.16.4/0.17.6 because pixman > shouldn't take down the X server with asserts for things that would > otherwise be harmless. Apparently we're still using 0.16.0 in SLE11SP1. So that explains. I double checked that there were no related changes in the region code before fixing. However, what happens if the code would have been compiled with -NDEBUG? Is the code path stable with empty regions? If it is, it can be argued that the patch is not necessary, but it could also be argued that the assert() shouldn't have been there in the first place. > If you are using an older version than those in SLES, then you might > want to either upgrade to 0.16.4, or backport No chance. We are version and feature freeze for quite some time now. Time cycles are much different in enterprise products :-/ Especially in SLE11SP1, as changes to SLE11 should be as minimal as possible (and SLE11 is over a year old). > ec6de472d042bec05aaa53f9d14bfbe23edbe01e That sounds more realistic. However, we don't have any other issues with assert()s, and there is a slim chance that this backport introduces additional regressions (asserts with side effects etc.). > - How does the degenerate region get created? Unfortunately I don't know the offending Xserver code by heart. It probably has to be reviewed, I guess there are sleeping dragons there. > - The patch does not look wrong to me - a region with empty extents > should probably be considered empty rather than degenerate. However, > unless this issue is truly causing crashes, changing it is the kind It is definitely causing crashes here, reproducibly, but only on very rare occasions (of which we run into one during installation :o] ) > candidate and a stable release. Did you carefully review all the > code to make sure it always uses PIXMAN_NIL to check for empty > regions? No, but that shouldn't change behavior. Unless a single code snippet uses both PIXMAN_NIL and handcoded tests at the same time. > - Finally, we need much more detail in commit messages than "Fixes > Novell bug xxxxxx". Understood. I thought the code change to be trivial in itself. > Is that even a public bug? I couldn't figure out how to look at > it. I did not create a Novell Login though. Unfortunately, no. Not that I did notice before, oops. You tend to miss those things if you're working with credentials set all the time... It doesn't contain much more information than the backtrace I published, though. Matthias -- Matthias Hopf <[email protected]> __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ [email protected] Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
