Re: dlls/ntdll/file.c: Use FIXME_ONCE for quieter reports.
Hello Max, are you replacing all FIXME with FIXME_ONCE? While I do agree that we need the _ONCE variant changing all FIXME is not useful. bye michael On 07/14/2010 09:02 PM, Max TenEyck Woodbury wrote:
--- dlls/ntdll/file.c | 40 ++++++++++++++++++---------------------- 1 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index 0a6ee55..0b59765 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -117,7 +117,7 @@ static NTSTATUS FILE_CreateFile( PHANDLE handle, ACCESS_MASK access, POBJECT_ATT
if (!attr || !attr->ObjectName) return STATUS_INVALID_PARAMETER;
- if (alloc_size) FIXME( "alloc_size not supported\n" ); + if (alloc_size) FIXME_ONCE( "alloc_size not supported\n" );
if (options& FILE_OPEN_BY_FILE_ID) io->u.Status = file_id_to_unix_file_name( attr,&unix_name ); @@ -1484,7 +1484,7 @@ NTSTATUS WINAPI NtFsControlFile(HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc break;
case FSCTL_PIPE_IMPERSONATE: - FIXME("FSCTL_PIPE_IMPERSONATE: impersonating self\n"); + FIXME_ONCE("FSCTL_PIPE_IMPERSONATE: impersonating self\n"); status = RtlImpersonateSelf( SecurityImpersonation ); break;
@@ -1531,7 +1531,7 @@ NTSTATUS WINAPI NtSetVolumeInformationFile( ULONG Length, FS_INFORMATION_CLASS FsInformationClass) { - FIXME("(%p,%p,%p,0x%08x,0x%08x) stub\n", + FIXME_ONCE("(%p,%p,%p,0x%08x,0x%08x) stub\n", FileHandle,IoStatusBlock,FsInformation,Length,FsInformationClass); return 0; } @@ -2397,8 +2397,7 @@ static NTSTATUS get_device_info( int fd, FILE_FS_DEVICE_INFORMATION *info ) } } #else - static int warned; - if (!warned++) FIXME( "device info not properly supported on this platform\n" ); + FIXME_ONCE( "device info not properly supported on this platform\n" ); info->DeviceType = FILE_DEVICE_DISK_FILE_SYSTEM; #endif info->Characteristics |= FILE_DEVICE_IS_MOUNTED; @@ -2430,7 +2429,6 @@ NTSTATUS WINAPI NtQueryVolumeInformationFile( HANDLE handle, PIO_STATUS_BLOCK io { int fd, needs_close; struct stat st; - static int once;
if ((io->u.Status = server_get_unix_fd( handle, 0,&fd,&needs_close, NULL, NULL )) != STATUS_SUCCESS) return io->u.Status; @@ -2441,10 +2439,10 @@ NTSTATUS WINAPI NtQueryVolumeInformationFile( HANDLE handle, PIO_STATUS_BLOCK io switch( info_class ) { case FileFsVolumeInformation: - if (!once++) FIXME( "%p: volume info not supported\n", handle ); + FIXME_ONCE( "%p: volume info not supported\n", handle ); break; case FileFsLabelInformation: - FIXME( "%p: label info not supported\n", handle ); + FIXME_ONCE( "%p: label info not supported\n", handle ); break; case FileFsSizeInformation: if (length< sizeof(FILE_FS_SIZE_INFORMATION)) @@ -2513,19 +2511,19 @@ NTSTATUS WINAPI NtQueryVolumeInformationFile( HANDLE handle, PIO_STATUS_BLOCK io } break; case FileFsAttributeInformation: - FIXME( "%p: attribute info not supported\n", handle ); + FIXME_ONCE( "%p: attribute info not supported\n", handle ); break; case FileFsControlInformation: - FIXME( "%p: control info not supported\n", handle ); + FIXME_ONCE( "%p: control info not supported\n", handle ); break; case FileFsFullSizeInformation: - FIXME( "%p: full size info not supported\n", handle ); + FIXME_ONCE( "%p: full size info not supported\n", handle ); break; case FileFsObjectIdInformation: - FIXME( "%p: object id info not supported\n", handle ); + FIXME_ONCE( "%p: object id info not supported\n", handle ); break; case FileFsMaximumInformation: - FIXME( "%p: maximum info not supported\n", handle ); + FIXME_ONCE( "%p: maximum info not supported\n", handle ); break; default: io->u.Status = STATUS_INVALID_PARAMETER; @@ -2560,7 +2558,7 @@ NTSTATUS WINAPI NtQueryEaFile( HANDLE hFile, PIO_STATUS_BLOCK iosb, PVOID buffer BOOLEAN single_entry, PVOID ea_list, ULONG ea_list_len, PULONG ea_index, BOOLEAN restart ) { - FIXME("(%p,%p,%p,%d,%d,%p,%d,%p,%d) stub\n", + FIXME_ONCE("(%p,%p,%p,%d,%d,%p,%d,%p,%d) stub\n", hFile, iosb, buffer, length, single_entry, ea_list, ea_list_len, ea_index, restart); return STATUS_ACCESS_DENIED; @@ -2584,7 +2582,7 @@ NTSTATUS WINAPI NtQueryEaFile( HANDLE hFile, PIO_STATUS_BLOCK iosb, PVOID buffer */ NTSTATUS WINAPI NtSetEaFile( HANDLE hFile, PIO_STATUS_BLOCK iosb, PVOID buffer, ULONG length ) { - FIXME("(%p,%p,%p,%d) stub\n", hFile, iosb, buffer, length); + FIXME_ONCE("(%p,%p,%p,%d) stub\n", hFile, iosb, buffer, length); return STATUS_ACCESS_DENIED; }
@@ -2636,18 +2634,16 @@ NTSTATUS WINAPI NtLockFile( HANDLE hFile, HANDLE lock_granted_event, NTSTATUS ret; HANDLE handle; BOOLEAN async; - static BOOLEAN warn = TRUE;
if (apc || io_status || key) { - FIXME("Unimplemented yet parameter\n"); + FIXME_ONCE("Unimplemented yet parameter\n"); return STATUS_NOT_IMPLEMENTED; }
- if (apc_user&& warn) + if (apc_user ) { - FIXME("I/O completion on lock not implemented yet\n"); - warn = FALSE; + FIXME_ONCE("I/O completion on lock not implemented yet\n"); }
for (;;) @@ -2672,7 +2668,7 @@ NTSTATUS WINAPI NtLockFile( HANDLE hFile, HANDLE lock_granted_event,
if (async) { - FIXME( "Async I/O lock wait not implemented, might deadlock\n" ); + FIXME_ONCE( "Async I/O lock wait not implemented, might deadlock\n" ); if (handle) NtClose( handle ); return STATUS_PENDING; } @@ -2710,7 +2706,7 @@ NTSTATUS WINAPI NtUnlockFile( HANDLE hFile, PIO_STATUS_BLOCK io_status,
if (io_status || key) { - FIXME("Unimplemented yet parameter\n"); + FIXME_ONCE("Unimplemented yet parameter\n"); return STATUS_NOT_IMPLEMENTED; }
Michael Stefaniuc wrote:
Hello Max,
are you replacing all FIXME with FIXME_ONCE? While I do agree that we need the _ONCE variant changing all FIXME is not useful.
bye michael
On 07/14/2010 09:02 PM, Max TenEyck Woodbury wrote:
--- dlls/ntdll/file.c | 40 ++++++++++++++++++---------------------- 1 files changed, 18 insertions(+), 22 deletions(-)
Also, the patch that you sent earlier, that implements FIXME_ONCE has been deferred until after wine 1.2, which means you have to send it again after wine 1.2 has been released. The same counts for every patch you have sent today (because they depend on yesterday's patch), so you might want to wait until after the release before sending any more patches. Sven P.S. You can see that it has been deferred here: http://source.winehq.org/patches/
On 07/14/2010 03:57 PM, Sven Baars wrote:
Michael Stefaniuc wrote:
Hello Max,
are you replacing all FIXME with FIXME_ONCE? While I do agree that we need the _ONCE variant changing all FIXME is not useful.
bye michael
On 07/14/2010 09:02 PM, Max TenEyck Woodbury wrote:
--- dlls/ntdll/file.c | 40 ++++++++++++++++++---------------------- 1 files changed, 18 insertions(+), 22 deletions(-)
Also, the patch that you sent earlier, that implements FIXME_ONCE has been deferred until after wine 1.2, which means you have to send it again after wine 1.2 has been released. The same counts for every patch you have sent today (because they depend on yesterday's patch), so you might want to wait until after the release before sending any more patches.
Sven
P.S. You can see that it has been deferred here: http://source.winehq.org/patches/
1) No, I am not replacing all 'FIXME's with 'FIXME_ONCE's. I'm working on things labeled 'stub' or equivalent. Also 'FIXME's that do not have any additional information. I went over the files that bug 15435 said were too noisy. There are almost certainly many other places 'FIXME_ONCE' can be used. 2) Consider committing yesterday's patch instead of deferring it?
On 14 July 2010 22:57, Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
1) No, I am not replacing all 'FIXME's with 'FIXME_ONCE's. I'm working on things labeled 'stub' or equivalent. Also 'FIXME's that do not have any additional information. I went over the files that bug 15435 said were too noisy. There are almost certainly many other places 'FIXME_ONCE' can be used.
Actually, since this seems to come up from time to time, let me go and explicitly say it. I think bug 15435 is mildly offensive at best. There are basically two claims at the basis of that bug report: "debug output confuses users" and "debug output makes log analysis hard". For the first claim, I'd like to think most of our users aren't complete idiots, so I find this hard to believe. Regardless, debug output isn't meant for users, so if you don't want to see debug output run with WINEDEBUG=-all, or don't run from the terminal in the first place. The second claim actively offends me though, because it basically comes down to "I think developers are idiots". If, as a developer who actually does analyze those logs, I really thought those FIXMEs made my life so much harder I would've changed them myself. I like to think I'm capable enough of deciding for myself which debug output I like to see, and I imagine other actual developers are as well. That's not to say there aren't FIXMEs that would be better of only being printed once, but please leave those kinds of decisions to qualified developers familiar with the code in question.
On 07/14/2010 05:57 PM, Henri Verbeet wrote:
On 14 July 2010 22:57, Max TenEyck Woodbury<max(a)mtew.isa-geek.net> wrote:
1) No, I am not replacing all 'FIXME's with 'FIXME_ONCE's. I'm working on things labeled 'stub' or equivalent. Also 'FIXME's that do not have any additional information. I went over the files that bug 15435 said were too noisy. There are almost certainly many other places 'FIXME_ONCE' can be used.
Actually, since this seems to come up from time to time, let me go and explicitly say it. I think bug 15435 is mildly offensive at best. There are basically two claims at the basis of that bug report: "debug output confuses users" and "debug output makes log analysis hard". For the first claim, I'd like to think most of our users aren't complete idiots, so I find this hard to believe. Regardless, debug output isn't meant for users, so if you don't want to see debug output run with WINEDEBUG=-all, or don't run from the terminal in the first place. The second claim actively offends me though, because it basically comes down to "I think developers are idiots". If, as a developer who actually does analyze those logs, I really thought those FIXMEs made my life so much harder I would've changed them myself. I like to think I'm capable enough of deciding for myself which debug output I like to see, and I imagine other actual developers are as well.
That's not to say there aren't FIXMEs that would be better of only being printed once, but please leave those kinds of decisions to qualified developers familiar with the code in question.
I did read the bug report and I think I understand the issue. Dan Kegel suggested that that bug report was a good place to start working on wine. I did think about what was happening. If a report contains variable information that should be printed on each entry, that report should us 'TRACE', not 'FIXME'. 'FIXME's that contain no variable information are completely redundant after their first report. After the first reminder, the additional reports are just noise. They add no additional information in terms of action required. If this is not true in a particular situation, again, 'TRACE' should be used. Similarly, 'WARN' without variable information are usually redundant after they are first issued. I may be new to Wine, but I have a substantial amount of experience designing, writing and fixing code. I learned programming by fixing my fathers code. I more than understand and accept your desire to decide that '_ONCE' is appropriate in special situations but there are many situations where the need for '_ONCE' is obvious. The changes I have made are of that type, at least in my opinion.
On Wednesday, July 14, 2010 5:02:59 pm Max TenEyck Woodbury wrote:
'FIXME's that contain no variable information are completely redundant after their first report. After the first reminder, the additional reports are just noise. They add no additional information in terms of action required.
I wouldn't say that. Sometimes the simple knowledge that a FIXME is called a whole lot says enough on its own (eg, in WineD3D, you get a fixme when an sRGB reload occurs, because it's a performance drain; if this happens a lot, it can be taken as a source for performance issues). Sometimes knowing particular a fixme is triggered near to a crash or other behavioral anomaly can say a bit, too. If such fixmes were only printed once, it would be impossible to see this information without more in-depth testing that most users won't bother with. If the fixme is only printed once and the rest are TRACEs, it would still cause more work and a whole lot more noise (ie. all the other traces) in trying to spot it.
On 07/14/2010 08:53 PM, Chris Robinson wrote:
On Wednesday, July 14, 2010 5:02:59 pm Max TenEyck Woodbury wrote:
'FIXME's that contain no variable information are completely redundant after their first report. After the first reminder, the additional reports are just noise. They add no additional information in terms of action required.
I wouldn't say that. Sometimes the simple knowledge that a FIXME is called a whole lot says enough on its own (eg, in WineD3D, you get a fixme when an sRGB reload occurs, because it's a performance drain; if this happens a lot, it can be taken as a source for performance issues). Sometimes knowing particular a fixme is triggered near to a crash or other behavioral anomaly can say a bit, too.
If such fixmes were only printed once, it would be impossible to see this information without more in-depth testing that most users won't bother with. If the fixme is only printed once and the rest are TRACEs, it would still cause more work and a whole lot more noise (ie. all the other traces) in trying to spot it.
Good points, but the formatting and I/O can contribute significantly to the lack of performance. There is also the fact that trying to fix performance problems before you have the operation working correctly is really poor technique. Which 'FIXME's for stubs should not be '_ONCE'd is an issue for specialists. With the new macros, it is easy to back out this kind of change. Another variation could be made to use an exponential back-off for the reports. That is, the 2^0, 2^1, 2^2, 2^3, 2^4... instances could be allowed to print, but that's something that we might want to discuss before throwing it into the mix of options available. What would you call that anyway? '_22I'?
Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
I may be new to Wine, but I have a substantial amount of experience designing, writing and fixing code. I learned programming by fixing my fathers code. I more than understand and accept your desire to decide that '_ONCE' is appropriate in special situations but there are many situations where the need for '_ONCE' is obvious. The changes I have made are of that type, at least in my opinion.
It's better to spend time by implementing missing features and effectively removing those FIXMEs instead of inventing new ways to disable output. As been said if too much FIXME messages hurts your eyes just use '-all'. That bug should be closed as wontfix if not as inavlid. -- Dmitry.
On 07/15/2010 01:13 AM, Dmitry Timoshkov wrote:
Max TenEyck Woodbury<max(a)mtew.isa-geek.net> wrote:
I may be new to Wine, but I have a substantial amount of experience designing, writing and fixing code. I learned programming by fixing my fathers code. I more than understand and accept your desire to decide that '_ONCE' is appropriate in special situations but there are many situations where the need for '_ONCE' is obvious. The changes I have made are of that type, at least in my opinion.
It's better to spend time by implementing missing features and effectively removing those FIXMEs instead of inventing new ways to disable output. As been said if too much FIXME messages hurts your eyes just use '-all'. That bug should be closed as wontfix if not as inavlid.
Thank you for your opinion. In order to actually implement the features, I have to have good descriptions of what those features are, which was the reason I started the API documentation pages on the wiki. Since things are at an impasse on that front, I have time to work on less critical things. I've done a fair amount with debugging frameworks in the past, so I think I can make useful contributions in that area. I am aware of the '-all' option, but there are times when it is inappropriate. Particularly when it comes to regression and unit tests. It is not a matter of visual discomfort. It is a matter of signal to noise ratios. Reducing the noise makes it easier to identify the problems. Each of us has strengths and weaknesses. I have some idea of my own but I am loath to try to prescribe for others. On a project the size of wine, the diversity of talents available is a great positive. Let's not screw things up by trying to dictate what others should or should not do. If I am not mistaken, Alexandre is the one who decides what gets fixed. I observe that some movement has already taken place with respect to bug 15345. In reviewing the various files, some patching has already been done, so 'WONTFIX' is not really an option. How much more should be done is another question of course. It will be answered by people who are willing to do the work and by Alexandre.
Max TenEyck Woodbury <max(a)mtew.isa-geek.net> wrote:
If I am not mistaken, Alexandre is the one who decides what gets fixed. I observe that some movement has already taken place with respect to bug 15345. In reviewing the various files, some patching has already been done, so 'WONTFIX' is not really an option. How much more should be done is another question of course. It will be answered by people who are willing to do the work and by Alexandre.
The patches which silense some FIXME's have been accepted as an exception. That covers the code which is unlikely to be fixed in near future, or the code nobody is working on. Having a blanket FIXME_ONCE is not acceptable at all. As I said before, better spend time learning things and fix those FIXMEs instead. Creating documentation for the whole Win32 API and writing the tests for single API and implementing missing features one by one are completely different things of the scale and effort required. -- Dmitry.
* On Wed, 14 Jul 2010, Chris Robinson wrote:
* On Wednesday, July 14, 2010 5:02:59 pm Max TenEyck Woodbury wrote:
'FIXME's that contain no variable information are completely redundant after their first report. After the first reminder, the additional reports are just noise. They add no additional information in terms of action required.
I wouldn't say that. Sometimes the simple knowledge that a FIXME is called a whole lot says enough on its own (eg, in WineD3D, you get a fixme when an sRGB reload occurs, because it's a performance drain; if this happens a lot, it can be taken as a source for performance issues). Sometimes knowing particular a fixme is triggered near to a crash or other behavioral anomaly can say a bit, too.
Although I stand for *_ONCE implementation, I agree with Chris. And there might be some alternative: lets then implement it like a FIXME_ONCE_PER(N)("a_debug_string"); Which could print something like a "fixme:channel:a_debug_string [repeated N times]" This would not oly still tell you approximate frequency of a FIXME, but it also would require developer to know reasonable occurence quanta value (N) or to tune it later. This (for me) sounds like a way to know the code's stochastic behaviour better! Which would be a plus.
If such fixmes were only printed once, it would be impossible to see this information without more in-depth testing that most users won't bother with. If the fixme is only printed once and the rest are TRACEs, it would still cause more work and a whole lot more noise (ie. all the other traces) in trying to spot it.
TRACE_ONCE probably could help in some cases too. There I see another constraint of *_ONCE functionality. "Once" could be per block (eg. for-loop), per wrapping function call, per process or even once per thread's life. Implementing all onces might be difficult, IMHO. Though not all of them could be necessary, probably. * On Thu, 15 Jul 2010, Dmitry Timoshkov wrote:
The patches which silense some FIXME's have been accepted as an exception. That covers the code which is unlikely to be fixed in near future, or the code nobody is working on.
Then at least replacement of these (ugly by their size, IMHO) silence-blocks with a simple FIXME_ONCE seems rational. S.
Saulius Krasuckas wrote:
* On Wed, 14 Jul 2010, Chris Robinson wrote:
* On Wednesday, July 14, 2010 5:02:59 pm Max TenEyck Woodbury wrote:
'FIXME's that contain no variable information are completely redundant after their first report. After the first reminder, the additional reports are just noise. They add no additional information in terms of action required. I wouldn't say that. Sometimes the simple knowledge that a FIXME is called a whole lot says enough on its own (eg, in WineD3D, you get a fixme when an sRGB reload occurs, because it's a performance drain; if this happens a lot, it can be taken as a source for performance issues). Sometimes knowing particular a fixme is triggered near to a crash or other behavioral anomaly can say a bit, too.
Although I stand for *_ONCE implementation, I agree with Chris.
And there might be some alternative: lets then implement it like a
FIXME_ONCE_PER(N)("a_debug_string");
Which could print something like a
"fixme:channel:a_debug_string [repeated N times]"
This would not oly still tell you approximate frequency of a FIXME, but it also would require developer to know reasonable occurence quanta value (N) or to tune it later. This (for me) sounds like a way to know the code's stochastic behaviour better! Which would be a plus.
If such fixmes were only printed once, it would be impossible to see this information without more in-depth testing that most users won't bother with. If the fixme is only printed once and the rest are TRACEs, it would still cause more work and a whole lot more noise (ie. all the other traces) in trying to spot it.
TRACE_ONCE probably could help in some cases too. There I see another I fail to see how TRACE_ONCE could make any sense. TRACE is used to trace the important parts of the code flow. Just printing a TRACE once is less than helpful; it is waay better to not print the TRACE at all. Of course TRACE_ONCE should be defined for symmetry and preventing somebody wasting time to send a patch with an implementation for it. Too bad that Alexandre is too nice else that would be something like:
#define TRACE_ONCE TRACE_ONCE_Misguided_developer_detected bye michael
constraint of *_ONCE functionality. "Once" could be per block (eg. for-loop), per wrapping function call, per process or even once per thread's life.
Implementing all onces might be difficult, IMHO. Though not all of them could be necessary, probably.
* On Thu, 15 Jul 2010, Dmitry Timoshkov wrote:
The patches which silense some FIXME's have been accepted as an exception. That covers the code which is unlikely to be fixed in near future, or the code nobody is working on.
Then at least replacement of these (ugly by their size, IMHO) silence-blocks with a simple FIXME_ONCE seems rational.
* On Thu, 15 Jul 2010, Michael Stefaniuc wrote:
Saulius Krasuckas wrote:
TRACE_ONCE probably could help in some cases too. There I see another
I fail to see how TRACE_ONCE could make any sense. TRACE is used to trace the important parts of the code flow. Just printing a TRACE once is less than helpful; it is waay better to not print the TRACE at all.
I was in a hurry a bit and I am persuaded now :), thanks.
Of course TRACE_ONCE should be defined for symmetry and preventing somebody wasting time to send a patch with an implementation for it. Too bad that Alexandre is too nice else that would be something like:
#define TRACE_ONCE TRACE_ONCE_Misguided_developer_detected
I am away from development now, but you will never know :). Such define sounds OK. Or has it already been decided on IRC to not exist? S. PS: Congrats to all the folks for Wine-1.2!
participants (7)
-
Chris Robinson -
Dmitry Timoshkov -
Henri Verbeet -
Max TenEyck Woodbury -
Michael Stefaniuc -
Saulius Krasuckas -
Sven Baars