[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: [1/2] imagehlp/tests: Add test for BindImageEx (try 12)
From:       Bernhard Reiter <ockham () raz ! or ! at>
Date:       2014-07-25 16:22:53
Message-ID: 53D2845D.7000305 () raz ! or ! at
[Download RAW message or body]

Am 2014-07-24 um 18:33 schrieb Alexandre Julliard:
> Bernhard Reiter <ockham@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


[prev in list] [next in list] [prev in thread] [next in thread]