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);
>  }