Re: [PATCH] kernel32: implement GetLogicalProcessorInformation

[Charles Davis]

Hi,

On Nov 24, 2011, at 4:21 AM, Claudio Fontana wrote:
> First implementation of GetLogicalProcessorInformation.
> Limitations: all logical processors are added to the same NUMA set,
> and all cores are added to the same package.
> Only the linux-specific helper function is implemented, so for now
> everybody else gets the fallback hard-coded results only.
> The fallback is also triggered when the OS-specific APIs fail.
>
> The linux-specific function is based on sysfs.
>
> The new OS-specific functions can be easily added as additional
> ifdefs by following the linux example.
>
> dlls/kernel32/process.c         |  350 ++++++++++++++++++++++++++++++++++++++-
> dlls/kernel32/tests/Makefile.in |    1 +
> dlls/kernel32/tests/glpi.c      |   82 +++++++++

You should really implement this in ntdll.NtQuerySystemInformation (information class SystemLogicalProcessorInformation). In general, kernel32 forwards most of its implementation to ntdll, which reduces code duplication.
> 3 files changed, 430 insertions(+), 3 deletions(-)
> create mode 100644 dlls/kernel32/tests/glpi.c

IMO, you should add the test to dlls/ntdll/tests/info.c, and test NtQuerySystemInformation() with info class SystemLogicalProcessorInformation.
> diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c
> index b6c0b9d..de39244 100644
> --- a/dlls/kernel32/process.c
> +++ b/dlls/kernel32/process.c
> @@ -51,6 +51,12 @@
>
> #include "ntstatus.h"
> #define WIN32_NO_STATUS
> -
>
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winerror.h"
> +#include "winnt.h"
> -
>
> #include "winternl.h"
> #include "kernel_private.h"
> #include "psapi.h"
> @@ -3705,14 +3711,352 @@ HANDLE WINAPI GetCurrentProcess(void)
>     return (HANDLE)~(ULONG_PTR)0;
> }
>
> +/***************
> -
> - glpi_impl_fb:
> -
> - a fallback implementation of GetLogicalProcessorInformation
> -
> - for the systems we do not support yet,
> -
> - or for when the OS-specific API fails.
> -
> - Same arguments and return value as glpi.
> - */

This comment should really come right before the function you're documenting. Also, it's formatted wrong. It should look more like this:

/***********************************************************************************
 *        logical_processor_info_fallback
 *
 * Fallback implementation of NtQuerySystemInformation() with info class
 * SystemLogicalProcessorInfo.
 */
> -
>
> +#undef SLPI
> +#ifdef NONAMELESSUNION
> +#define SLPI(buffer, idx) buffer[idx].DUMMYUNIONNAME
> +#else
> +#define SLPI(buffer, idx) buffer[idx]
> +#endif /* NONAMELESSUNION */

You could avoid the ifdef if you used the U() macro. In fact, why are you defining a macro at all? Just use U(buffer[idx]).
> -
>
> +static BOOL
> +glpi_impl_fb(PSYSTEM_LOGICAL_PROCESSOR_INFORMATION buffer, PDWORD pbuflen)

Surely you could use a more descriptive name. (See one example in my example doc comment.) This isn't 1978, you know.
> +{
> - int glpi_n = 5;
> - FIXME("(%p,%p): reporting hard coded information\n", buffer, pbuflen);
> -
> - if (*pbuflen < sizeof(*buffer) * glpi_n)
> - {
> - SetLastError(ERROR_INSUFFICIENT_BUFFER);
> - *pbuflen = sizeof(*buffer) * glpi_n;
> - return FALSE;
> - }
> -
> - buffer[0].ProcessorMask = (ULONG_PTR)0x01;
> - buffer[1].ProcessorMask = (ULONG_PTR)0x01;
> - buffer[2].ProcessorMask = (ULONG_PTR)0x01;
> - buffer[3].ProcessorMask = (ULONG_PTR)0x01;
> - buffer[4].ProcessorMask = (ULONG_PTR)0x01;

These casts aren't necessary.
> -
> - buffer[0].Relationship = RelationProcessorCore;
> - buffer[1].Relationship = RelationNumaNode;
> - buffer[2].Relationship = RelationCache;
> - buffer[3].Relationship = RelationCache;
> - buffer[4].Relationship = RelationProcessorPackage;
> -
> - SLPI(buffer, 0).ProcessorCore.Flags = (BYTE)0x00;
> - SLPI(buffer, 1).NumaNode.NodeNumber = (DWORD)0x00;

Neither are these.
> -
> - SLPI(buffer, 2).Cache.Level = (BYTE)0x01;
> - SLPI(buffer, 2).Cache.Associativity = (BYTE)0x08;
> - SLPI(buffer, 2).Cache.LineSize = (WORD)64;
> - SLPI(buffer, 2).Cache.Size = (DWORD)16 << 10; /* 16 KB */

Or these.
> - SLPI(buffer, 2).Cache.Type = CacheData;
> -
> - SLPI(buffer, 3).Cache.Level = (BYTE)0x02;
> - SLPI(buffer, 3).Cache.Associativity = (BYTE)0x08;
> - SLPI(buffer, 3).Cache.LineSize = (WORD)64;
> - SLPI(buffer, 3).Cache.Size = (DWORD)1024 << 10; /* 1024 KB */

Or these.
> - SLPI(buffer, 3).Cache.Type = CacheUnified;
> - /* no additional info for processor package at buffer[4] needed. */
> -
> - *pbuflen = sizeof(*buffer) * glpi_n;
> - return TRUE;
>
> +}
> -
>
> +#ifdef linux
> +/******************
> -
> - glpi_impl_linux:
> -
> - an implementation of GetLogicalProcessorInformation
> -
> - for linux sysfs.
> -
> - Returns: -1 on API failure (trigger fallback),
> -
> -
>      otherwise see glpi.
> - */

See above.
> -
>
> +#define SYS_CPU "/sys/devices/system/cpu/cpu"
> +#define SYS_CPU_MAX 63

