Re: [dpdk-dev] [PATCH v3] eal/windows: fix invalid thread handle

[Tasnim Bashar <tbashar@mellanox.com>]

> From: Thomas Monjalon <thomas@monjalon.net>
> 02/06/2020 04:00, Tasnim Bashar:
> > Casting thread ID to handle is not accurate way to get thread handle.
> > Need to use OpenThread function to get thread handle from thread ID.
> >
> > pthread_setaffinity_np and pthread_getaffinity_np functions for
> > Windows are affected because of it.
> >
> > Signed-off-by: Tasnim Bashar <tbashar@mellanox.com>
> > ---
> > v3: WA to remove warning(-Wmaybe-uninitialized)
> 
> The -Wmaybe-uninitialized warning was there before this patch.
> Shouldn't it be a separate patch before this one?

The warning appeared only on this patch, so we don't need to separate it
> 
> >  static inline int
> > -eal_get_thread_affinity_mask(pthread_t threadid, unsigned long
> > *cpuset)
> > +eal_get_thread_affinity_mask(pthread_t threadid, rte_cpuset_t
> > +*cpuset)
> >  {
> >  	/* Workaround for the lack of a GetThreadAffinityMask()
> >  	 *API in Windows
> >  	 */
> > -		/* obtain previous mask by setting dummy mask */
> > -	DWORD dwprevaffinitymask =
> > -		SetThreadAffinityMask((HANDLE) threadid, 0x1);
> > +	DWORD_PTR dwprevaffinitymask;
> 
> Please use underscores to separate parts in names.
> 
> > +	HANDLE thread_handle;
> > +	DWORD_PTR ret;
> > +
> > +	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
> > +	if (thread_handle == NULL) {
> > +		RTE_LOG_WIN32_ERR("OpenThread()");
> > +		return -1;
> > +	}
> > +
> > +	/* obtain previous mask by setting dummy mask */
> > +	dwprevaffinitymask = SetThreadAffinityMask(thread_handle, 0x1);
> > +	if (dwprevaffinitymask == 0) {
> > +		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
> > +		CloseHandle(thread_handle);
> > +		return -1;
> > +	}
> > +
> >  	/* set it back! */
> > -	SetThreadAffinityMask((HANDLE) threadid, dwprevaffinitymask);
> > -	*cpuset = dwprevaffinitymask;
> > +	ret = SetThreadAffinityMask(thread_handle, dwprevaffinitymask);
> > +	if (ret == 0) {
> > +		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
> > +		CloseHandle(thread_handle);
> > +		return -1;
> > +	}
> > +	memset(cpuset, 0, sizeof(rte_cpuset_t));
> 
> Shouldn't we use RTE_CPU_ZERO instead of memset?
If we use CPU_ZERO or CPU_SET, we still get the same warning!
> 
> > +	*cpuset->_bits = dwprevaffinitymask;
> > +	CloseHandle(thread_handle);
> >  	return 0;
> >  }
> 
>