Re: [AppDB] Allow admins to remove their admin rights
På Onsdag 04 juni 2008 , 01:24:07 skrev Chris Morgan: What new policy on dissidents? ;-) How does this patch enable admins to remove themselves? I'm afraid I can't see how this change lets an admin remove their own admin status. Chris Yea, that particular piece of code isn't very clear on what it's doing. I found out that $aClean['iUserId'] would only be set when modifying another user, and it previously compared it to $oUser-iUserId before allowing the admin removal. Now we only check for admin rights, and the oUser-iUserId == aClean['iUserId'] check is performed later to see if we should redirect back to the user admin page. On Tue, Jun 3, 2008 at 6:34 AM, Alexander Nicolaysen Sørnes [EMAIL PROTECTED] wrote: This is in accordance with the AppDB's new policy on dissidents. :) Alexander N. Sørnes
Re: mmio: Do not zero current file position whenever mmioSetBuffer is called
On Tue, Jun 03, 2008 at 07:57:00PM -0700, Matthew D'Asaro wrote: Instead I set wm-info.lBufOffset to wm-info.lDiskOffset because the buffer is flushed at the beginning of a call to mmioSetBuffer so wm-info.lDiskOffset is synchronized with wm-info.lBufOffset and so wm-info.lDiskOffset contains the correct current file position. The test for mmio still passes with this patch. wouldn't that indicate, that the current patch does not cover that case? can you add a test for it? -- cu pgpbJjlUNATaz.pgp Description: PGP signature
Re: Time to revert bad commits?
Hello Vitaliy, 2008/6/2 Vitaliy Margolen [EMAIL PROTECTED]: I'm afraid we are out of time to fix bugs that introduces major d3d regressions. I've yet to see a single patch that addressed those issues. It's time to start reverting bad commits and testing the results. The bugs I'm talking about: Bug 10580 Bug 11584 Bug 12794 Bug 13101 In general, you can't revert patches as that may have other side effects. Usually while they can cause regressions they also might make issues reoccur which the patch is trying to fix, blindly reverting is not the answer. Cheers, Maarten.
Re: [AppDB] Allow admins to remove their admin rights
On Wed, Jun 4, 2008 at 4:15 AM, Alexander Nicolaysen Sørnes [EMAIL PROTECTED] wrote: På Onsdag 04 juni 2008 , 01:24:07 skrev Chris Morgan: What new policy on dissidents? ;-) How does this patch enable admins to remove themselves? I'm afraid I can't see how this change lets an admin remove their own admin status. Chris Yea, that particular piece of code isn't very clear on what it's doing. I found out that $aClean['iUserId'] would only be set when modifying another user, and it previously compared it to $oUser-iUserId before allowing the admin removal. Now we only check for admin rights, and the oUser-iUserId == aClean['iUserId'] check is performed later to see if we should redirect back to the user admin page. So if the admin was removing their own admin status $aClean['iUserId'] wouldn't be set? If this is the case we should document it there because otherwise the functionality is confusing. How does the redirect relate to removing admin rights? Chris
Re: [AppDB] Allow admins to remove their admin rights
På Onsdag 04 juni 2008 , 17:37:15 skrev Chris Morgan: On Wed, Jun 4, 2008 at 4:15 AM, Alexander Nicolaysen Sørnes [EMAIL PROTECTED] wrote: På Onsdag 04 juni 2008 , 01:24:07 skrev Chris Morgan: What new policy on dissidents? ;-) How does this patch enable admins to remove themselves? I'm afraid I can't see how this change lets an admin remove their own admin status. Chris Yea, that particular piece of code isn't very clear on what it's doing. I found out that $aClean['iUserId'] would only be set when modifying another user, and it previously compared it to $oUser-iUserId before allowing the admin removal. Now we only check for admin rights, and the oUser-iUserId == aClean['iUserId'] check is performed later to see if we should redirect back to the user admin page. So if the admin was removing their own admin status $aClean['iUserId'] wouldn't be set? If this is the case we should document it there because otherwise the functionality is confusing. How does the redirect relate to removing admin rights? Chris If $aClean['iUserId'] isn't set, it is assumed the user is editing his own preferences. As for the admin right remoal, I probably added it in the wrong place when I first introduced the feature. It hadn't occured to me that anyone would want to do it, I guess :) I can send a follow-up patch to add comments or clarify the code. Alexander
RE: Fixing crashes in Tests (OS version check)
Juan Lang wrote: So, in my opinion, the general principle is this: if the behavior is very different on different Windows versions, and if the behavior on some Windows version is to crash, it seems reasonable to remove the test, as: 1) we want our tests to reflect Windows behavior that applications count on, and different behavior (especially crashing) often implies the applications don't count on a particular behavior, and 2) having regression tests crash is undesirable, as many other valid tests will not be run on a platform. I don't think you can fully argument with this nowadays. There are many applications out there that specifically state to not support pre W2K or sometimes even XP and often refuse to even install on older systems. So in this case they could be perfectly happy with passing a NULL value to that function and since they never have been tested on an older system the developer wouldn't even know that there exists a potential problem. So I would think it's useful to test that the Wine API does not crash on a NULL parameter since newer Windows versions won't do that either. This doesn't mean that we can't make Wine not crash given the bogus input, it just means we may not be able to have a regression test for this behavior. And, of course, there may be specific instances in which we wish to test potentially crashy behavior, but I'd argue that they better be really important to override points 1) and 2). This case seems fairly simple to me. As long as Wine strives to provide support for Win9x and NT4 it needs to make efforts to support both the newer and the older behaviour. For the API itself this means allowing NULL values to be passed without crashing and for the tests it means skipping that particular test for those versions that do crash on it. And while a version test seems the easiest I can also understand why the tests should not have such artificial dependencies especially since these tests would have to distinguish between running on Windows or Wine set to emulate Win9x. But here it seems fairly easy to work around this problem. The fact that certain Snmp APIs are not implemented or possibly return an ERROR_UNIMPLEMENTED status on those platforms should be enough to determine if this test should be skipped. And coming back to this test: I still think it's easier just to remove the test than to come up with a hacky way of avoiding it on a crashy system. No argument with the being easier part. But not writing tests at all would be also easier, but I do not think that is a valid argument to not do them. Typically to do a good test costs more time than an actual implementation of a function and Wine development could be sped up by not writing tests. But it would only implement more functionality with a lot more errors (or should that be less bugs :-). Rolf Kalbermatter
Re: [AppDB] Allow admins to remove their admin rights
On Wed, Jun 4, 2008 at 11:56 AM, Alexander Nicolaysen Sørnes [EMAIL PROTECTED] wrote: På Onsdag 04 juni 2008 , 17:37:15 skrev Chris Morgan: On Wed, Jun 4, 2008 at 4:15 AM, Alexander Nicolaysen Sørnes [EMAIL PROTECTED] wrote: På Onsdag 04 juni 2008 , 01:24:07 skrev Chris Morgan: What new policy on dissidents? ;-) How does this patch enable admins to remove themselves? I'm afraid I can't see how this change lets an admin remove their own admin status. Chris Yea, that particular piece of code isn't very clear on what it's doing. I found out that $aClean['iUserId'] would only be set when modifying another user, and it previously compared it to $oUser-iUserId before allowing the admin removal. Now we only check for admin rights, and the oUser-iUserId == aClean['iUserId'] check is performed later to see if we should redirect back to the user admin page. So if the admin was removing their own admin status $aClean['iUserId'] wouldn't be set? If this is the case we should document it there because otherwise the functionality is confusing. How does the redirect relate to removing admin rights? Chris If $aClean['iUserId'] isn't set, it is assumed the user is editing his own preferences. As for the admin right remoal, I probably added it in the wrong place when I first introduced the feature. It hadn't occured to me that anyone would want to do it, I guess :) I can send a follow-up patch to add comments or clarify the code. A follow up patch would be useful. I don't want to leave that section undocumented if we are making changes to it. Chris
re: [PATCH] oleaut32: Fix valgrind warnings in vatype.c test
Hi Jon, I guess http://www.winehq.org/pipermail/wine-patches/2008-May/055352.html must have been too scary to commit. I independently sent in a fix for the easy part, and it was committed as http://winehq.org/pipermail/wine-cvs/2008-June/043999.html The last part of your patch, having to do with SysAllocStringByteLen(), seems correct to me, offhand. Maybe if you clean up the comment a bit more, sprinkle sugar on it, and resend, it'll make it in this time. It's so gross, though, that it really should serve as a reminder to us to switch to IMalloc... - Dan