I'd criticize your use of a static limit, if only Windows didn't have that limit ;). Actually, you can only have 32 processors on a 32-bit system because a ULONG_PTR is 32 bits there.
> +#define SYS_CPU_COREID "/topology/core_id"
> +#define SYS_CPU_PACKAGEID "/topology/physical_package_id"
> +#define SYS_CPU_CACHE "/cache/index"
> +#define SYS_CPU_CACHE_MAX 12
> +#define SYS_CPU_CACHE_LEVEL "/level"
> +#define SYS_CPU_CACHE_SIZE "/size"
> +#define SYS_CPU_CACHE_TYPE "/type"
> +#define SYS_CPU_CACHE_LINE "/coherency_line_size"
> +#define SYS_CPU_CACHE_ASSOC "/ways_of_associativity"
> -
>
> +static int glpi_impl_linux(SYSTEM_LOGICAL_PROCESSOR_INFORMATION *buffer,
> -
> 	   PDWORD pbuflen)
>
> +{
> - /* remember the glpi results, so that subsequent calls are faster */
> - static int n_cores = 0;
> - static int n_caches = 0;
> - static PSYSTEM_LOGICAL_PROCESSOR_INFORMATION cores = NULL;
> - static PSYSTEM_LOGICAL_PROCESSOR_INFORMATION caches = NULL;
> - static SYSTEM_LOGICAL_PROCESSOR_INFORMATION numa[1];
> - static SYSTEM_LOGICAL_PROCESSOR_INFORMATION package[1];
> -
> - if (!n_cores)
> - {
> - int n_cpus = 0; int cpu_i;
> - char path[PATH_MAX];
> -
> - TRACE("building new cached result.\n");
> -
> - for (n_cpus = 0; n_cpus < SYS_CPU_MAX; n_cpus++)
> - {
> -
>    snprintf(path, sizeof(path), SYS_CPU "%d", n_cpus);
> -
>    if (access(path, R_OK | X_OK) != 0)

Don't use access(2). There's a big fat warning on the man-page telling you not to use it because of a race condition. Another way to solve the problem you're trying to solve might be to scan the "/sys/devices/system/cpu/" directory for the number of entries using the POSIX readdir(3) interface. You could scan them all up front, or you could take each one as you go, re-allocating the table for each new entry.
> -
> break;
> - }
> -
> - TRACE("detected %d logical cpus.\n", n_cpus);
> - if (n_cpus < 1)
> - {
> -
>    TRACE("requesting fallback implementation.\n");
> -
>    return -1;
> - }
> -
> - cores = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*cores) * n_cpus);
> - caches = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*caches) * 1);
> -
> - for (cpu_i = 0; cpu_i < n_cpus; cpu_i++)
> - {
> -
>    FILE *f; int core_id; int i;
> -
>    snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_COREID, cpu_i);
> -
>    cores[n_cores].ProcessorMask = (ULONG_PTR)0x01 << cpu_i;

This cast might be needed if there are more than 32 CPUs, so that's OK.
> -
>    cores[n_cores].Relationship = RelationProcessorCore;
> -
> -
>    if (!(f = fopen(path, "r")) || fscanf(f, "%d", &core_id) != 1)
> -
>    {
> -
> /* topology info not available, provide default cpuid=cpu_i */
> -
> ERR("could not read cpu%d topology.\n", cpu_i);
> -
> SLPI(cores, n_cores).Reserved[0] = cpu_i;
> -
> n_cores++;
> -
> if (f)
> -
>     fclose(f);
> -
> continue; /* next cpu */
> -
>    }
> -
> -
>    /* normal sane case */
> -
>    fclose(f);
> -
>    /* store the core_id to catch cpus with same core_id */
> -
>    SLPI(cores, n_cores).Reserved[0] = core_id;
> -
> -
>    /* look for this core_id in previous elements */
> -
>    for (i = 0; i < n_cores; i++)
> -
> if (SLPI(cores, i).Reserved[0] == SLPI(cores, n_cores).Reserved[0])
> -
>     break;
> -
> -
>    if (i < n_cores)
> -
>    {
> -
> TRACE("found existing coreid %d for cpu%d.\n", core_id, cpu_i);
> -
> /* add this logical processor to the existing mask */
> -
> cores[i].ProcessorMask |= cores[n_cores].ProcessorMask;
> -
> /* do not advance n_cores: this element can be overwritten. */
> -
>    }
> -
>    else
> -
>    {
> -
> TRACE("found new coreid %d for cpu%d.\n", core_id, cpu_i);
> -
> -
> /* look for caches */
> -
> for (i = 0; i < SYS_CPU_CACHE_MAX; i++)
> -
> {
> -
>     unsigned long value;
> -
>     char string[80];
> -
> -
>     snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d",
> -
> 	     cpu_i, i);
> -
>     if (access(path, R_OK | X_OK) != 0)

Don't use access(2).
> -
> 	break; /* no more caches found. */
> -
> -
>     caches = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> -
> 			 caches, sizeof(*caches) * (n_caches + 1));
> -
> -
>     /* we keep the index into the cores array to set
> -
>        the right value at the end. */
> -
>     caches[n_caches].ProcessorMask = (ULONG_PTR)n_cores;

This cast, however, is definitely not needed.
> -
>     caches[n_caches].Relationship = RelationCache;
> -
> -
>     snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> -
> 	     SYS_CPU_CACHE_LEVEL, cpu_i, i);
> -
>     if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> -
> 	SLPI(caches, n_caches).Cache.Level = (BYTE)0x01;

Neither is this.
> -
>     else
> -
> 	SLPI(caches, n_caches).Cache.Level = (BYTE)value;
> -
>     if (f)
> -
> 	fclose(f);
> -
> -
>     snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> -
> 	     SYS_CPU_CACHE_ASSOC, cpu_i, i);
> -
>     if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> -
> 	SLPI(caches, n_caches).Cache.Associativity = 0x08;
> -
>     else
> -
> 	SLPI(caches, n_caches).Cache.Associativity = (BYTE)value;
> -
>     if (f)
> -
> 	fclose(f);
> -
> -
>     snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> -
> 	     SYS_CPU_CACHE_LINE, cpu_i, i);
> -
>     if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> -
> 	SLPI(caches, n_caches).Cache.LineSize = (WORD)64;

This cast is not needed.
> -
>     else
> -
> 	SLPI(caches, n_caches).Cache.LineSize = (WORD)value;
> -
>     if (f)
> -
> 	fclose(f);
> -
> -
>     snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> -
> 	     SYS_CPU_CACHE_SIZE, cpu_i, i);
> -
>     if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> -
> 	SLPI(caches, n_caches).Cache.Size = (DWORD)16 << 10;

This cast isn't needed unless an int is less than 32 bits in size.
> -
>     else
> -
> 	SLPI(caches, n_caches).Cache.Size = (DWORD)value << 10;
> -
>     if (f)
> -
> 	fclose(f);
> -
> -
>     snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> -
> 	     SYS_CPU_CACHE_TYPE, cpu_i, i);
> -
>     if (!(f = fopen(path, "r")) || !fgets(string, sizeof(string), f))
> -
> 	SLPI(caches, n_caches).Cache.Type = CacheData;
> -
>     else switch (string[0])
> -
>                {
> -
>     case 'D':
> -
> 	SLPI(caches, n_caches).Cache.Type = CacheData; break;
> -
>     case 'I':
> -
> 	SLPI(caches, n_caches).Cache.Type = CacheInstruction; break;
> -
>     case 'U': default:
> -
> 	SLPI(caches, n_caches).Cache.Type =
> -
> 	    string[2] == 'i' ? CacheUnified : CacheTrace; break;
> -
>     }
> -
> -
>     if (f)
> -
> 	fclose(f);
> -
> -
>     n_caches++;
> -
> }
> -
> n_cores++;
> -
> TRACE("detected %d caches for coreid %d\n", i, core_id);
> -
>    }
> - }
> -
> - if (n_caches < 1)
> - {
> -
>    HeapFree(GetProcessHeap(), 0, caches);
> -
>    caches = NULL;
> - }
> -
> - /* trim leftovers */
> - cores = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> -
> 	    cores, sizeof(*cores) * n_cores);
> -
> - if (1) /* final cleanup work */

