November 2009 - Wine-Devel - list.winehq.org

Re: advapi32.dll: Reduce Registry access to MAXIMUM_ALLOWED wherever possible

by Rob Shearman

2009/11/27 Paul Chitescu <paulc(a)voip.null.ro>: > Changelog: > advapi32.dll: Reduce Registry access to MAXIMUM_ALLOWED wherever possible > > Makes read operation succeed on read/only Registry keys. Needs tests to show that this behaviour matches that on Windows. You should be able to achieve this by setting an appropriate security descriptor on the registry key before calling the function you are changing. However, the change to get_special_root_hkey looks fine to me. The rest of your patch isn't easy to review because you haven't generated the diff with the "-p" option. -- Rob Shearman

Nov. 28, 2009

Re: Wine programs - replicating bad design/bugs

by Jerome Leclanche

The issue with notepad is, the search menu is completely different in Windows notepad (AFAIK it's in Edit and there's no search menu). Fair enough for 18622. J. Leclanche / Adys On Sat, Nov 28, 2009 at 6:20 PM, Vitaliy Margolen <wine-devel(a)kievinfo.com> wrote: > Jerome Leclanche wrote: >> http://bugs.winehq.org/show_bug.cgi?id=20844 > IMHO this could be done, however this will be a "hidden feature". You can't > add that to the menu - it will break programs that "script" notepad (yes > there are programs like that). > >> ** http://bugs.winehq.org/show_bug.cgi?id=18622 > This error coming from API layer, not regedit. Windows' registry API is > case-insensitive and does not allow that. As well as wineserver handles > registry as case-insensitive. Altering API means breaking programs. > > Vitaliy. > >

Nov. 28, 2009

Re: Corrections for signal handlers

by Markus Elfring

> And don't ask me to actually think about what I'm doing, I am still open for further discussions. > it's too hard to figure out how to print a string and a number without using printf(). This data display (e. g. for the input command and line number) is generally not a problem. But the situation is different in the context of a signal handler because of the limited set of async-signal-safe functions. Regards, Markus

Nov. 28, 2009

Re: Corrections for signal handlers

by Vitaliy Margolen

Don't CC wine-patches. Markus Elfring wrote: >> Original printed more useful information. >> Just dropping it in sake of "correctness" is wrong. >> You'll have to come up with something that prints the same >> or don't touch it. > > I am aware of the desire to get some data for easier program debugging conveniently. > I have got an opposite opinion. I am curious if more software developers would like to care for complete async-signal-safety. > This will not happen at the moment because of conflicting expectations. > I assume that the involved implementation details will be reconsidered some day in the future. So basically you saying: ---------- I don't care what you had in there, and I don't care how useful that information was to you. I care about some highly hypothetical error cases that might never happen in your program but just because theoretically they can happen I'll remove some code that looks wrong. And don't ask me to actually think about what I'm doing, it's too hard to figure out how to print a string and a number without using printf(). So I'll leave that to you to code. ----------- No wonder you receive such a "welcome" from other projects you've tried to "improve" before. Vitaliy.

Nov. 28, 2009

Re: Corrections for signal handlers

by Markus Elfring

> Original printed more useful information. I am aware of the desire to get some data for easier program debugging conveniently. > Just dropping it in sake of "correctness" is wrong. I have got an opposite opinion. I am curious if more software developers would like to care for complete async-signal-safety. > You'll have to come up with something that prints the same This will not happen at the moment because of conflicting expectations. > or don't touch it. I assume that the involved implementation details will be reconsidered some day in the future. Regards, Markus

Nov. 28, 2009

Re: Corrections for signal handlers

by Vitaliy Margolen

Markus Elfring wrote: > Hello, > > Some signal handler implementations are also affected by technical details that are described in an article by Justin Pincar. > https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+… > Your code looks even worse then your arguments for "correct" codding practices. > -static int watchdog; > +static sig_atomic_t watchdog; Don't do this, you've been told to leave it alone. > - fprintf( stderr, "wineserver crashed, please enable coredumps (ulimit -c unlimited) and restart.\n"); > + char const message[] = "wineserver crashed, please enable coredumps (ulimit -c unlimited) and restart.\n"; Should be "static const char" in that exact order. > + > + (void) write(STDERR_FILENO, message, sizeof(message) - 1); What's "(void)" doing here? Yes we do ignore returned states here and in 10000s or other places. Don't do that. > + (void) fsync(STDERR_FILENO); Why do you need sync here? > abort(); > --- a/tools/wmc/wmc.c Wine fix per program please. You'll have to split this into 3 separate patches. > static void segvhandler(int sig) > { > -<----->fprintf(stderr, "\n%s:%d: Oops, segment violation\n", input_name, line_number); > -<----->fflush(stdout); > -<----->fflush(stderr); > + char const message[] = "\nOops, segment violation\n"; Same as above - should be "static const char" in that exact order. > + > + (void) write(STDERR_FILENO, message, sizeof(message) - 1); > + (void) fsync(STDOUT_FILENO); > + (void) fsync(STDERR_FILENO); Remove bogus "(void)". > <----->abort(); > } Original printed more useful information. Just dropping it in sake of "correctness" is wrong. You'll have to come up with something that prints the same or don't touch it. Original used tabs (8-spaces) your patch uses 4-space indentation. Either keep the file's formatting or stick to 4-space indentation but DO NOT mix and match both inside one function. Same applies to tools/wrc/wrc.c Vitaliy.

