Re: [PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

[Sebastian Lackner]

On 08.07.2016 00:16, John Sheu wrote:
> Signed-off-by: John Sheu <sheu(a)google.com>
> ---
>  dlls/kernel32/tests/fiber.c | 291 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 258 insertions(+), 33 deletions(-)
>
> diff --git a/dlls/kernel32/tests/fiber.c b/dlls/kernel32/tests/fiber.c
> index 1e4ca66..4f61f80 100644
> --- a/dlls/kernel32/tests/fiber.c
> +++ b/dlls/kernel32/tests/fiber.c
> @@ -33,9 +33,13 @@ static BOOL (WINAPI *pFlsFree)(DWORD);
>  static PVOID (WINAPI *pFlsGetValue)(DWORD);
>  static BOOL (WINAPI *pFlsSetValue)(DWORD,PVOID);
>
> -static LPVOID fibers[2];
> +static LPVOID fibers[3];
>  static BYTE testparam = 185;
> -static WORD cbCount;
> +static DWORD fls_index_to_set = FLS_OUT_OF_INDEXES;
> +static PVOID fls_value_to_set = NULL;
> +
> +static int fiberCount = 0;
> +static int cbCount = 0;
>
> static VOID init_funcs(void)
>  {
> @@ -59,15 +63,44 @@ static VOID init_funcs(void)
>
> static VOID WINAPI FiberLocalStorageProc(PVOID lpFlsData)
>  {
> +    ok(lpFlsData == fls_value_to_set,
> +       "FlsData expected not to be changed, value is %p, expected %p\n",
> +       lpFlsData, fls_value_to_set);
>      cbCount++;
> -    ok(lpFlsData == (PVOID) 1587, "FlsData expected not to be changed\n");
>  }
>
> static VOID WINAPI FiberMainProc(LPVOID lpFiberParameter)
>  {
>      BYTE *tparam = (BYTE *)lpFiberParameter;
> -    cbCount++;
> +    fiberCount++;
>      ok(*tparam == 185, "Parameterdata expected not to be changed\n");
> +    if (fls_index_to_set != FLS_OUT_OF_INDEXES)
> +    {
> +        if (!pFlsSetValue || !pFlsGetValue)
> +        {
> +            win_skip( "Fiber Local Storage not supported\n" );

You can get rid of this check, fls_index_to_set should not be set
when Fiber Local Storage is not supported.
> +        }
> +        else
> +        {
> +            PVOID pret;

Usually "void *" types and variable names without "p" prefix are preferred.
> +            BOOL bret;
> +
> +            pret = pFlsGetValue(fls_index_to_set);
> +            ok(pret == NULL, "FlsGetValue returned %p, expected NULL\n", pret);
> +
> +            /* Set the FLS value */
> +            SetLastError( 0xdeadbeef );

You can remove this line when you are not testing the error value.
> +            bret = pFlsSetValue(fls_index_to_set, fls_value_to_set);
> +            ok(bret, "FlsSetValue failed with error %u\n", GetLastError());
> +
> +            /* Verify that FlsGetValue retrieves the value set by FlsSetValue */
> +            SetLastError( 0xdeadbeef );
> +            pret = pFlsGetValue(fls_index_to_set);
> +            ok(pret == fls_value_to_set,
> +               "FlsGetValue returned %p, expected %p\n", pret, fls_value_to_set);
> +            ok(GetLastError() == ERROR_SUCCESS, "FlsGetValue error %u\n", GetLastError());
> +        }
> +    }
>      pSwitchToFiber(fibers[0]);
>  }
>
> @@ -110,9 +143,23 @@ static void test_ConvertFiberToThread(void)
>      }
>  }
>
> +static void test_IsThreadAFiber(BOOL is_fiber)
> +{
> +    if (pIsThreadAFiber)
> +    {
> +        BOOL ret = pIsThreadAFiber();
> +        ok(ret == is_fiber, "IsThreadAFiber reported %d, expected %d\n", ret, is_fiber);
> +    }
> +    else
> +    {
> +        win_skip( "IsThreadAFiber not present\n" );
> +        return;

return at the end of a function is unnecessary. Also, I'm not sure if it makes much sense
to have this as a separate function. You could probably just inline it in this case.
> +    }
> +}
> +
>  static void test_FiberHandling(void)
>  {
> -    cbCount = 0;
> +    fiberCount = 0;
>      fibers[0] = pCreateFiber(0,FiberMainProc,&testparam);
>      ok(fibers[0] != 0, "CreateFiber failed with error %d\n", GetLastError());
>      pDeleteFiber(fibers[0]);
> @@ -124,12 +171,11 @@ static void test_FiberHandling(void)
>      else
>          test_ConvertThreadToFiber();
>
> -
>      fibers[1] = pCreateFiber(0,FiberMainProc,&testparam);
>      ok(fibers[1] != 0, "CreateFiber failed with error %d\n", GetLastError());
>
> pSwitchToFiber(fibers[1]);
> -    ok(cbCount == 1, "Wrong callback count: %d\n", cbCount);
> +    ok(fiberCount == 1, "Wrong fiber count: %d\n", fiberCount);
>      pDeleteFiber(fibers[1]);
>
> if (!pCreateFiberEx)
> @@ -143,46 +189,72 @@ static void test_FiberHandling(void)
>      ok(fibers[1] != 0, "CreateFiberEx failed with error %d\n", GetLastError());
>
> pSwitchToFiber(fibers[1]);
> -    ok(cbCount == 2, "Wrong callback count: %d\n", cbCount);
> +    ok(fiberCount == 2, "Wrong fiber count: %d\n", fiberCount);
>      pDeleteFiber(fibers[1]);
>
> -    if (!pIsThreadAFiber)
> -    {
> -        win_skip( "IsThreadAFiber not present\n" );
> -        return;
> -    }
> -
> -    ok(pIsThreadAFiber(), "IsThreadAFiber reported FALSE\n");
> +    test_IsThreadAFiber(TRUE);
>      test_ConvertFiberToThread();
> -    ok(!pIsThreadAFiber(), "IsThreadAFiber reported TRUE\n");
> +    test_IsThreadAFiber(FALSE);
>  }
>
> -static void test_FiberLocalStorage(PFLS_CALLBACK_FUNCTION cbfunc)
> +static void test_FiberLocalStorage(void)
>  {
> -    DWORD fls;
> +    DWORD fls, fls_2;
>      BOOL ret;
> -    PVOID val = (PVOID) 1587;
> +    PVOID val;

"void *" would be preferred here.
> -    if (!pFlsAlloc)
> +    if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree)
>      {
>          win_skip( "Fiber Local Storage not supported\n" );
>          return;
>      }
> -    cbCount = 0;
>
> -    fls = pFlsAlloc(cbfunc);
> -    ok(fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %d\n", GetLastError());
> +    /* Test an unallocated index
> +     * FlsFree should fail
> +     * FlsGetValue and FlsSetValue should succeed
> +     */
> +    SetLastError( 0xdeadbeef );
> +    ret = pFlsFree( 127 );
> +    ok( !ret, "freeing fls index 127 (unallocated) succeeded\n" );
> +    ok( GetLastError() == ERROR_INVALID_PARAMETER,
> +        "freeing fls index 127 (unallocated) wrong error %u\n", GetLastError() );
>
> -    ret = pFlsSetValue(fls, val);
> -    ok(ret, "FlsSetValue failed\n");
> -    ok(val == pFlsGetValue(fls), "FlsGetValue failed\n");
> +    SetLastError( 0xdeadbeef );

This is unnecessary if you don't test the error.
> +    val = pFlsGetValue( 127 );
> +    ok( val == NULL,
> +        "getting fls index 127 (unallocated) failed with error %u\n", GetLastError() );
> +
> +    SetLastError( 0xdeadbeef );

Same here.
> +    ret = pFlsSetValue( 127, (PVOID) 0x217 );

"(void *)0x217" would be preferred here (and at a couple of places below).
> +    ok( ret, "setting fls index 127 (unallocated) failed with error %u\n", GetLastError() );
> +
> +    SetLastError( 0xdeadbeef );
> +    val = pFlsGetValue( 127 );
> +    ok( val == (PVOID) 0x217, "fls index 127 (unallocated) wrong value %p\n", val );
> +    ok( GetLastError() == ERROR_SUCCESS,
> +        "getting fls index 127 (unallocated) failed with error %u\n", GetLastError() );
> +
> +    /* FlsFree, FlsGetValue, and FlsSetValue out of bounds should return
> +     * ERROR_INVALID_PARAMETER
> +     */
> +    SetLastError( 0xdeadbeef );
> +    ret = pFlsFree( 128 );
> +    ok( !ret, "freeing fls index 128 (out of bounds) succeeded\n" );
> +    ok( GetLastError() == ERROR_INVALID_PARAMETER,
> +        "freeing fls index 128 (out of bounds) wrong error %u\n", GetLastError() );
>
> -    ret = pFlsFree(fls);
> -    ok(ret, "FlsFree failed\n");
> -    if (cbfunc)
> -        todo_wine ok(cbCount == 1, "Wrong callback count: %d\n", cbCount);
> +    SetLastError( 0xdeadbeef );
> +    ret = pFlsSetValue( 128, (PVOID) 0x217 );
> +    ok( !ret, "setting fls index 128 (out of bounds) succeeded\n" );
> +    ok( GetLastError() == ERROR_INVALID_PARAMETER,
> +        "setting fls index 128 (out of bounds) wrong error %u\n", GetLastError() );
> +
> +    SetLastError( 0xdeadbeef );
> +    val = pFlsGetValue( 128 );

It would make sense to test "val" aswell.
> +    ok( GetLastError() == ERROR_INVALID_PARAMETER,
> +        "setting fls index 128 (out of bounds) wrong error %u\n", GetLastError() );

The function above is no set function.
> -    /* test index 0 */
> +    /* Test index 0 */
>      SetLastError( 0xdeadbeef );
>      val = pFlsGetValue( 0 );
>      ok( !val, "fls index 0 set to %p\n", val );
> @@ -196,6 +268,7 @@ static void test_FiberLocalStorage(PFLS_CALLBACK_FUNCTION cbfunc)
>      ok( !val, "fls index 0 wrong value %p\n", val );
>      ok( GetLastError() == ERROR_INVALID_PARAMETER, "setting fls index wrong error %u\n", GetLastError() );
>
> +    /* Test creating an FLS index */
>      fls = pFlsAlloc( NULL );
>      ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed\n" );
>      ok( fls != 0, "fls index 0 allocated\n" );
> @@ -205,11 +278,162 @@ static void test_FiberLocalStorage(PFLS_CALLBACK_FUNCTION cbfunc)
>      ok( ret, "setting fls index %u failed\n", fls );
>      val = pFlsGetValue( fls );
>      ok( val == (void *)0xdeadbeef, "fls index %u wrong value %p\n", fls, val );
> +    ok( GetLastError() == ERROR_SUCCESS,
> +        "getting fls index %u failed with error %u\n", fls, GetLastError() );
>      pFlsFree( fls );
> +
> +    /* Undefined behavior: verify the value is NULL after it the slot is freed */
> +    val = pFlsGetValue( fls );
> +    ok( val == NULL, "fls index %u wrong value %p\n", fls, val );
> +    ok( GetLastError() == ERROR_SUCCESS,
> +        "getting fls index %u failed with error %u\n", fls, GetLastError() );
> +
> +    /* Undefined behavior: verify the value is settable after the slot is freed */
>      ret = pFlsSetValue( fls, (void *)0xdeadbabe );
>      ok( ret, "setting fls index %u failed\n", fls );
>      val = pFlsGetValue( fls );
>      ok( val == (void *)0xdeadbabe, "fls index %u wrong value %p\n", fls, val );
> +
> +    /* Try to create the same FLS index again, and verify that is initialized to NULL */
> +    fls_2 = pFlsAlloc( NULL );
> +    ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed\n" );
> +    /* If this fails it is not an API error, but the test will be inconclusive */
> +    ok( fls_2 == fls, "different FLS index allocated, was %u, now %u\n", fls, fls_2 );
> +    val = pFlsGetValue( fls_2 );
> +    ok( val == NULL, "fls index %u wrong value %p\n", fls, val );
> +    ok( GetLastError() == ERROR_SUCCESS,
> +        "getting fls index %u failed with error %u\n", fls_2, GetLastError() );

A SetLastError() call is missing here, otherwise this test is not really convincing.
> +    pFlsFree( fls_2 );
> +}
> +
> +static void test_FiberLocalStorageCallback(PFLS_CALLBACK_FUNCTION cbfunc) {

All the other functions have the "{" on the next line.
> +    DWORD fls;
> +    BOOL ret;
> +    PVOID val, val2;
> +
> +    if (!pFlsAlloc || !pFlsSetValue || !pFlsGetValue || !pFlsFree)
> +    {
> +        win_skip( "Fiber Local Storage not supported\n" );
> +        return;
> +    }
> +
> +    /* Test that the callback is executed */
> +    SetLastError( 0xdeadbeef );

This is unnecessary.
> +    cbCount = 0;
> +    fls = pFlsAlloc( cbfunc );
> +    ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() );
> +
> +    SetLastError( 0xdeadbeef );