Why the if (1)?
> - {
> -
>    int i;
> -
>    /* set the real processor mask in the caches. */
> -
>    for (i = 0; i < n_caches; i++)
> -
>    {
> -
> int cores_i;
> -
> cores_i = caches[i].ProcessorMask;
> -
> caches[i].ProcessorMask = cores[cores_i].ProcessorMask;
> -
>    }
> -
> -
>    /* remove the reserved values from the cores, set flags,
> -
>       build numa and package. */
> -
> -
>    numa[0].ProcessorMask = 0x00;
> -
>    numa[0].Relationship = RelationNumaNode;
> -
>    SLPI(numa, 0).NumaNode.NodeNumber = 0x00;
> -
>    package[0].Relationship = RelationProcessorPackage;
> -
> -
>    for (i = 0; i < n_cores; i++)
> -
>    {
> -
> SLPI(cores, i).Reserved[0] = 0;
> -
> SLPI(cores, i).ProcessorCore.Flags = 0x00; /* ?unclear */
> -
> numa[0].ProcessorMask |= cores[i].ProcessorMask;
> -
>    }
> -
> -
>    package[0].ProcessorMask = numa[0].ProcessorMask;
> - }
> - }
> -
> - TRACE("getting cached result.\n");
> -
> - if (1) /* final check for required size, final result */

Same here.
> - {
> - DWORD required; required = sizeof(*cores) * (n_cores + n_caches + 2);
> - if (*pbuflen < required)
> - {
> -
>    TRACE("return with failure (ERROR_INSUFFICIENT_BUFFER).\n");
> -
>    SetLastError(ERROR_INSUFFICIENT_BUFFER);
> -
>    *pbuflen = required;
> -
>    return 0;
> - }
> -
> - *pbuflen = required;
> -
> - /* beware: buffer is SYSTEM_LOGICAL_PROCESSOR_INFORMATION *buffer; */
> - memcpy(buffer, cores, sizeof(*cores) * n_cores);
> - buffer += n_cores;
> -
> - if (n_caches > 0)
> - {
> -
>    memcpy(buffer, caches, sizeof(*caches) * n_caches);
> -
>    buffer += n_caches;
> - }
> -
> - *buffer++ = package[0];
> - *buffer++ = numa[0];
> - }
> -
> - TRACE("return success!\n");

Is this really necessary? IMO it's generally better to be quiet if you have nothing interesting to say (in typical Unix fashion). And a function succeeding isn't interesting.
> - return 1;
>
> +}
> -
>
> +#undef SYS_CPU
> +#undef SYS_CPU_MAX
> +#undef SYS_CPU_COREID
> +#undef SYS_CPU_PACKAGEID
> +#undef SYS_CPU_CACHE
> +#undef SYS_CPU_CACHE_MAX
> +#undef SYS_CPU_CACHE_LEVEL
> +#undef SYS_CPU_CACHE_SIZE
> +#undef SYS_CPU_CACHE_TYPE
> +#undef SYS_CPU_CACHE_LINE
> +#undef SYS_CPU_CACHE_ASSOC
> -
>
> +#endif /* linux */
> -
>
> /***********************************************************************
> -
>       GetLogicalProcessorInformation     (KERNEL32.@)
>
> */
> BOOL WINAPI GetLogicalProcessorInformation(PSYSTEM_LOGICAL_PROCESSOR_INFORMATION buffer, PDWORD pBufLen)
> {
> - FIXME("(%p,%p): stub\n", buffer, pBufLen);
> - SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
> - return FALSE;
> - int rv;
> -
> - if (pBufLen == NULL || buffer == NULL)
> - {
> - SetLastError(ERROR_BAD_ARGUMENTS);
> - return FALSE;
> - }
> -
>
> +#if defined linux
> - rv = glpi_impl_linux(buffer, pBufLen);
>
> +#else /* no supported OS found */
> - rv = -1;
>
> +#endif
> -
> - /* fallback hard-coded result */
> - if (rv < 0)
> - rv = glpi_impl_fb(buffer, pBufLen);
> -
> - return rv;
>
> }
>
> /***********************************************************************
> diff --git a/dlls/kernel32/tests/Makefile.in b/dlls/kernel32/tests/Makefile.in
> index dce27db..1505944 100644
> --- a/dlls/kernel32/tests/Makefile.in
> +++ b/dlls/kernel32/tests/Makefile.in
> @@ -17,6 +17,7 @@ C_SRCS = \
>    file.c \
>    format_msg.c \
>    generated.c \
> - glpi.c \
> heap.c \
> loader.c \
> locale.c \
>
> diff --git a/dlls/kernel32/tests/glpi.c b/dlls/kernel32/tests/glpi.c
> new file mode 100644
> index 0000000..a81d1d5
> --- /dev/null
> +++ b/dlls/kernel32/tests/glpi.c

Like I said, this should be integrated into ntdll:info instead of being its own test in kernel32. Note that the style of reporting errors is different between ntdll and kernel32. In kernel32, the function returns a BOOL, and if it's false, you have to check the LastError. In ntdll, however, functions simply return the status code directly. Also, ntdll functions return an NTSTATUS instead of a constant from <winerror.h>.
> @@ -0,0 +1,82 @@
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <time.h>
> +#include <stdio.h>
> -
>
> +#include "wine/test.h"
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winerror.h"
> -
>
> +static void test_glpi(void)
> +{
> - SYSTEM_LOGICAL_PROCESSOR_INFORMATION buffer[200];
> - DWORD buflen = sizeof(buffer);
> - BOOL ret; int i, n;
> -
> - ret = GetLogicalProcessorInformation(NULL, &buflen);
> - ok(!ret, "Passing NULL as buffer should not be ok.\n");
> -
> - ret = GetLogicalProcessorInformation(buffer, NULL);
> - ok(!ret, "Passing NULL as pbuflen should not be ok.\n");
> -
> - ret = GetLogicalProcessorInformation(buffer, &buflen);
> - ok(ret, "Normal glpi call (%d)\n", GetLastError());

Don't call GetLastError() inside an ok(). (Actually, that's moot, because this test belongs in ntdll:process, but in general, you shouldn't call GetLastError() inside an ok().)
> -
> - ret = GetLogicalProcessorInformation(buffer, &buflen);
> - ok(ret, "glpi call with the resulting buflen (%d)\n", buflen);

Showing the last error (ntdll: return status) here might help diagnosing any problems with this test. (But don't call GetLastError() inside the ok().)
> -
> - buflen--;
> -
> - ret = GetLogicalProcessorInformation(buffer, &buflen);
> - ok(!ret, "glpi call with insufficient buflen (%d)\n", buflen);

Here you should make sure the LastError is ERROR_INSUFFICIENT_BUFFER (corresponding NTSTATUS: STATUS_BUFFER_TOO_SMALL, or possibly STATUS_INVALID_BUFFER_SIZE), and not some other error.
> -
> - ret = GetLogicalProcessorInformation(buffer, &buflen);
> - ok(ret, "glpi call with resulting buflen (%d)\n", buflen);

Trace the LastError (ntdll: return status) here, too.
> -
> - n = buflen / sizeof(SYSTEM_LOGICAL_PROCESSOR_INFORMATION);
> -
> - printf("glpi result: %d array elements.\n", n);

Don't use printf(3); use trace().
> -
>
> +#ifdef NONAMELESSUNION
> +#define BUFFER_I buffer[i].u
> +#else
> +#define BUFFER_I buffer[i]
> +#endif /* NONAMELESSUNION */

