Re: [1/2] imagehlp/tests: Add test for BindImageEx (try 12)
Bernhard Reiter <ockham(a)raz.or.at> writes:
@@ -31,6 +31,8 @@ static HMODULE hImageHlp;
static BOOL (WINAPI *pImageGetDigestStream)(HANDLE, DWORD, DIGEST_FUNCTION, DIGEST_HANDLE); +static BOOL (WINAPI *pBindImageEx)(DWORD Flags, const char *ImageName, const char *DllPath, + const char *SymbolPath, void *StatusRoutine);
That's not the correct prototype.
+static BOOL WINAPI testing_status_routine(IMAGEHLP_STATUS_REASON reason, char *ImageName, + char *DllName, ULONG_PTR Va, ULONG_PTR Parameter) +{ + char kernel32_path[MAX_PATH]; + + status_routine_called[reason]++;
This will have nasty results if you get an unexpected code.
+ default: + ok(0, "expected reason to be one of BindImportModule, BindImportProcedure, or " + "BindForwarderNOT, got %d\n", reason);
Please simplify your error messages. In particular, this one would need to be changed every time the test is expanded, that's not a good idea.
+static void test_bind_image_ex(void) +{ + BOOL ret; + HANDLE file; + char temp_file[MAX_PATH] = "nonexistant.dll";
There's no reason to use a variable for this.
+ /* Under 64 bit images of Vista Ultimate, 2008 Server, and 7 Pro, StatusRoutine is called with + * reason BindForwarderNOT instead of BindImportProcedure. */ + todo_wine ok((status_routine_called[BindImportProcedure] == 1) || + (status_routine_called[BindForwarderNOT] == 1), + "Expected StatusRoutine to be called once with reason BindImportProcedure or " + "BindForwarderNOT, but it was called %d and %d times, respectively\n", + status_routine_called[BindImportProcedure], status_routine_called[BindForwarderNOT]);
You'd most likely want the test to depend on 32/64-bit instead of always accepting both. -- Alexandre Julliard julliard(a)winehq.org
Am 2014-07-24 um 18:33 schrieb Alexandre Julliard:
Bernhard Reiter <ockham(a)raz.or.at> writes:
@@ -31,6 +31,8 @@ static HMODULE hImageHlp;
static BOOL (WINAPI *pImageGetDigestStream)(HANDLE, DWORD, DIGEST_FUNCTION, DIGEST_HANDLE); +static BOOL (WINAPI *pBindImageEx)(DWORD Flags, const char *ImageName, const char *DllPath, + const char *SymbolPath, void *StatusRoutine);
That's not the correct prototype.
Meaning I should change the const char*s to char*s (as MSDN says PSTR)? I was looking at the BindImageEx stub in dlls/imagehlp/modify.c, and that had PCSTRs. (Or do you mean using PIMAGEHLP_STATUS_ROUTINE instead of void* for StatusRoutine? Because that would require me to typecast testing_status_routine to that when invoking pBindImageEx, wouldn't it?)
+static BOOL WINAPI testing_status_routine(IMAGEHLP_STATUS_REASON reason, char *ImageName, + char *DllName, ULONG_PTR Va, ULONG_PTR Parameter) +{ + char kernel32_path[MAX_PATH]; + + status_routine_called[reason]++;
This will have nasty results if you get an unexpected code.
Okay, I'll add checks if reason is in the expected range.
+ default: + ok(0, "expected reason to be one of BindImportModule, BindImportProcedure, or " + "BindForwarderNOT, got %d\n", reason);
Please simplify your error messages. In particular, this one would need to be changed every time the test is expanded, that's not a good idea.
I'm not exactly creative how to change this... would ok(0, "testing_status_routine got wrong reason %d\n", reason); be okay?
+static void test_bind_image_ex(void) +{ + BOOL ret; + HANDLE file; + char temp_file[MAX_PATH] = "nonexistant.dll";
There's no reason to use a variable for this.
But I need it later to retrieve the name of the file created by create_temp_file. Plus, if I change the const char*s in pBindImageEx's declaration to char*s, I will run into problems here if I pass a const char* to pBindImageEx, won't I? So is it okay to leave it as it is or am I overlooking something?
+ /* Under 64 bit images of Vista Ultimate, 2008 Server, and 7 Pro, StatusRoutine is called with + * reason BindForwarderNOT instead of BindImportProcedure. */ + todo_wine ok((status_routine_called[BindImportProcedure] == 1) || + (status_routine_called[BindForwarderNOT] == 1), + "Expected StatusRoutine to be called once with reason BindImportProcedure or " + "BindForwarderNOT, but it was called %d and %d times, respectively\n", + status_routine_called[BindImportProcedure], status_routine_called[BindForwarderNOT]);
You'd most likely want the test to depend on 32/64-bit instead of always accepting both.
I had #if defined(_WIN64) macros there earlier, but the behavior isn't consistent: Windows 8 gives BindImportProcedure also with the 64 bit image. (I tried it with test job 7953, if that can be retrieved somehow. Could that BindForwarderNOT be due to a bug in WoW that was fixed in Windows 8?) I asked on #winehackers if I should add a check for the Windows version, but people suggested not to. What's your stance on this? Bernhard
Okay, as there hasn't been any response, I've tried to fix these issues as well as I could on my own. Looking forward to comments! Bernhard Am 2014-07-25 um 18:22 schrieb Bernhard Reiter:
Am 2014-07-24 um 18:33 schrieb Alexandre Julliard:
Bernhard Reiter <ockham(a)raz.or.at> writes:
@@ -31,6 +31,8 @@ static HMODULE hImageHlp;
static BOOL (WINAPI *pImageGetDigestStream)(HANDLE, DWORD, DIGEST_FUNCTION, DIGEST_HANDLE); +static BOOL (WINAPI *pBindImageEx)(DWORD Flags, const char *ImageName, const char *DllPath, + const char *SymbolPath, void *StatusRoutine);
That's not the correct prototype.
Meaning I should change the const char*s to char*s (as MSDN says PSTR)? I was looking at the BindImageEx stub in dlls/imagehlp/modify.c, and that had PCSTRs.
(Or do you mean using PIMAGEHLP_STATUS_ROUTINE instead of void* for StatusRoutine? Because that would require me to typecast testing_status_routine to that when invoking pBindImageEx, wouldn't it?)
+static BOOL WINAPI testing_status_routine(IMAGEHLP_STATUS_REASON reason, char *ImageName, + char *DllName, ULONG_PTR Va, ULONG_PTR Parameter) +{ + char kernel32_path[MAX_PATH]; + + status_routine_called[reason]++;
This will have nasty results if you get an unexpected code.
Okay, I'll add checks if reason is in the expected range.
+ default: + ok(0, "expected reason to be one of BindImportModule, BindImportProcedure, or " + "BindForwarderNOT, got %d\n", reason);
Please simplify your error messages. In particular, this one would need to be changed every time the test is expanded, that's not a good idea.
I'm not exactly creative how to change this... would
ok(0, "testing_status_routine got wrong reason %d\n", reason);
be okay?
+static void test_bind_image_ex(void) +{ + BOOL ret; + HANDLE file; + char temp_file[MAX_PATH] = "nonexistant.dll";
There's no reason to use a variable for this.
But I need it later to retrieve the name of the file created by create_temp_file. Plus, if I change the const char*s in pBindImageEx's declaration to char*s, I will run into problems here if I pass a const char* to pBindImageEx, won't I?
So is it okay to leave it as it is or am I overlooking something?
+ /* Under 64 bit images of Vista Ultimate, 2008 Server, and 7 Pro, StatusRoutine is called with + * reason BindForwarderNOT instead of BindImportProcedure. */ + todo_wine ok((status_routine_called[BindImportProcedure] == 1) || + (status_routine_called[BindForwarderNOT] == 1), + "Expected StatusRoutine to be called once with reason BindImportProcedure or " + "BindForwarderNOT, but it was called %d and %d times, respectively\n", + status_routine_called[BindImportProcedure], status_routine_called[BindForwarderNOT]);
You'd most likely want the test to depend on 32/64-bit instead of always accepting both.
I had #if defined(_WIN64) macros there earlier, but the behavior isn't consistent: Windows 8 gives BindImportProcedure also with the 64 bit image. (I tried it with test job 7953, if that can be retrieved somehow. Could that BindForwarderNOT be due to a bug in WoW that was fixed in Windows 8?) I asked on #winehackers if I should add a check for the Windows version, but people suggested not to. What's your stance on this?
Bernhard
participants (2)
-
Alexandre Julliard -
Bernhard Reiter