Re: [1/2] imagehlp: Implement (parts of) BindImageEx (try 10)

[Jacek Caban]

Hi Bernhard,

On 06/30/14 19:15, Bernhard Reiter wrote:
> diff --git a/dlls/imagehlp/tests/image.c b/dlls/imagehlp/tests/image.c
> index e008b0b..d93130c 100644
> --- a/dlls/imagehlp/tests/image.c
> +++ b/dlls/imagehlp/tests/image.c
> @@ -31,6 +31,9 @@
>  static HMODULE hImageHlp;
>
> static BOOL (WINAPI *pImageGetDigestStream)(HANDLE, DWORD, DIGEST_FUNCTION, DIGEST_HANDLE);
> +static BOOL (WINAPI *pBindImageEx)(DWORD Flags, PCSTR ImageName,
> +                                   PCSTR DllPath, PCSTR SymbolPath,
> +                                   PIMAGEHLP_STATUS_ROUTINE StatusRoutine);
>
> /* minimal PE file image */
>  #define VA_START 0x400000
> @@ -149,6 +152,9 @@ struct expected_update_accum
>      BOOL  todo;
>  };
>
> +static int status_routine_called[14] = {0};
> +
> +
>  static BOOL WINAPI accumulating_stream_output(DIGEST_HANDLE handle, BYTE *pb,
>   DWORD cb)
>  {
> @@ -273,6 +279,52 @@ static void update_checksum(void)
>      bin.nt_headers.OptionalHeader.CheckSum = sum;
>  }
>
> +BOOL WINAPI TestingStatusRoutine(IMAGEHLP_STATUS_REASON reason, PSTR ImageName, PSTR DllName,
> +                                 ULONG_PTR Va, ULONG_PTR Parameter)
> +{

Make it static.
> +    char kernel32_path[MAX_PATH];
> +
> +    status_routine_called[reason]++;
> +
> +    todo_wine ok((reason == BindImportModule) || (reason == BindImportProcedure) ||
> +                 (reason == BindForwarderNOT), "expected reason to be one of BindImportModule, "
> +                 "BindImportProcedure, or BindForwarderNOT, got %d\n", reason);

How about moving that to the switch that you need anyway?
> +
> +    switch(reason)
> +    {
> +        case BindOutOfMemory:
> +        case BindRvaToVaFailed:
> +        case BindNoRoomInImage:
> +        case BindImportModuleFailed:
> +        case BindImportProcedureFailed:
> +            break;

How about

default: ok(0, "...");
> +
> +        case BindImportModule:
> +            ok(!strcmp(DllName, "KERNEL32.DLL"), "expected DllName to be KERNEL32.DLL, got %s\n",
> +               DllName);
> +            break;
> +
> +        case BindImportProcedure:
> +            GetSystemDirectoryA(kernel32_path, MAX_PATH);
> +            strcat(kernel32_path, "\\KERNEL32.DLL");
> +            ok(!lstrcmpiA(DllName, kernel32_path), "expected DllName to be %s, got %s\n",
> +               kernel32_path, DllName);
> +            ok(!strcmp((LPSTR)Parameter, "ExitProcess"),
> +               "expected Parameter to be ExitProcess, got %s\n", (LPSTR)Parameter);
> +            break;
> +
> +        case BindForwarder:
> +        case BindForwarderNOT:
> +        case BindImageModified:
> +        case BindExpandFileHeaders:
> +        case BindImageComplete:
> +        case BindMismatchedSymbols:
> +        case BindSymbolsNotUpdated:
> +            break;
> +    }

Same here. You really need cases for expected values.
> +    return TRUE;
> +}
> +
>  static void test_get_digest_stream(void)
>  {
>      BOOL ret;
> @@ -329,6 +381,54 @@ static void test_get_digest_stream(void)
>      DeleteFileA(temp_file);
>  }
>
> +static void test_bind_image_ex(void)
> +{
> +    BOOL ret;
> +    HANDLE file;
> +    char temp_file[MAX_PATH];
> +    DWORD count;
> +
> +    /* Call with a non-existant file */
> +    SetLastError(0xdeadbeef);
> +    ret = pBindImageEx(BIND_NO_BOUND_IMPORTS | BIND_NO_UPDATE | BIND_ALL_IMAGES, temp_file, NULL,
> +                       NULL, (void *)TestingStatusRoutine);
> +    todo_wine ok(!ret && ((GetLastError() == ERROR_FILE_NOT_FOUND) ||
> +                 (GetLastError() == ERROR_INVALID_PARAMETER)),
> +                 "expected ERROR_FILE_NOT_FOUND or ERROR_INVALID_PARAMETER, got %d\n",
> +                 GetLastError());
> +
> +    file = create_temp_file(temp_file);
> +    if (file == INVALID_HANDLE_VALUE)
> +    {
> +        skip("couldn't create temp file\n");
> +        return;
> +    }
> +
> +    WriteFile(file, &bin, sizeof(bin), &count, NULL);
> +    FlushFileBuffers(file);
> +    CloseHandle(file);
> +
> +    /* Finally, call with a proper PE file */
> +    SetLastError(0xdeadbeef);
> +    ret = pBindImageEx(BIND_NO_BOUND_IMPORTS | BIND_NO_UPDATE | BIND_ALL_IMAGES, temp_file, NULL,
> +                       NULL, (void *)TestingStatusRoutine);
> +    ok(ret, "BindImageEx failed: %d\n", GetLastError());
> +
> +    todo_wine ok(status_routine_called[BindImportModule] == 1,
> +                 "Expected StatusRoutine to be called once with reason BindImportModule, "
> +                 "but it was called %d times\n", status_routine_called[BindImportModule]);
> +
> +    /* Under wvistau64 (64 bit image), w2008s64 (64 bit image) and w7pro64 (64 bit image),
> +     * StatusRoutine is called with reason BindForwarderNOT instead of BindImportProcedure. */

Please avoid such comments. Testbot VM names are not related to the code
nor permanent.

Cheers,
Jacek