Like I said above, just use U(buffer[i]).
> -
> - for (i = 0; i < n; i++)
> - {
> - switch (buffer[i].Relationship)
> - {
> - case RelationProcessorCore:
> -
>    printf("ProcessorCore idx=%03d mask=%lxh flags=%u\n",
> -
>    i, buffer[i].ProcessorMask, BUFFER_I.ProcessorCore.Flags);
> -
>    break;
> - case RelationNumaNode:
> -
>    printf("NumaNode      idx=%03d mask=%lxh noden=%u\n",
> -
>     i, buffer[i].ProcessorMask, BUFFER_I.NumaNode.NodeNumber);
> -
>    break;
> - case RelationCache:
> -
>    printf("Cache         idx=%03d mask=%lxh "
> -
>     "l=%u, a=%u, ls=%u, sz=%u, ty=%u\n",
> -
>    i, buffer[i].ProcessorMask,
> -
>    BUFFER_I.Cache.Level, BUFFER_I.Cache.Associativity,
> -
>    BUFFER_I.Cache.LineSize, BUFFER_I.Cache.Size,
> -
>    BUFFER_I.Cache.Type);
> -
>    break;
> - case RelationProcessorPackage:
> -
>    printf("ProcessorPack idx=%03d mask=%lxh\n",
> -
>    i, buffer[i].ProcessorMask);

For all of these, use trace() instead of printf(3).
> -
>    break;
> - default:
> -
>    ok(0, "invalid relationship %d", buffer[i].Relationship);
> - }
> - }
>
> +#undef BUFFER_I
> +}
> -
>
> +START_TEST(glpi)
> +{
> - test_glpi();
>
> +}
> -
>
> -- 
> 1.6.4

One general note about your patch: use spaces, not tabs. The rest of the file only uses tabs in doc comments.

Chip

[PATCH] kernel32: implement GetLogicalProcessorInformation

[Ken Thomases]

On Nov 24, 2011, at 5:13 PM, Charles Davis wrote:
> On Nov 24, 2011, at 4:21 AM, Claudio Fontana wrote:
> > - ret = GetLogicalProcessorInformation(buffer, &buflen);
> > - ok(ret, "Normal glpi call (%d)\n", GetLastError());
>
> Don't call GetLastError() inside an ok(). (Actually, that's moot, because this test belongs in ntdll:process, but in general, you shouldn't call GetLastError() inside an ok().)

I don't think there's such a general rule.  As I understand it, one has to be mindful of the undefined order of evaluation of function arguments.  So, the call which may set the last error must not be in an argument list with the call of GetLastError(), because you can't be sure which will be called first.

That is, in this case, the call to GetLogicalProcessorInformation() must not be within the ok() argument list along with GetLastError().  Since it is not, the above code is fine.

(I also think your paranoia about access(2) is overblown, but I agree it isn't the best means of checking for the existence of a file.)

Regards,
Ken

[PATCH] kernel32: implement GetLogicalProcessorInformation

[Claudio Fontana]

Hello,

I saw a bug 27189 in the bug database where Austin English was waiting
for a GLPI implementation.
I had one already (among other patches I keep against the wine tree),
so I attached it, and while I was at it, I thought to submit it to
wine-patches as well.

I am not sure I have the time to work on all items you mentioned,
so if you want to cannibalize it and do things differently, you are
more than welcome to take what you need from my patch.
Some comments on your comments below :)

On Fri, Nov 25, 2011 at 12:13 AM, Charles Davis  wrote:
> Hi,
>
> On Nov 24, 2011, at 4:21 AM, Claudio Fontana wrote:
> > First implementation of GetLogicalProcessorInformation.
> > Limitations: all logical processors are added to the same NUMA set,
> > and all cores are added to the same package.
> > Only the linux-specific helper function is implemented, so for now
> > everybody else gets the fallback hard-coded results only.
> > The fallback is also triggered when the OS-specific APIs fail.
> >
> > The linux-specific function is based on sysfs.
> >
> > The new OS-specific functions can be easily added as additional
> > ifdefs by following the linux example.
> >
> > dlls/kernel32/process.c         |  350 ++++++++++++++++++++++++++++++++++++++-
> > dlls/kernel32/tests/Makefile.in |    1 +
> > dlls/kernel32/tests/glpi.c      |   82 +++++++++
>
> You should really implement this in ntdll.NtQuerySystemInformation (information class SystemLogicalProcessorInformation). In general, kernel32 forwards most of its implementation to ntdll, which reduces code duplication.

You understand this more than me. Maybe it could be a subsequent commit?
> > 3 files changed, 430 insertions(+), 3 deletions(-)
> > create mode 100644 dlls/kernel32/tests/glpi.c
>
> IMO, you should add the test to dlls/ntdll/tests/info.c, and test NtQuerySystemInformation() with info class SystemLogicalProcessorInformation.

I see.
> > diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c
> > index b6c0b9d..de39244 100644
> > --- a/dlls/kernel32/process.c
> > +++ b/dlls/kernel32/process.c
> > @@ -51,6 +51,12 @@
> >
> > #include "ntstatus.h"
> > #define WIN32_NO_STATUS
> > -
> >
> > +#include "windef.h"
> > +#include "winbase.h"
> > +#include "winerror.h"
> > +#include "winnt.h"
> > -
> >
> > #include "winternl.h"
> > #include "kernel_private.h"
> > #include "psapi.h"
> > @@ -3705,14 +3711,352 @@ HANDLE WINAPI GetCurrentProcess(void)
> >     return (HANDLE)~(ULONG_PTR)0;
> > }
> >
> > +/***************
> > -
> > - glpi_impl_fb:
> > -
> > - a fallback implementation of GetLogicalProcessorInformation
> > -
> > - for the systems we do not support yet,
> > -
> > - or for when the OS-specific API fails.
> > -
> > - Same arguments and return value as glpi.
> > - */
>
> This comment should really come right before the function you're documenting. Also, it's formatted wrong. It should look more like this:

