2008/8/24 Markus Hitter mah@jump-ing.de:
+ if (!attr || !attr->ObjectName) + { + TRACE("returning STATUS_INVALID_PARAMETER\n"); + return STATUS_INVALID_PARAMETER; + }
These are all very useless TRACES, except for possibly the returned handle value. You should be able to look at the traced parameters and see if there is an invalid parameter. As a side note of something I just noticed, the check for NULL attr will never be true because we'll crash in the TRACE when we dereference attr.
Am 25.08.2008 um 01:31 schrieb James Hawkins:
2008/8/24 Markus Hitter mah@jump-ing.de:
- if (!attr || !attr->ObjectName)
- {
TRACE("returning STATUS_INVALID_PARAMETER\n");return STATUS_INVALID_PARAMETER;- }
These are all very useless TRACES, except for possibly the returned handle value.
Well, the idea is to TRACE() something for all possible return values. Michael Karcher, Rob Shearman and me obviously consider them as useful:
http://thread.gmane.org/gmane.comp.emulators.wine.patches/54527/
As a side note of something I just noticed, the check for NULL attr will never be true because we'll crash in the TRACE when we dereference attr.
In case of (attr == NULL), same as (!attr), the code right to the || shouldn't be reached, so no dereferencing should take place, then. TRACE()/printf() is capable of handling 0/NULL/nil values as well.
BTW., the TRACE() in the code you cited doesn't dereference anything and for the other parts of the patch, dereferencing only takes place where the current code dereferences anyways.
MarKus
- - - - - - - - - - - - - - - - - - - Dipl. Ing. Markus Hitter http://www.jump-ing.de/
On Mon, Aug 25, 2008 at 3:12 AM, Markus Hitter mah@jump-ing.de wrote:
Am 25.08.2008 um 01:31 schrieb James Hawkins:
2008/8/24 Markus Hitter mah@jump-ing.de:
- if (!attr || !attr->ObjectName)
- {
TRACE("returning STATUS_INVALID_PARAMETER\n");return STATUS_INVALID_PARAMETER;- }
These are all very useless TRACES, except for possibly the returned handle value.
Well, the idea is to TRACE() something for all possible return values. Michael Karcher, Rob Shearman and me obviously consider them as useful:
http://thread.gmane.org/gmane.comp.emulators.wine.patches/54527/
I doubt Rob was agreeing with the above trace. He merely clarified how you should print variable values and returned handles.
As a side note of something I just noticed, the check for NULL attr will never be true because we'll crash in the TRACE when we dereference attr.
In case of (attr == NULL), same as (!attr), the code right to the || shouldn't be reached, so no dereferencing should take place, then. TRACE()/printf() is capable of handling 0/NULL/nil values as well.
No, take a look at the TRACE again. If attr is NULL, you'll crash in the TRACE. Neither TRACE nor printf handle NULL strings; debugstr_a/w is what handles NULL strings, but that's beside the point I'm making, as we're talking about attr being NULL, not attr->ObjectName.
BTW., the TRACE() in the code you cited doesn't dereference anything and for the other parts of the patch, dereferencing only takes place where the current code dereferences anyways.
Thus why I said "as a side note." It had nothing to do with your patch.
Am 25.08.2008 um 17:58 schrieb James Hawkins:
On Mon, Aug 25, 2008 at 3:12 AM, Markus Hitter mah@jump-ing.de wrote:
Am 25.08.2008 um 01:31 schrieb James Hawkins:
2008/8/24 Markus Hitter mah@jump-ing.de:
- if (!attr || !attr->ObjectName) return
STATUS_INVALID_PARAMETER;
- if (!attr || !attr->ObjectName)
- {
TRACE("returning STATUS_INVALID_PARAMETER\n");return STATUS_INVALID_PARAMETER;- }
If attr is NULL, you'll crash in the TRACE.
Pardon. If attr is NULL, the TRACE() isn't even reached.
As I obviously can't follow you how the additional TRACE() makes the code more fragile, please go ahead and post a short sample code showing how the above snippet is done right. Thanks.
Markus
P.S.: I took the freedom and extended your patch citation slightly.
- - - - - - - - - - - - - - - - - - - Dipl. Ing. Markus Hitter http://www.jump-ing.de/
On Mon, Aug 25, 2008 at 2:29 PM, Markus Hitter mah@jump-ing.de wrote:
Am 25.08.2008 um 17:58 schrieb James Hawkins:
On Mon, Aug 25, 2008 at 3:12 AM, Markus Hitter mah@jump-ing.de wrote:
Am 25.08.2008 um 01:31 schrieb James Hawkins:
2008/8/24 Markus Hitter mah@jump-ing.de:
- if (!attr || !attr->ObjectName) return STATUS_INVALID_PARAMETER;
- if (!attr || !attr->ObjectName)
- {
TRACE("returning STATUS_INVALID_PARAMETER\n");return STATUS_INVALID_PARAMETER;- }
If attr is NULL, you'll crash in the TRACE.
Pardon. If attr is NULL, the TRACE() isn't even reached.
Have you even gone back to look at the code? Do you know what a side note is? I've said it twice now that I wasn't talking about your patch. As the code stands now, regardless of your patch, the check for NULL attr on line 154 of dlls/kernel32/file.c will never be hit because we will crash in the TRACE on line 148 of dlls/kernel32/file.c.
As I obviously can't follow you how the additional TRACE() makes the code more fragile, please go ahead and post a short sample code showing how the above snippet is done right. Thanks.
The added TRACE has nothing to do with the fragility of the code. My comment still stands that the added TRACE is absolutely superfluous.
Am 25.08.2008 um 21:39 schrieb James Hawkins:
As the code stands now, regardless of your patch, the check for NULL attr on line 154 of dlls/kernel32/file.c will never be hit because we will crash in the TRACE on line 148 of dlls/kernel32/ file.c.
Now I got it. You aren't talking about the TRACE() you cited, but a different one. Thanks for the line numbers, applied to dlls/ntdll/ file.c they make sense.
So, why not fix this? Please push the patch trough Wine's patch accepting mechanism yourself, I'm currently somewhat sick of it.
My comment still stands that the added TRACE is absolutely superfluous.
Makes 2 pro, 1 neutral. Do whatever you want with it.
MarKus
- - - - - - - - - - - - - - - - - - - Dipl. Ing. Markus Hitter http://www.jump-ing.de/
So, why not fix this? Please push the patch trough Wine's patch accepting mechanism yourself, I'm currently somewhat sick of it.
It seems your tolerance for reworking patches is rather low. You must be willing to rework patches until they're correct to have many committed around here. I've probably had more patches rejected than you've sent so far, so I know whereof I speak ;-)
My comment still stands that the added TRACE is absolutely superfluous.
Makes 2 pro, 1 neutral. Do whatever you want with it.
James is the wrong target for this comment, and voting is irrelevant. The burden is always on you to convince AJ that your patch is correct.
Since you're new-ish around here, I had a look.
- if (!attr || !attr->ObjectName) return STATUS_INVALID_PARAMETER; + if (!attr || !attr->ObjectName) + { + TRACE("returning STATUS_INVALID_PARAMETER\n"); + return STATUS_INVALID_PARAMETER; + }
I agree with James that this is superfluous, or at least that it doesn't match the style of dlls/ntdll/file.c: when error is encountered, e.g. in NtReadFile line 568, it's returned directly.
if (attr->RootDirectory) { FIXME( "RootDirectory %p not supported\n", attr->RootDirectory ); + TRACE("returning STATUS_OBJECT_NAME_NOT_FOUND\n");
Adding a trace after a fixme is clearly superfluous.
- if (io->u.Status == STATUS_SUCCESS) io->Information = FILE_OPENED; + if (io->u.Status == STATUS_SUCCESS) + { + io->Information = FILE_OPENED; + TRACE("returning STATUS_SUCCESS, handle %p\n", *handle); + } + else + TRACE("returning 0x%08x\n", io->u.Status); return io->u.Status;
Since the return is done regardless if the status, why do you trace a different value depending on the status? One trace just before the return should suffice. Same with the remaining returns: a single trace just before the return should suffice.
Please try to minimize the changes you're making, pay attention to the style of the file you're changing, and be receptive to comments you get from other developers. --Juan
Am 26.08.2008 um 01:33 schrieb Juan Lang:
It seems your tolerance for reworking patches is rather low.
Currently, yes. I'll have to take a breath.
- if (!attr || !attr->ObjectName) return STATUS_INVALID_PARAMETER;
- if (!attr || !attr->ObjectName)
- {
TRACE("returning STATUS_INVALID_PARAMETER\n");return STATUS_INVALID_PARAMETER;- }
I agree with James that this is superfluous, or at least that it doesn't match the style of dlls/ntdll/file.c: when error is encountered, e.g. in NtReadFile line 568, it's returned directly.
It's returned, but won't show up in the console. I thought the idea of TRACE()s is to allow some sort of cheap printf()-style debugging and should be complete. Perhaps there are other uses I didn't figure yet.
if (attr->RootDirectory) { FIXME( "RootDirectory %p not supported\n", attr-RootDirectory );
TRACE("returning STATUS_OBJECT_NAME_NOT_FOUND\n");Adding a trace after a fixme is clearly superfluous.
OK.
if (io->u.Status == STATUS_SUCCESS) io->Information =FILE_OPENED;
if (io->u.Status == STATUS_SUCCESS){io->Information = FILE_OPENED;TRACE("returning STATUS_SUCCESS, handle %p\n", *handle);}elseTRACE("returning 0x%08x\n", io->u.Status); return io->u.Status;Since the return is done regardless if the status, why do you trace a different value depending on the status?
Because *handle is valid on success only and to not confuse the observer with invalid handles. Perhaps this is more care than needed.
Please try to minimize the changes you're making
Sure. After the breath :-)
MarKus
- - - - - - - - - - - - - - - - - - - Dipl. Ing. Markus Hitter http://www.jump-ing.de/
Markus Hitter mah@jump-ing.de writes:
It's returned, but won't show up in the console. I thought the idea of TRACE()s is to allow some sort of cheap printf()-style debugging and should be complete.
It can't be complete, that would make it totally unmanageable, and make the code unreadable. It's a trade-off between quality and quantity of information. Module-specific traces like this one should try to print as little information as necessary to make sense of the trace, so that the resulting log is manageable. In general a single trace at function entry is enough to understand the code flow. If you want details like the exact error codes you can use +relay or +server.
-
Alexandre Julliard -
James Hawkins -
Juan Lang -
Markus Hitter