Nov. 28, 2009

Re: Wine programs - replicating bad design/bugs

by Vitaliy Margolen

Jerome Leclanche wrote: > http://bugs.winehq.org/show_bug.cgi?id=20844 IMHO this could be done, however this will be a "hidden feature". You can't add that to the menu - it will break programs that "script" notepad (yes there are programs like that). > ** http://bugs.winehq.org/show_bug.cgi?id=18622 This error coming from API layer, not regedit. Windows' registry API is case-insensitive and does not allow that. As well as wineserver handles registry as case-insensitive. Altering API means breaking programs. Vitaliy.

Nov. 28, 2009

re: Corrections for signal handlers

by Dan Kegel

I'm not sure this change does what you think: - fflush(stdout); ... + (void) fsync(STDOUT_FILENO); The real problem with your change is that *we don't care* whether the output of wineserver is garbled, because it is low priority debugging information, and I've never seen it garbled in practice. In short, it's not worth the time reviewing the patch. So while we appreciate your effort to improve the correctness of Wine's code, we would prefer it if you chose a more useful target. Also, Wine's patch review system is reputation-based. Your best avenue for getting pedantic corrections accepted is to get some pragmatic improvements accepted first. You might then be able to use some of your earned karma points on your favorite bikeshed problem. - Dan

Nov. 28, 2009

Re: Whitespace cleanups

by Jerome Leclanche

I was just working on shell32/shlview.c: ShellView_OnNotify. There is so much mixing between tabs, spaces, different indent levels, trailing whitespace and what not that the code is properly unreadable. git-blame is one thing, but when it goes as far as almost preventing a developer to make changes... J. Leclanche / Adys On Fri, Nov 27, 2009 at 7:27 AM, Ken Thomases <ken(a)codeweavers.com> wrote: > On Nov 26, 2009, at 10:15 AM, Francois Gouget wrote: > >> On Wed, 25 Nov 2009, Ken Thomases wrote: >> [...] >>> >>> One important reason to avoid whitespace-only changes is it makes >>> git-blame essentially useless for finding the real source of >>> functional changes >> >> Such changes don't make git-blame useless. They just make it a bit >> harder to use. >> >> Let's say your 'git-blame dlls/mshtml/tests/dom.c' leads you to commit >> 1e4412d7 which, by looking at it, you decide is a false positive. Then >> you do: >> >> $ git cat-file -p 1e4412d7 >> tree ea3b2e117b383ade8afc7cc9bf534c48faf891c7 >> parent 44f520a5b6f2bd6ec6da8793177711e57bda9c72 >> author Francois Gouget <fgouget(a)free.fr> 1258539107 +0100 >> committer Alexandre Julliard <julliard(a)winehq.org> 1258553736 +0100 >> >> mshtml/tests: Add a trailing '\n' to ok() calls. >> >> Then you run git-blame again: >> >> git blame 44f520a5 dlls/mshtml/tests/dom.c >> >> And continue the analysis until you find the right culprit. > > Um, it's just easier to add a caret ('^') to the commit ID. > > git blame 1e4412d7^ dlls/mshtml/tests/dom.c > > I'm well aware of this technique, but if it doesn't make git-blame useless, > it makes it a hell of a lot less useful, especially if whitespace-only > changes become non-rare. I wasn't aware of and haven't tried Kai's > suggestion of using the -w switch. > > -Ken > > > >

Nov. 28, 2009

Wine programs - replicating bad design/bugs

by Jerome Leclanche

Ok, posting here to get a broader feedback on the issue. Recently, another bug* was closed about an enhancement to a wine program - a minor feature not present on Windows. This is an issue I've seen brought up before, and which I've brought up myself**. As far as the win32 api goes, I can totally agree on dysfunctions that need to be replicated. However I find this unnecessary for the wine programs (notepad, wordpad, regedit, any or more). The idea behind bug 20844 was to have a better integration with the desktop, where Shift-F3 is a common keybind for "Search previous...". The only problem with this kind of mindset is "where should we stop with new features?". IMHO, the issue behind the two bugs linked below is only a matter of better integration, better design. I can also understand how some developers' goal is to close bugs, sometimes disregarding the fact there is a problem to deal with before then. * http://bugs.winehq.org/show_bug.cgi?id=20844 ** http://bugs.winehq.org/show_bug.cgi?id=18622 J. Leclanche PS. In bug 18622, I'm not talking about a case-insensitive registry, but for the key being actually renamed.

Nov. 28, 2009