the comment is right before the function I am documenting. The
function I am documenting is
As for the formatting, I am not sure what you mean, they look pretty
much the same.
Your formatting adds some more spaces before the function name, and
less before the additional comments below.
> /***********************************************************************************
>  *        logical_processor_info_fallback
>  *
>  * Fallback implementation of NtQuerySystemInformation() with info class
>  * SystemLogicalProcessorInfo.
>  */
> > -
> >
> > +#undef SLPI
> > +#ifdef NONAMELESSUNION
> > +#define SLPI(buffer, idx) buffer[idx].DUMMYUNIONNAME
> > +#else
> > +#define SLPI(buffer, idx) buffer[idx]
> > +#endif /* NONAMELESSUNION */
>
> You could avoid the ifdef if you used the U() macro. In fact, why are you defining a macro at all? Just use U(buffer[idx]).

good to know. I did not know about the U() macro. ACK for the U(buffer[idx]).
> > -
> >
> > +static BOOL
> > +glpi_impl_fb(PSYSTEM_LOGICAL_PROCESSOR_INFORMATION buffer, PDWORD pbuflen)
>
> Surely you could use a more descriptive name. (See one example in my example doc comment.) This isn't 1978, you know.

This is not 1978, no. And?
glpi_impl_fb
short and sweet and to the point. The right name for a static function
that is glpi implementation fallback.
This isn't java, you know.
> > +{
> > - int glpi_n = 5;
> > - FIXME("(%p,%p): reporting hard coded information\n", buffer, pbuflen);
> > -
> > - if (*pbuflen < sizeof(*buffer) * glpi_n)
> > - {
> > - SetLastError(ERROR_INSUFFICIENT_BUFFER);
> > - *pbuflen = sizeof(*buffer) * glpi_n;
> > - return FALSE;
> > - }
> > -
> > - buffer[0].ProcessorMask = (ULONG_PTR)0x01;
> > - buffer[1].ProcessorMask = (ULONG_PTR)0x01;
> > - buffer[2].ProcessorMask = (ULONG_PTR)0x01;
> > - buffer[3].ProcessorMask = (ULONG_PTR)0x01;
> > - buffer[4].ProcessorMask = (ULONG_PTR)0x01;
>
> These casts aren't necessary.

I know.
> > -
> > - buffer[0].Relationship = RelationProcessorCore;
> > - buffer[1].Relationship = RelationNumaNode;
> > - buffer[2].Relationship = RelationCache;
> > - buffer[3].Relationship = RelationCache;
> > - buffer[4].Relationship = RelationProcessorPackage;
> > -
> > - SLPI(buffer, 0).ProcessorCore.Flags = (BYTE)0x00;
> > - SLPI(buffer, 1).NumaNode.NodeNumber = (DWORD)0x00;
>
> Neither are these.

I know.
> > -
> > - SLPI(buffer, 2).Cache.Level = (BYTE)0x01;
> > - SLPI(buffer, 2).Cache.Associativity = (BYTE)0x08;
> > - SLPI(buffer, 2).Cache.LineSize = (WORD)64;
> > - SLPI(buffer, 2).Cache.Size = (DWORD)16 << 10; /* 16 KB */
>
> Or these.

I know.
> > - SLPI(buffer, 2).Cache.Type = CacheData;
> > -
> > - SLPI(buffer, 3).Cache.Level = (BYTE)0x02;
> > - SLPI(buffer, 3).Cache.Associativity = (BYTE)0x08;
> > - SLPI(buffer, 3).Cache.LineSize = (WORD)64;
> > - SLPI(buffer, 3).Cache.Size = (DWORD)1024 << 10; /* 1024 KB */
>
> Or these.

I know.
The thing is, having those explicit casts is kinda handy as
documentation when working on the OS-specific implementations, to
remember which is which. Also, in some of the cases the cast is
needed.
But I understand your angle. No cast where it is not absolutely needed.
> > - SLPI(buffer, 3).Cache.Type = CacheUnified;
> > - /* no additional info for processor package at buffer[4] needed. */
> > -
> > - *pbuflen = sizeof(*buffer) * glpi_n;
> > - return TRUE;
> >
> > +}
> > -
> >
> > +#ifdef linux
> > +/******************
> > -
> > - glpi_impl_linux:
> > -
> > - an implementation of GetLogicalProcessorInformation
> > -
> > - for linux sysfs.
> > -
> > - Returns: -1 on API failure (trigger fallback),
> > -
> > - otherwise see glpi.
> > - */
>
> See above.
> > -
> >
> > +#define SYS_CPU "/sys/devices/system/cpu/cpu"
> > +#define SYS_CPU_MAX 63
>
> I'd criticize your use of a static limit, if only Windows didn't have that limit ;). Actually, you can only have 32 processors on a 32-bit system because a ULONG_PTR is 32 bits there.

