Re: [dpdk-dev] [PATCH v7 3/9] eal: add additional function overrides in windows header files

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

> +#include <Windows.h>

Nit: better use lowercase to support cross-compilation from OS with
case-sensitive FS. It's not directly relevant for this patch. 

> +static inline int
> +eal_get_thread_affinity_mask(pthread_t threadID, unsigned long *cpuset)
> +{
> +	/* Workaround for the lack of a GetThreadAffinityMask()
> +	 *API in Windows
> +	 */
> +		/* obtain previous mask by setting dummy mask */
> +	DWORD dwPrevAffinityMask =
> +		SetThreadAffinityMask((HANDLE) threadID, 0x1);
> +	/* set it back! */
> +	SetThreadAffinityMask((HANDLE) threadID, dwPrevAffinityMask);
> +	*cpuset = dwPrevAffinityMask;
> +	return 0;
> +}

Shouldn't EAL implementation follow DPDK code style? If so, "threadID" and
"dwPrevAffinityMask" violate naming convention, also there's a bogus TAB.

> +static inline int
> +eal_create_thread(void *threadID, void *threadfunc, void *args)
> +{
> +	HANDLE hThread;
> +	hThread = CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)threadfunc,
> +		args, 0, (LPDWORD)threadID);
> +	if (hThread) {
> +		SetPriorityClass(GetCurrentProcess(), REALTIME_PRIORITY_CLASS);
> +		SetThreadPriority(hThread, THREAD_PRIORITY_TIME_CRITICAL);
> +	}
> +	return ((hThread != NULL) ? 0 : E_FAIL);
> +}

Duplicates eal_thread_create() from "eal_thread.c", probably
eal_thread_create() should call pthread_create() as in Linux EAL.

> +static inline int
> +pthread_join(pthread_t thread __attribute__((__unused__)),
> +	void **value_ptr __attribute__((__unused__)))
> +{
> +	return 0;
> +}

If not implemented, should return EINVAL (ENOSUP is not listed as a valid
error code for this function). Even better, implement trivially with
WaitForSingleObject().


> +static inline int
> +asprintf(char **buffer, const char *format, ...)
> +{
> +	int size, ret;
> +	va_list arg;
> +
> +	va_start(arg, format);
> +	size = vsnprintf(NULL, 0, format, arg) + 1;
> +	va_end(arg);
> +
> +	*buffer = malloc(size);

Missing a check for NULL from malloc().

> +
> +	va_start(arg, format);
> +	ret = vsnprintf(*buffer, size, format, arg);
> +	va_end(arg);
> +	if (ret != size - 1) {
> +		free(*buffer);
> +		return -1;
> +	}
> +	return ret;
> +}

> +static inline int
> +count_cpu(rte_cpuset_t *s)
> +{
> +	unsigned int _i;

Why the underscore? It's not a macro, identifiers are function-local.

> +/*
> + * List definitions.
> + */
> +#define	LIST_HEAD(name, type)						\
> +struct name {								\
> +	struct type *lh_first;	/* first element */			\
> +}
> +
>  #define	QMD_TRACE_ELEM(elem)
>  #define	QMD_TRACE_HEAD(head)
>  #define	TRACEBUF

Probably ouf of scope for this patchset, but DPDK should probably include
the entire FreeBSD <sys/queue.h> so that we won't need to add it
piece-by-piece like so. It's BSD-3-Clause anyway.

-- 
Dmitry Kozlyuk