Hello Ismael,
reviewing this patch shows some issues.
> +#define _okHR(expected, result) \
- ok( (expected) == (result), \
"expected=%d(%s) got=%d(%s)\n", \HRESULT_CODE(expected), dpResult2str(expected), \HRESULT_CODE(result), dpResult2str(result) );
I dislike the order of the parameters and the name. You would write ok( hr == DP_OK, "Something failed with %x\n", hr); In this line, "hr" is before "DP_OK". So as an analogy, I also want to write _okHR( hr, DD_OK );
Why do you start the macro name with an underscore? And last but not least: As I see it, a static inline would do the job too, and is always preferrable to macros.
- heap = HeapCreate( 0, 0, 0 );
Do you have any reason to create your own heap? Each windows process already comes equipped with a standard heap you can obtain using the API function GetProcessHeap, so this line:
- IUnknown* pUnk = HeapAlloc( heap, 0, sizeof(LPDWORD) );
would then look like: + IUnknown* pUnk = HeapAlloc( GetProcessHeap(), 0, sizeof(LPDWORD) );
Another point is that this line is totally wrong anyway. An IUnknown* is a pointer to an COM object, which starts with a vtable pointer. You let pUnk point to uninitialized memory. While it doesn't matter in this case (as DirectPlayCreate will never access what the pointer points to), it is really bad style. I understand the intention to test that aggregation is not supported for DirectPlay, but you still should keep the contract of the creation function that pUnk either is NULL or points to a valid COM object. A basic COM object is just a pointer to a vtable that contains a QueryInterface method that returns "this" for IUnknown and fails in every other case, an AddRef method that returns 2 and does nothing, and a Release method that returns 1 and does nothing else.
+ /* pUnk==NULL, pDP!=NULL */
- hr = DirectPlayCreate( NULL, &pDP, NULL );
- _okHR( DPERR_INVALIDPARAMS, hr );
- hr = DirectPlayCreate( (LPGUID) &_GUID_NULL, &pDP, NULL );
- _okHR( DP_OK, hr );
- hr = DirectPlayCreate( (LPGUID) &DPSPGUID_TCPIP, &pDP, NULL );
- todo_wine _okHR( DP_OK, hr );
Here you are leaking DirectPlay objects. If DirectPlayCreate worked, you have to release the object before you overwrite pDP (by passing it to DirectPlayCreate again) using IDirectPlay_Release(pDP).
Regards, Michael Karcher
On Wed, Jul 9, 2008 at 11:21 AM, Michael Karcher wine@mkarcher.dialup.fu-berlin.de wrote:
Hello Ismael,
reviewing this patch shows some issues.
> +#define _okHR(expected, result) \
- ok( (expected) == (result), \
"expected=%d(%s) got=%d(%s)\n", \HRESULT_CODE(expected), dpResult2str(expected), \HRESULT_CODE(result), dpResult2str(result) );I dislike the order of the parameters and the name. You would write ok( hr == DP_OK, "Something failed with %x\n", hr); In this line, "hr" is before "DP_OK". So as an analogy, I also want to write _okHR( hr, DD_OK );
Why do you start the macro name with an underscore? And last but not least: As I see it, a static inline would do the job too, and is always preferrable to macros.
We use macros for this (ok*/expected) all over the code base whenever the check is small enough. See dlls/comctl32/tests/trackbar.c:28 for an example. If you use an inlined function, you either have to pass the __LINE__ value to the function or wrap the call to the function in a macro anyway.
Thanks for the review :)
On 7/9/08, Michael Karcher wine@mkarcher.dialup.fu-berlin.de wrote:
Hello Ismael,
reviewing this patch shows some issues.
> +#define _okHR(expected, result) \
- ok( (expected) == (result), \
"expected=%d(%s) got=%d(%s)\n", \HRESULT_CODE(expected), dpResult2str(expected), \HRESULT_CODE(result), dpResult2str(result) );I dislike the order of the parameters and the name. You would write ok( hr == DP_OK, "Something failed with %x\n", hr); In this line, "hr" is before "DP_OK". So as an analogy, I also want to write _okHR( hr, DD_OK );
Depending on the situation I write ok( DP_OK == hr ) instead of ok( hr == DP_OK, ... ), specially if instead of "hr" there is a DirectPlay call or some complex expression, in order to improve readability. Regarding this second case, I decided to use the (expected, result) order, but it's true that sometimes it can be confusing.
Why do you start the macro name with an underscore?
Because _okHR compares HRESULT codes, but there's also _ok to compare integers, or _okFlags to compare flags (the main difference being how to print info about the expected and actual values), or _okStr to compare strings.
And last but not least: As I see it, a static inline would do the job too, and is always preferrable to macros.
The main reason of using ditry macros instead of functions was to preserve the __LINE__ number, otherwise the debug information provided is pretty much useless.
- heap = HeapCreate( 0, 0, 0 );
Do you have any reason to create your own heap? Each windows process already comes equipped with a standard heap you can obtain using the API function GetProcessHeap, so this line:
- IUnknown* pUnk = HeapAlloc( heap, 0, sizeof(LPDWORD) );
would then look like:
- IUnknown* pUnk = HeapAlloc( GetProcessHeap(), 0, sizeof(LPDWORD) );
That's what I used in the begining, but I've got some HeapAllocs that I don't free. This is intended, as freeing that memory would make the code unnecessarily complex, I can't use static local variables (I use the result of the function inside the same ok() or trace()), and loosing memory is always undesirable but not critical in a test case. The main problem was to get yelled when valgrind complains about the memory leak, and Kai Blin advised me to use my own heap to avoid valgrind complains. Anyway is this is too dirty I'll have no problem in doing it "correctly".
Another point is that this line is totally wrong anyway. An IUnknown* is a pointer to an COM object, which starts with a vtable pointer. You let pUnk point to uninitialized memory. While it doesn't matter in this case (as DirectPlayCreate will never access what the pointer points to), it is really bad style. I understand the intention to test that aggregation is not supported for DirectPlay, but you still should keep the contract of the creation function that pUnk either is NULL or points to a valid COM object. A basic COM object is just a pointer to a vtable that contains a QueryInterface method that returns "this" for IUnknown and fails in every other case, an AddRef method that returns 2 and does nothing, and a Release method that returns 1 and does nothing else.
I just wrote is to check the return value in case of pUnk != NULL, but there would be no problem in either removing that test (it's probably unneeded) or doing it the right way.
+ /* pUnk==NULL, pDP!=NULL */
- hr = DirectPlayCreate( NULL, &pDP, NULL );
- _okHR( DPERR_INVALIDPARAMS, hr );
- hr = DirectPlayCreate( (LPGUID) &_GUID_NULL, &pDP, NULL );
- _okHR( DP_OK, hr );
- hr = DirectPlayCreate( (LPGUID) &DPSPGUID_TCPIP, &pDP, NULL );
- todo_wine _okHR( DP_OK, hr );
Here you are leaking DirectPlay objects. If DirectPlayCreate worked, you have to release the object before you overwrite pDP (by passing it to DirectPlayCreate again) using IDirectPlay_Release(pDP).
Roger
Regards, Michael Karcher
On Wednesday 09 July 2008 19:15:34 Ismael Barros wrote:
- heap = HeapCreate( 0, 0, 0 );
Do you have any reason to create your own heap? Each windows process already comes equipped with a standard heap you can obtain using the API
That's what I used in the begining, but I've got some HeapAllocs that I don't free. This is intended, as freeing that memory would make the code unnecessarily complex, I can't use static local variables (I use the result of the function inside the same ok() or trace()), and loosing memory is always undesirable but not critical in a test case. The main problem was to get yelled when valgrind complains about the memory leak, and Kai Blin advised me to use my own heap to avoid valgrind complains. Anyway is this is too dirty I'll have no problem in doing it "correctly".
This might just be me getting used to Samba's libtalloc too much, but what downside is there to allocate all memory for a test case on one heap and just freeing that once you're done?
Cheers, Kai
On Wed, 9 Jul 2008, Kai Blin wrote:
On Wednesday 09 July 2008 19:15:34 Ismael Barros wrote:
- heap = HeapCreate( 0, 0, 0 );
Do you have any reason to create your own heap? Each windows process already comes equipped with a standard heap you can obtain using the API
That's what I used in the begining, but I've got some HeapAllocs that I don't free. This is intended, as freeing that memory would make the code unnecessarily complex, I can't use static local variables (I use the result of the function inside the same ok() or trace()), and loosing memory is always undesirable but not critical in a test case. The main problem was to get yelled when valgrind complains about the memory leak, and Kai Blin advised me to use my own heap to avoid valgrind complains. Anyway is this is too dirty I'll have no problem in doing it "correctly".
This might just be me getting used to Samba's libtalloc too much, but what downside is there to allocate all memory for a test case on one heap and just freeing that once you're done?
Would it cause trouble for Valgrind? I.e. will Valgrind recognize that the heap destruction actually frees all the relevant HeapAlloc() calls? Or will it just report massive memory leakage?
Am Mittwoch, den 09.07.2008, 20:15 +0300 schrieb Ismael Barros:
Thanks for the review :)
No problem.
On 7/9/08, Michael Karcher wine@mkarcher.dialup.fu-berlin.de wrote:
I dislike the order of the parameters and the name. You would write
Because _okHR compares HRESULT codes, but there's also _ok to compare integers, or _okFlags to compare flags (the main difference being how to print info about the expected and actual values), or _okStr to compare strings.
OK, taking back the criticism on parameter order. Passing the actual value last is common practice in the user32 test, so your variant has a precedence case in wine. Yet I couldn't find these kind of macros starting with an underscore. More on that point below.
And last but not least: As I see it, a static inline would do the job too, and is always preferrable to macros.
The main reason of using ditry macros instead of functions was to preserve the __LINE__ number, otherwise the debug information provided is pretty much useless.
Yes, you are right of course. I didn't think of __LINE__ (also James noted the same thing). Now you get to the point where a precedence case for identifiers with an underscore are used: shell32/tests/shlexec.c. They have inline functions, called _okFooBar which take file and line as parameter, and have a macro okFooBar which calls the _okFooBar method, so the test case writer does not get to see the identifier starting with an underscore. Thats how I feel about them: They are internal identifiers, telling you "if you use this directly, you better know very well what you are doing; most probably there is a safer or easier way to accomplish the goal". In this case, the _okFooBar function is meant just for the easier okFooBar macro.
- heap = HeapCreate( 0, 0, 0 );
Do you have any reason to create your own heap? Each windows process already comes equipped with a standard heap you can obtain using the API function GetProcessHeap, so this line:
- IUnknown* pUnk = HeapAlloc( heap, 0, sizeof(LPDWORD) );
would then look like:
- IUnknown* pUnk = HeapAlloc( GetProcessHeap(), 0, sizeof(LPDWORD) );
That's what I used in the begining, but I've got some HeapAllocs that I don't free.
OK. That's a point. I didn't expect it, though. I would like a comment in there along /* Create an extra heap to throw away garbage after tests */
This is intended, as freeing that memory would make the code unnecessarily complex, I can't use static local variables (I use the result of the function inside the same ok() or trace()), and loosing memory is always undesirable but not critical in a test case.
You can use static variables, if you go along the lines of of get_temp_buffer in libs/wine/debug.c, where a static local array is cyclical indexed. This might not be a good idea, though, depending on how long the result has to stay alive.
Another point is that this line is totally wrong anyway. An IUnknown* is a pointer to an COM object, which starts with a vtable pointer. You let pUnk point to uninitialized memory. While it doesn't matter in this case (as DirectPlayCreate will never access what the pointer points to), it is really bad style. I understand the intention to test that aggregation is not supported for DirectPlay, but you still should keep the contract of the creation function that pUnk either is NULL or points to a valid COM object. A basic COM object is just a pointer to a vtable that contains a QueryInterface method that returns "this" for IUnknown and fails in every other case, an AddRef method that returns 2 and does nothing, and a Release method that returns 1 and does nothing else.
I just wrote is to check the return value in case of pUnk != NULL, but there would be no problem in either removing that test (it's probably unneeded) or doing it the right way.
A test case is always good, if it does not take too much execution time. These tests with NULL pointers seemed strange to me when I first encountered, as sometimes NULL pointers are passed into functions where they are not useful or even forbidden by the documented contract. But as wine is all about fulfilling the same contract as the windows implementation which might deviate from MSDN, the test case serves as documentation of Windows behavior, which is good.
But in my opinion, testing of the reaction to contract violation should not go into passing somehow valid-looking pointers, that do not point to anything sensible. And the main point was, that I really don't get why it must be a heap pointer. The an LPDWORD on the stack would do as well as allocating sizeof(LPDWORD) area.
Regards, Michael Karcher
And last but not least: As I see it, a static inline would do the job too, and is always preferrable to macros.
The main reason of using ditry macros instead of functions was to preserve the __LINE__ number, otherwise the debug information provided is pretty much useless.
Yes, you are right of course. I didn't think of __LINE__ (also James noted the same thing). Now you get to the point where a precedence case for identifiers with an underscore are used: shell32/tests/shlexec.c. They have inline functions, called _okFooBar which take file and line as parameter, and have a macro okFooBar which calls the _okFooBar method, so the test case writer does not get to see the identifier starting with an underscore. Thats how I feel about them: They are internal identifiers, telling you "if you use this directly, you better know very well what you are doing; most probably there is a safer or easier way to accomplish the goal". In this case, the _okFooBar function is meant just for the easier okFooBar macro.
Sounds fine, I'll change it.
- heap = HeapCreate( 0, 0, 0 );
Do you have any reason to create your own heap? Each windows process already comes equipped with a standard heap you can obtain using the API function GetProcessHeap, so this line:
- IUnknown* pUnk = HeapAlloc( heap, 0, sizeof(LPDWORD) );
would then look like:
- IUnknown* pUnk = HeapAlloc( GetProcessHeap(), 0, sizeof(LPDWORD) );
That's what I used in the begining, but I've got some HeapAllocs that I don't free.
OK. That's a point. I didn't expect it, though. I would like a comment in there along /* Create an extra heap to throw away garbage after tests */
No problem.
This is intended, as freeing that memory would make the code unnecessarily complex, I can't use static local variables (I use the result of the function inside the same ok() or trace()), and loosing memory is always undesirable but not critical in a test case.
You can use static variables, if you go along the lines of of get_temp_buffer in libs/wine/debug.c, where a static local array is cyclical indexed. This might not be a good idea, though, depending on how long the result has to stay alive.
Hm, yeah, I thought about a cyclic buffer but wanted to keep the code simple. However this heap thing is creating some skepticism, so I'll go that way.
Another point is that this line is totally wrong anyway. An IUnknown* is a pointer to an COM object, which starts with a vtable pointer. You let pUnk point to uninitialized memory. While it doesn't matter in this case (as DirectPlayCreate will never access what the pointer points to), it is really bad style. I understand the intention to test that aggregation is not supported for DirectPlay, but you still should keep the contract of the creation function that pUnk either is NULL or points to a valid COM object. A basic COM object is just a pointer to a vtable that contains a QueryInterface method that returns "this" for IUnknown and fails in every other case, an AddRef method that returns 2 and does nothing, and a Release method that returns 1 and does nothing else.
I just wrote is to check the return value in case of pUnk != NULL, but there would be no problem in either removing that test (it's probably unneeded) or doing it the right way.
A test case is always good, if it does not take too much execution time. These tests with NULL pointers seemed strange to me when I first encountered, as sometimes NULL pointers are passed into functions where they are not useful or even forbidden by the documented contract. But as wine is all about fulfilling the same contract as the windows implementation which might deviate from MSDN, the test case serves as documentation of Windows behavior, which is good.
But in my opinion, testing of the reaction to contract violation should not go into passing somehow valid-looking pointers, that do not point to anything sensible. And the main point was, that I really don't get why it must be a heap pointer. The an LPDWORD on the stack would do as well as allocating sizeof(LPDWORD) area.
I've been checking how this is done in other tests, and it's particularly interesting how they do it in ddraw/tests/refcount.c:81: hr = IDirectDraw7_CreatePalette(DDraw7, DDPCAPS_ALLOW256 | DDPCAPS_8BIT, Table, &palette, (void *) 0xdeadbeef); This is the only example in the current code. I could either do it that way (not correct, but simple), or use some IUnknown interface. I tried to get the IUnknown of the classes I have access to, DirectPlay or DirectPlayLobby, but the current implementation is not able to return an interface for IID_IUnknown (it just fails with E_NOINTERFACE), so I would have to fix that too (and I'm not very sure about how to do it, without breaking anything. Would returning any of the existing interfaces that have CreateInstance, AddRef and Release, be correct? This seems to be done a lot in other dlls)
Am Donnerstag, den 10.07.2008, 22:04 +0300 schrieb Ismael Barros:
- heap = HeapCreate( 0, 0, 0 );
Do you have any reason to create your own heap? Each windows process already comes equipped with a standard heap you can obtain using the API function GetProcessHeap, so this line:
- IUnknown* pUnk = HeapAlloc( heap, 0, sizeof(LPDWORD) );
would then look like:
- IUnknown* pUnk = HeapAlloc( GetProcessHeap(), 0, sizeof(LPDWORD) );
That's what I used in the begining, but I've got some HeapAllocs that I don't free.
OK. That's a point. I didn't expect it, though. I would like a comment in there along /* Create an extra heap to throw away garbage after tests */
No problem.
[...]
Hm, yeah, I thought about a cyclic buffer but wanted to keep the code simple. However this heap thing is creating some skepticism, so I'll go that way.
For me, the create-heap-and-throw-away approach is OK, but as it is not the way it is usually handled in wine, I would like to see a comment. Perhaps AJ prefers cyclic buffers or throw-away heaps. Or perhaps he doesn't care how it is done in test cases.
But in my opinion, testing of the reaction to contract violation should not go into passing somehow valid-looking pointers, that do not point to anything sensible. And the main point was, that I really don't get why it must be a heap pointer. The an LPDWORD on the stack would do as well as allocating sizeof(LPDWORD) area.
I've been checking how this is done in other tests, and it's particularly interesting how they do it in ddraw/tests/refcount.c:81: hr = IDirectDraw7_CreatePalette(DDraw7, DDPCAPS_ALLOW256 | DDPCAPS_8BIT, Table, &palette, (void *) 0xdeadbeef);
I don't think this line is good style, but I still like it more than your program, as this will definitely crash if the pointer is derefenced, while your variant makes the function derefencing the pointer accessing uninitialized data.
This is the only example in the current code. I could either do it that way (not correct, but simple), or use some IUnknown interface. I tried to get the IUnknown of the classes I have access to, DirectPlay or DirectPlayLobby, but the current implementation is not able to return an interface for IID_IUnknown (it just fails with E_NOINTERFACE),
Thats bad. Every COM object must return a valid interface pointer for IID_IUnknown. And even worse: The IID_IUnknown pointer *has* to be stable. So if you do the following (pretend that CreateFooBar(REFIID, DWORD, IUnknown*) is a factory function:
IFooBar *fb1, *fb2; IUnknown *unk1, *unk2; fb1 = CreateFooBar(IID_IFooBar, 0, NULL); IFooBar_QueryInterface(fb1, IID_IUnknown, &unk1); IFooBar_QueryInterface(fb1, IID_IFooBar, (IUnknown*)&fb2); IFooBar_Release(fb1); IFooBar_QueryInterface(fb2, IID_IUnknown, &unk2); IFooBar_Release(fb2); ok(unk1 == unk2, "COM violation: IUnknown interface must be stable!"); IUnknown_Release(unk1); IUnknown_Release(unk2);
The COM specification guarantees the test to succeed, independent of what type IFooBar has! DirectPlay currently has no permanent COM object, but creates a new object on the fly in QueryInterface; this is needed to support the different vtables.
so I would have to fix that too (and I'm not very sure about how to do it, without breaking anything. Would returning any of the existing interfaces that have CreateInstance, AddRef and Release, be correct?
If they were permanent, it would be OK. All COM interfaces are derived from IUnknown, and can thus be used as IUnknown. The only catch is the object lifetime uniqueness of the IUnknown interface.
This seems to be done a lot in other dlls)
For ddraw for example, it is correct. The IDirectDrawImpl object is live until all references to it (including the IUnknown one) are gone.
Regards, Michael Karcher
On 7/11/08, Michael Karcher wine@mkarcher.dialup.fu-berlin.de wrote:
Am Donnerstag, den 10.07.2008, 22:04 +0300 schrieb Ismael Barros:
I've been checking how this is done in other tests, and it's particularly interesting how they do it in ddraw/tests/refcount.c:81: hr = IDirectDraw7_CreatePalette(DDraw7, DDPCAPS_ALLOW256 | DDPCAPS_8BIT, Table, &palette, (void *) 0xdeadbeef);
I don't think this line is good style, but I still like it more than your program, as this will definitely crash if the pointer is derefenced, while your variant makes the function derefencing the pointer accessing uninitialized data.
If it's not that bad, would it be okay to leave it with the deadbeef solution, with a TODO comment to do it the right way later? Right now I would like to focus on other functionality (correct networking etc), and maybe leave that kind of details for after GSoC. From a pragmatic point of view, I doubt many games make use of theese COM features, while they still can't properly enumerate service providers.
This is the only example in the current code. I could either do it that way (not correct, but simple), or use some IUnknown interface. I tried to get the IUnknown of the classes I have access to, DirectPlay or DirectPlayLobby, but the current implementation is not able to return an interface for IID_IUnknown (it just fails with E_NOINTERFACE),
Thats bad. Every COM object must return a valid interface pointer for IID_IUnknown. And even worse: The IID_IUnknown pointer *has* to be stable. So if you do the following (pretend that CreateFooBar(REFIID, DWORD, IUnknown*) is a factory function:
IFooBar *fb1, *fb2; IUnknown *unk1, *unk2; fb1 = CreateFooBar(IID_IFooBar, 0, NULL); IFooBar_QueryInterface(fb1, IID_IUnknown, &unk1); IFooBar_QueryInterface(fb1, IID_IFooBar, (IUnknown*)&fb2); IFooBar_Release(fb1); IFooBar_QueryInterface(fb2, IID_IUnknown, &unk2); IFooBar_Release(fb2); ok(unk1 == unk2, "COM violation: IUnknown interface must be stable!"); IUnknown_Release(unk1); IUnknown_Release(unk2);
The COM specification guarantees the test to succeed, independent of what type IFooBar has! DirectPlay currently has no permanent COM object, but creates a new object on the fly in QueryInterface; this is needed to support the different vtables.
Great input, I'll bear it in mind :)
Am Freitag, den 11.07.2008, 18:11 +0300 schrieb Ismael Barros:
On 7/11/08, Michael Karcher wine@mkarcher.dialup.fu-berlin.de wrote: If it's not that bad, would it be okay to leave it with the deadbeef solution, with a TODO comment to do it the right way later? Right now I would like to focus on other functionality (correct networking etc), and maybe leave that kind of details for after GSoC. From a pragmatic point of view, I doubt many games make use of theese COM features, while they still can't properly enumerate service providers.
Probably no one ever calls DirectPlayCreate with unkOuter != NULL, as it is documented to not work. You may well postpone this check, it seems to be academic. On the other hand, it is quite easy. I have attached a patch to make the DirectDrawPalette creation test use a real IUnknown. You can copy that, if you like.
Great input, I'll bear it in mind :)
That's what wine-devel is for.
Regards, Michael Karcher
-
Francois Gouget -
Ismael Barros -
James Hawkins -
Kai Blin -
Michael Karcher