:)
> > +#define SYS_CPU_COREID "/topology/core_id"
> > +#define SYS_CPU_PACKAGEID "/topology/physical_package_id"
> > +#define SYS_CPU_CACHE "/cache/index"
> > +#define SYS_CPU_CACHE_MAX 12
> > +#define SYS_CPU_CACHE_LEVEL "/level"
> > +#define SYS_CPU_CACHE_SIZE "/size"
> > +#define SYS_CPU_CACHE_TYPE "/type"
> > +#define SYS_CPU_CACHE_LINE "/coherency_line_size"
> > +#define SYS_CPU_CACHE_ASSOC "/ways_of_associativity"
> > -
> >
> > +static int glpi_impl_linux(SYSTEM_LOGICAL_PROCESSOR_INFORMATION *buffer,
> > - PDWORD pbuflen)
> >
> > +{
> > - /* remember the glpi results, so that subsequent calls are faster */
> > - static int n_cores = 0;
> > - static int n_caches = 0;
> > - static PSYSTEM_LOGICAL_PROCESSOR_INFORMATION cores = NULL;
> > - static PSYSTEM_LOGICAL_PROCESSOR_INFORMATION caches = NULL;
> > - static SYSTEM_LOGICAL_PROCESSOR_INFORMATION numa[1];
> > - static SYSTEM_LOGICAL_PROCESSOR_INFORMATION package[1];
> > -
> > - if (!n_cores)
> > - {
> > - int n_cpus = 0; int cpu_i;
> > - char path[PATH_MAX];
> > -
> > - TRACE("building new cached result.\n");
> > -
> > - for (n_cpus = 0; n_cpus < SYS_CPU_MAX; n_cpus++)
> > - {
> > - snprintf(path, sizeof(path), SYS_CPU "%d", n_cpus);
> > - if (access(path, R_OK | X_OK) != 0)
>
> Don't use access(2). There's a big fat warning on the man-page telling you not to use it because of a race condition. Another way to solve the problem you're trying to solve might be to scan the "/sys/devices/system/cpu/" directory for the number of entries using the POSIX readdir(3) interface. You could scan them all up front, or you could take each one as you go, re-allocating the table for each new entry.

^mail formatting issue maybe..^

This is not the use of access that you are looking for. Replace with
stat if it makes you feel better.
If you read the thing again, I think you'll see that there is no need
for readdir, nor for reallocating anything.
Are you worried that somebody is hotplugging cpus?
> > - break;
> > - }
> > -
> > - TRACE("detected %d logical cpus.\n", n_cpus);
> > - if (n_cpus < 1)
> > - {
> > - TRACE("requesting fallback implementation.\n");
> > - return -1;
> > - }
> > -
> > - cores = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*cores) * n_cpus);
> > - caches = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*caches) * 1);
> > -
> > - for (cpu_i = 0; cpu_i < n_cpus; cpu_i++)
> > - {
> > - FILE *f; int core_id; int i;
> > - snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_COREID, cpu_i);
> > - cores[n_cores].ProcessorMask = (ULONG_PTR)0x01 << cpu_i;
>
> This cast might be needed if there are more than 32 CPUs, so that's OK.
> > - cores[n_cores].Relationship = RelationProcessorCore;
> > -
> > - if (!(f = fopen(path, "r")) || fscanf(f, "%d", &core_id) != 1)
> > - {
> > - /* topology info not available, provide default cpuid=cpu_i */
> > - ERR("could not read cpu%d topology.\n", cpu_i);
> > - SLPI(cores, n_cores).Reserved[0] = cpu_i;
> > - n_cores++;
> > - if (f)
> > - fclose(f);
> > - continue; /* next cpu */
> > - }
> > -
> > - /* normal sane case */
> > - fclose(f);
> > - /* store the core_id to catch cpus with same core_id */
> > - SLPI(cores, n_cores).Reserved[0] = core_id;
> > -
> > - /* look for this core_id in previous elements */
> > - for (i = 0; i < n_cores; i++)
> > - if (SLPI(cores, i).Reserved[0] == SLPI(cores, n_cores).Reserved[0])
> > - break;
> > -
> > - if (i < n_cores)
> > - {
> > - TRACE("found existing coreid %d for cpu%d.\n", core_id, cpu_i);
> > - /* add this logical processor to the existing mask */
> > - cores[i].ProcessorMask |= cores[n_cores].ProcessorMask;
> > - /* do not advance n_cores: this element can be overwritten. */
> > - }
> > - else
> > - {
> > - TRACE("found new coreid %d for cpu%d.\n", core_id, cpu_i);
> > -
> > - /* look for caches */
> > - for (i = 0; i < SYS_CPU_CACHE_MAX; i++)
> > - {
> > - unsigned long value;
> > - char string[80];
> > -
> > - snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d",
> > - cpu_i, i);
> > - if (access(path, R_OK | X_OK) != 0)
>
> Don't use access(2).

I can replace with stat if you like. Not that there is any reason to.
> > - break; /* no more caches found. */
> > -
> > - caches = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> > - caches, sizeof(*caches) * (n_caches + 1));
> > -
> > - /* we keep the index into the cores array to set
> > - the right value at the end. */
> > - caches[n_caches].ProcessorMask = (ULONG_PTR)n_cores;
>
> This cast, however, is definitely not needed.
> > - caches[n_caches].Relationship = RelationCache;
> > -
> > - snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> > - SYS_CPU_CACHE_LEVEL, cpu_i, i);
> > - if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> > - SLPI(caches, n_caches).Cache.Level = (BYTE)0x01;
>
> Neither is this.
> > - else
> > - SLPI(caches, n_caches).Cache.Level = (BYTE)value;
> > - if (f)
> > - fclose(f);
> > -
> > - snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> > - SYS_CPU_CACHE_ASSOC, cpu_i, i);
> > - if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> > - SLPI(caches, n_caches).Cache.Associativity = 0x08;
> > - else
> > - SLPI(caches, n_caches).Cache.Associativity = (BYTE)value;
> > - if (f)
> > - fclose(f);
> > -
> > - snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> > - SYS_CPU_CACHE_LINE, cpu_i, i);
> > - if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> > - SLPI(caches, n_caches).Cache.LineSize = (WORD)64;
>
> This cast is not needed.
> > - else
> > - SLPI(caches, n_caches).Cache.LineSize = (WORD)value;
> > - if (f)
> > - fclose(f);
> > -
> > - snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> > - SYS_CPU_CACHE_SIZE, cpu_i, i);
> > - if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> > - SLPI(caches, n_caches).Cache.Size = (DWORD)16 << 10;
>
> This cast isn't needed unless an int is less than 32 bits in size.
> > - else
> > - SLPI(caches, n_caches).Cache.Size = (DWORD)value << 10;
> > - if (f)
> > - fclose(f);
> > -
> > - snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> > - SYS_CPU_CACHE_TYPE, cpu_i, i);
> > - if (!(f = fopen(path, "r")) || !fgets(string, sizeof(string), f))
> > - SLPI(caches, n_caches).Cache.Type = CacheData;
> > - else switch (string[0])
> > - {
> > - case 'D':
> > - SLPI(caches, n_caches).Cache.Type = CacheData; break;
> > - case 'I':
> > - SLPI(caches, n_caches).Cache.Type = CacheInstruction; break;
> > - case 'U': default:
> > - SLPI(caches, n_caches).Cache.Type =
> > - string[2] == 'i' ? CacheUnified : CacheTrace; break;
> > - }
> > -
> > - if (f)
> > - fclose(f);
> > -
> > - n_caches++;
> > - }
> > - n_cores++;
> > - TRACE("detected %d caches for coreid %d\n", i, core_id);
> > - }
> > - }
> > -
> > - if (n_caches < 1)
> > - {
> > - HeapFree(GetProcessHeap(), 0, caches);
> > - caches = NULL;
> > - }
> > -
> > - /* trim leftovers */
> > - cores = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> > - cores, sizeof(*cores) * n_cores);
> > -
> > - if (1) /* final cleanup work */
>
> Why the if (1)?

To open a new scope, so I could declare int i; more locally.
The alternative is to use just the opening {, but it can get
confusing, and messes up some editors.

This is something that I would definitely suggest you guys pick up.
There are many variables instead in the codebase that line up in a
scope that does not belong to the problem at hand.
> > - {
> > - int i;
> > - /* set the real processor mask in the caches. */
> > - for (i = 0; i < n_caches; i++)
> > - {
> > - int cores_i;
> > - cores_i = caches[i].ProcessorMask;
> > - caches[i].ProcessorMask = cores[cores_i].ProcessorMask;
> > - }
> > -
> > - /* remove the reserved values from the cores, set flags,
> > - build numa and package. */
> > -
> > - numa[0].ProcessorMask = 0x00;
> > - numa[0].Relationship = RelationNumaNode;
> > - SLPI(numa, 0).NumaNode.NodeNumber = 0x00;
> > - package[0].Relationship = RelationProcessorPackage;
> > -
> > - for (i = 0; i < n_cores; i++)
> > - {
> > - SLPI(cores, i).Reserved[0] = 0;
> > - SLPI(cores, i).ProcessorCore.Flags = 0x00; /* ?unclear */
> > - numa[0].ProcessorMask |= cores[i].ProcessorMask;
> > - }
> > -
> > - package[0].ProcessorMask = numa[0].ProcessorMask;
> > - }
> > - }
> > -
> > - TRACE("getting cached result.\n");
> > -
> > - if (1) /* final check for required size, final result */
>
> Same here.

