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

[André Hentschel]

Am 02.07.2016 um 04:41 schrieb John Sheu:
> Signed-off-by: John Sheu

Hi and welcome to Wine development!
May I ask if you could name an application which will benefit from your fixes?
Your tests currently fail on our testbot as you can see here: https://source.winehq.org/patches/
Some inline comments:
> 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" );
> -
>    }
> -
>    else
> -
>    {
> -
>        PVOID pret;
> -
>        BOOL bret;
> -
> -
>        pret = FlsGetValue(fls_index_to_set);
> -
>        ok(pret == NULL, "FlsGetValue returned %p, expected NULL\n", pret);
> -
> -
>        /* Set the FLS value */
> -
>        SetLastError( 0xdeadbeef );
> -
>        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 = FlsGetValue(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]);
>
> }

You are mixing tabs and spaces in FiberMainProc, spaces are preferred. Further you can avoid some line brakes in ok() calls, only wrap at around 100
> +static void test_FiberLocalStorageWithFibers(PFLS_CALLBACK_FUNCTION cbfunc) {
> - PVOID val1 = (PVOID) 0x314;
> - PVOID val2 = (PVOID) 0x152;
> - BOOL ret;
> -
> - if (!pConvertThreadToFiber || !pSwitchToFiber || !pDeleteFiber)
> - {
> -
>    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());
> -
> - fiberCount = 0;
> - cbCount = 0;
> - fibers[0] = pConvertThreadToFiber(&testparam);

Why do you convert the current thread into a fiber? It seems to me that is causing the crash on w2k3.
You could try to run that test in a seperate thread
> - 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());
> - 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);
> - }

You can make that a one liner:
todo_wine ok();

Hope that helps
Have fun!