This also.
> +    val = (PVOID) 0x1587;
> +    fls_value_to_set = val;
> +    ret = pFlsSetValue( fls, val );
> +    ok(ret, "FlsSetValue failed with error %u\n", GetLastError() );
> +
> +    val2 = pFlsGetValue( fls );
> +    ok(val == val2, "FlsGetValue returned %p, expected %p\n", val2, val);
> +
> +    SetLastError( 0xdeadbeef );

Same here.
> +    ret = pFlsFree( fls );
> +    ok(ret, "FlsFree failed with error %u\n", GetLastError() );
> +    todo_wine
> +    {
> +        ok( cbCount == 1, "Wrong callback count: %d\n", cbCount );
> +    }
> +
> +    /* Test that callback is not executed if value is NULL */
> +    SetLastError( 0xdeadbeef );

Same here.
> +    cbCount = 0;
> +    fls = pFlsAlloc( cbfunc );
> +    ok( fls != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %u\n", GetLastError() );
> +
> +    SetLastError( 0xdeadbeef );

Same here.
> +    ret = pFlsSetValue( fls, NULL );
> +    ok( ret, "FlsSetValue failed with error %u\n", GetLastError() );
> +
> +    SetLastError( 0xdeadbeef );

Same here.
> +    pFlsFree( fls );
> +    ok( ret, "FlsFree failed with error %u\n", GetLastError() );
> +    ok( cbCount == 0, "Wrong callback count: %d\n", cbCount );
> +}
> +
> +static void test_FiberLocalStorageWithFibers(PFLS_CALLBACK_FUNCTION cbfunc) {
> +    PVOID val1 = (PVOID) 0x314;
> +    PVOID val2 = (PVOID) 0x152;
> +    BOOL ret;
> +
> +    if (!pConvertThreadToFiber || !pSwitchToFiber || !pDeleteFiber || !pConvertFiberToThread)
> +    {
> +        win_skip( "Fibers not supported\n" );
> +        return;
> +    }
> +    if (!pFlsAlloc || !pFlsFree || !pFlsSetValue || !pFlsGetValue)
> +    {
> +        win_skip( "Fiber Local Storage not supported\n" );
> +        return;
> +    }
> +
> +
> +    fls_index_to_set = pFlsAlloc(cbfunc);
> +    ok(fls_index_to_set != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %d\n", GetLastError());

Not really critical, but in the code above you used %u for GetLastError values.
> +
> +    test_ConvertThreadToFiber();
> +
> +    fiberCount = 0;
> +    cbCount = 0;
> +    fibers[1] = pCreateFiber(0,FiberMainProc,&testparam);
> +    fibers[2] = pCreateFiber(0,FiberMainProc,&testparam);
> +    ok(fibers[1] != 0, "CreateFiber failed with error %d\n", GetLastError());
> +    ok(fibers[2] != 0, "CreateFiber failed with error %d\n", GetLastError());

CreateFiber returns a pointer, so it would be better to compare against NULL.
> +    ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount);
> +    ok(cbCount == 0, "Wrong callback count: %d\n", cbCount);
> +
> +    fiberCount = 0;
> +    cbCount = 0;
> +    fls_value_to_set = val1;
> +    pSwitchToFiber(fibers[1]);
> +    ok(fiberCount == 1, "Wrong fiber count: %d\n", fiberCount);
> +    ok(cbCount == 0, "Wrong callback count: %d\n", cbCount);
> +
> +    fiberCount = 0;
> +    cbCount = 0;
> +    fls_value_to_set = val2;
> +    pSwitchToFiber(fibers[2]);
> +    ok(fiberCount == 1, "Wrong fiber count: %d\n", fiberCount);
> +    ok(cbCount == 0, "Wrong callback count: %d\n", cbCount);
> +
> +    fls_value_to_set = val2;
> +    ret = pFlsSetValue(fls_index_to_set, fls_value_to_set);
> +    ok(ret, "FlsSetValue failed\n");
> +    ok(val2 == pFlsGetValue(fls_index_to_set), "FlsGetValue failed\n");
> +
> +    fiberCount = 0;
> +    cbCount = 0;
> +    fls_value_to_set = val1;
> +    pDeleteFiber(fibers[1]);
> +    ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount);
> +    todo_wine
> +    {
> +        ok(cbCount == 1, "Wrong callback count: %d\n", cbCount);
> +    }
> +
> +    fiberCount = 0;
> +    cbCount = 0;
> +    fls_value_to_set = val2;
> +    pFlsFree(fls_index_to_set);
> +    ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount);
> +    todo_wine
> +    {
> +        ok(cbCount == 2, "Wrong callback count: %d\n", cbCount);
> +    }
> +
> +    fiberCount = 0;
> +    cbCount = 0;
> +    fls_value_to_set = val1;
> +    pDeleteFiber(fibers[2]);
> +    ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount);
> +    ok(cbCount == 0, "Wrong callback count: %d\n", cbCount);
> +
> +    test_ConvertFiberToThread();
>  }
>
> START_TEST(fiber)
> @@ -223,6 +447,7 @@ START_TEST(fiber)
>      }
>
> test_FiberHandling();
> -    test_FiberLocalStorage(NULL);
> -    test_FiberLocalStorage(FiberLocalStorageProc);
> +    test_FiberLocalStorage();
> +    test_FiberLocalStorageCallback(FiberLocalStorageProc);
> +    test_FiberLocalStorageWithFibers(FiberLocalStorageProc);
>  }

[PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

[John Sheu]

Updating the patch now.  Note in particular with the SetLastError()
calls, that the error messages immediately after the API calls do
print the result of GetLastError(), so it would be good to return the
last error to a known state for that.

Thanks!
-John Sheu

[PATCH] kernel32/tests/fiber: Add tests for fiber-local storage.

[Sebastian Lackner]

On 11.07.2016 22:07, John Sheu wrote:
> Updating the patch now.  Note in particular with the SetLastError()
> calls, that the error messages immediately after the API calls do
> print the result of GetLastError(), so it would be good to return the
> last error to a known state for that.
>
> Thanks!
> -John Sheu

Usually SetLastError() is only used when the error code is tested explicitly,
not when the error is printed in an ok() message for debugging purposes. If
there are test failures, a developer will have to investigate what exactly is
going wrong anyway.