..and same here. "DWORD required" is only useful in this scope, and it
makes no sense not to open a bracket here.
Keep things local to where they belong, and get rid of them when you
don't need them anymore.
> > - {
> > - DWORD required; required = sizeof(*cores) * (n_cores + n_caches + 2);
> > - if (*pbuflen < required)
> > - {
> > - TRACE("return with failure (ERROR_INSUFFICIENT_BUFFER).\n");
> > - SetLastError(ERROR_INSUFFICIENT_BUFFER);
> > - *pbuflen = required;
> > - return 0;
> > - }
> > -
> > - *pbuflen = required;
> > -
> > - /* beware: buffer is SYSTEM_LOGICAL_PROCESSOR_INFORMATION *buffer; */
> > - memcpy(buffer, cores, sizeof(*cores) * n_cores);
> > - buffer += n_cores;
> > -
> > - if (n_caches > 0)
> > - {
> > - memcpy(buffer, caches, sizeof(*caches) * n_caches);
> > - buffer += n_caches;
> > - }
> > -
> > - *buffer++ = package[0];
> > - *buffer++ = numa[0];
> > - }
> > -
> > - TRACE("return success!\n");
>
> Is this really necessary? IMO it's generally better to be quiet if you have nothing interesting to say (in typical Unix fashion). And a function succeeding isn't interesting.

it is not really necessary. But all other exit points are traced.
Would be a little bit asymmetrical. But yea.
> > - return 1;
> >
> > +}
> > -
> >
> > +#undef SYS_CPU
> > +#undef SYS_CPU_MAX
> > +#undef SYS_CPU_COREID
> > +#undef SYS_CPU_PACKAGEID
> > +#undef SYS_CPU_CACHE
> > +#undef SYS_CPU_CACHE_MAX
> > +#undef SYS_CPU_CACHE_LEVEL
> > +#undef SYS_CPU_CACHE_SIZE
> > +#undef SYS_CPU_CACHE_TYPE
> > +#undef SYS_CPU_CACHE_LINE
> > +#undef SYS_CPU_CACHE_ASSOC
> > -
> >
> > +#endif /* linux */
> > -
> >
> > /***********************************************************************
> >  *           GetLogicalProcessorInformation     (KERNEL32.@)
> >  */
> > BOOL WINAPI GetLogicalProcessorInformation(PSYSTEM_LOGICAL_PROCESSOR_INFORMATION buffer, PDWORD pBufLen)
> > {
> > - FIXME("(%p,%p): stub\n", buffer, pBufLen);
> > - SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
> > - return FALSE;
> > - int rv;
> > -
> > - if (pBufLen == NULL || buffer == NULL)
> > - {
> > - SetLastError(ERROR_BAD_ARGUMENTS);
> > - return FALSE;
> > - }
> > -
> >
> > +#if defined linux
> > - rv = glpi_impl_linux(buffer, pBufLen);
> >
> > +#else /* no supported OS found */
> > - rv = -1;
> >
> > +#endif
> > -
> > - /* fallback hard-coded result */
> > - if (rv < 0)
> > - rv = glpi_impl_fb(buffer, pBufLen);
> > -
> > - return rv;
> >
> > }
> >
> > /***********************************************************************
> > diff --git a/dlls/kernel32/tests/Makefile.in b/dlls/kernel32/tests/Makefile.in
> > index dce27db..1505944 100644
> > --- a/dlls/kernel32/tests/Makefile.in
> > +++ b/dlls/kernel32/tests/Makefile.in
> > @@ -17,6 +17,7 @@ C_SRCS = \
> >       file.c \
> >       format_msg.c \
> >       generated.c \
> > - glpi.c \
> >
> > heap.c \
> >       loader.c \
> >       locale.c \
> > diff --git a/dlls/kernel32/tests/glpi.c b/dlls/kernel32/tests/glpi.c
> > new file mode 100644
> > index 0000000..a81d1d5
> > --- /dev/null
> > +++ b/dlls/kernel32/tests/glpi.c
>
> Like I said, this should be integrated into ntdll:info instead of being its own test in kernel32. Note that the style of reporting errors is different between ntdll and kernel32. In kernel32, the function returns a BOOL, and if it's false, you have to check the LastError. In ntdll, however, functions simply return the status code directly. Also, ntdll functions return an NTSTATUS instead of a constant from <winerror.h>.

I am not sure I can do this, at least soon. You are free to do this
yourself if you like of course.
> > @@ -0,0 +1,82 @@
> > +#include <stdarg.h>
> > +#include <stdlib.h>
> > +#include <time.h>
> > +#include <stdio.h>
> > -
> >
> > +#include "wine/test.h"
> > +#include "windef.h"
> > +#include "winbase.h"
> > +#include "winerror.h"
> > -
> >
> > +static void test_glpi(void)
> > +{
> > - SYSTEM_LOGICAL_PROCESSOR_INFORMATION buffer[200];
> > - DWORD buflen = sizeof(buffer);
> > - BOOL ret; int i, n;
> > -
> > - ret = GetLogicalProcessorInformation(NULL, &buflen);
> > - ok(!ret, "Passing NULL as buffer should not be ok.\n");
> > -
> > - ret = GetLogicalProcessorInformation(buffer, NULL);
> > - ok(!ret, "Passing NULL as pbuflen should not be ok.\n");
> > -
> > - ret = GetLogicalProcessorInformation(buffer, &buflen);
> > - ok(ret, "Normal glpi call (%d)\n", GetLastError());
>
> Don't call GetLastError() inside an ok(). (Actually, that's moot, because this test belongs in ntdll:process, but in general, you shouldn't call GetLastError() inside an ok().)

hmm
> > -
> > - ret = GetLogicalProcessorInformation(buffer, &buflen);
> > - ok(ret, "glpi call with the resulting buflen (%d)\n", buflen);
>
> Showing the last error (ntdll: return status) here might help diagnosing any problems with this test. (But don't call GetLastError() inside the ok().)
> > -
> > - buflen--;
> > -
> > - ret = GetLogicalProcessorInformation(buffer, &buflen);
> > - ok(!ret, "glpi call with insufficient buflen (%d)\n", buflen);
>
> Here you should make sure the LastError is ERROR_INSUFFICIENT_BUFFER (corresponding NTSTATUS: STATUS_BUFFER_TOO_SMALL, or possibly STATUS_INVALID_BUFFER_SIZE), and not some other error.

makes sense.
> > -
> > - ret = GetLogicalProcessorInformation(buffer, &buflen);
> > - ok(ret, "glpi call with resulting buflen (%d)\n", buflen);
>
> Trace the LastError (ntdll: return status) here, too.

Hmm no no, here it must succeed.
> > -
> > - n = buflen / sizeof(SYSTEM_LOGICAL_PROCESSOR_INFORMATION);
> > -
> > - printf("glpi result: %d array elements.\n", n);
>
> Don't use printf(3); use trace().

Well the goal here is different.
Case in point: you see this stuff in the test output of the testbot.
> > -
> >
> > +#ifdef NONAMELESSUNION
> > +#define BUFFER_I buffer[i].u
> > +#else
> > +#define BUFFER_I buffer[i]
> > +#endif /* NONAMELESSUNION */
>
> Like I said above, just use U(buffer[i]).

ACK
> > -
> > - for (i = 0; i < n; i++)
> > - {
> > - switch (buffer[i].Relationship)
> > - {
> > - case RelationProcessorCore:
> > - printf("ProcessorCore idx=%03d mask=%lxh flags=%u\n",
> > - i, buffer[i].ProcessorMask, BUFFER_I.ProcessorCore.Flags);
> > - break;
> > - case RelationNumaNode:
> > - printf("NumaNode      idx=%03d mask=%lxh noden=%u\n",
> > - i, buffer[i].ProcessorMask, BUFFER_I.NumaNode.NodeNumber);
> > - break;
> > - case RelationCache:
> > - printf("Cache         idx=%03d mask=%lxh "
> > - "l=%u, a=%u, ls=%u, sz=%u, ty=%u\n",
> > - i, buffer[i].ProcessorMask,
> > - BUFFER_I.Cache.Level, BUFFER_I.Cache.Associativity,
> > - BUFFER_I.Cache.LineSize, BUFFER_I.Cache.Size,
> > - BUFFER_I.Cache.Type);
> > - break;
> > - case RelationProcessorPackage:
> > - printf("ProcessorPack idx=%03d mask=%lxh\n",
> > - i, buffer[i].ProcessorMask);
>
> For all of these, use trace() instead of printf(3).
> > - break;
> > - default:
> > - ok(0, "invalid relationship %d", buffer[i].Relationship);
> > - }
> > - }
> >
> > +#undef BUFFER_I
> > +}
> > -
> >
> > +START_TEST(glpi)
> > +{
> > - test_glpi();
> >
> > +}
> > -
> >
> > --
> > 1.6.4
>
> One general note about your patch: use spaces, not tabs. The rest of the file only uses tabs in doc comments.

ACK
> Chip

I have no urge to get my patch committed, so sorry if I put my
opinions forward so directly.
If you can prove me wrong especially on the access() thing, I would be
happy to learn something new.

C.

[PATCH] kernel32: implement GetLogicalProcessorInformation

[Claudio Fontana]

I have resent the patch as a "try 2" (there was a try 1.5 to fix test
issues under windows older than XP SP3).

The patch contains changes based on your feedback.

Some things I stand by, and won't change.
If you feel differently, you should argue for their validity a little
bit more than "don't use access(2)."

Some things I don't have time to address, for example this
NtQueryInformation thing I know nothing about. Sorry.
That seems to be something that can be done as a separate commit by
someone who cares about that aspect (you?).

So this is it. You like, you take. You don't you drop. For my wine
tree, it works beautifully and I am good with that.

Cheers

C.

[PATCH] kernel32: implement GetLogicalProcessorInformation

[Claudio Fontana]

On Fri, Nov 25, 2011 at 4:07 PM, Claudio Fontana
 wrote:
> I have resent the patch as a "try 2" (there was a try 1.5 to fix test
> issues under windows older than XP SP3).
>
> The patch contains changes based on your feedback.

Which is completely broken, since the U() macro you suggested to use
is a wine/test.h feature only,
and completely broke the patch.
So I fall back to my try 1.5 for my tree.

[PATCH] kernel32: implement GetLogicalProcessorInformation

[Charles Davis]

On Nov 25, 2011, at 8:17 AM, Claudio Fontana wrote:
> On Fri, Nov 25, 2011 at 4:07 PM, Claudio Fontana
>  wrote:
> > I have resent the patch as a "try 2" (there was a try 1.5 to fix test
> > issues under windows older than XP SP3).
> >
> > The patch contains changes based on your feedback.
>
> Which is completely broken, since the U() macro you suggested to use
> is a wine/test.h feature only,
> and completely broke the patch.

You are right. The way most files (other than tests) in Wine deal with that is that they unconditionally define NONAMELESSUNION and NONAMELESSSTRUCT. So you wouldn't use U(buffer[i]) (except in the tests, where you can do that), you'd use buffer[i].u .
> So I fall back to my try 1.5 for my tree.

OK, you do that. I'm pretty sure AJ won't take your patch anyway until it's done "right" (i.e. like I said, in ntdll). I guess I'll take that up, since I have nothing better to do ;).

Chip

[PATCH] kernel32: implement GetLogicalProcessorInformation

[Claudio Fontana]

On Fri, Nov 25, 2011 at 8:01 PM, Charles Davis  wrote:
> On Nov 25, 2011, at 8:17 AM, Claudio Fontana wrote:
> > On Fri, Nov 25, 2011 at 4:07 PM, Claudio Fontana
> >  wrote:
> > > I have resent the patch as a "try 2" (there was a try 1.5 to fix test
> > > issues under windows older than XP SP3).
> > >
> > > The patch contains changes based on your feedback.
> >
> > Which is completely broken, since the U() macro you suggested to use
> > is a wine/test.h feature only,
> > and completely broke the patch.
>
> You are right. The way most files (other than tests) in Wine deal with that is that they unconditionally define NONAMELESSUNION and NONAMELESSSTRUCT. So you wouldn't use U(buffer[i]) (except in the tests, where you can do that), you'd use buffer[i].u .
> > So I fall back to my try 1.5 for my tree.
>
> OK, you do that. I'm pretty sure AJ won't take your patch anyway until it's done "right" (i.e. like I said, in ntdll). I guess I'll take that up, since I have nothing better to do ;).

That's great! When it's done it will be one less out of tree patch to
maintain, always a win.

Ciao,

C

[PATCH] kernel32: implement GetLogicalProcessorInformation

[GOUJON Alexandre]

On 11/25/2011 08:35 PM, Claudio Fontana wrote:
> That's great! When it's done it will be one less out of tree patch to
> maintain, always a win.
>
> Ciao,
>
> C

You're free to keep your patches on your own/private tree but if you're 
discussing here, I guess you wanted your stuff being committed in the 
main/official tree.

People here take time to read patches and to give useful advices to make 
patches accepted.

Open Source philosophy is "I know how to do that better, let's build 
together something great", not "I'll code this part myself and keep it 
private".

Of course, it's my two cents but it's sad people react like you.