Re: [PATCH v4 06/17] eal: add eal_common_thread.c for common thread API

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

Hi,

On 02/09/2015 02:12 PM, Liang, Cunming wrote:
>>> +int
>>> +rte_thread_get_affinity(rte_cpuset_t *cpusetp)
>>> +{
>>> +	if (!cpusetp)
>>> +		return -1;
>>
>> Same here. This is the only reason why rte_thread_get_affinity() could
>> fail. Removing this test would allow to change the API to return void
>> instead. It will avoid a useless test below in
>> eal_thread_dump_affinity().
> [LCM] The cpusetp is used as destination of memcpy and the function suppose an EAL API.
> I don't think it's a good idea to remove the check, do you ?

I know we often have debate on this subject on the list. My personal
opinion is that checking a NULL pointer in these cases is useless
because the user is suppose to give a non-NULL pointer. Returning
an error will result in managing an error for something that cannot
happen.

On the other hand, adding an assert() (or the dpdk equivalent) would
be a good idea.


>>
>>> +
>>> +	rte_memcpy(cpusetp, &RTE_PER_LCORE(_cpuset),
>>> +		   sizeof(rte_cpuset_t));
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +void
>>> +eal_thread_dump_affinity(char str[], unsigned size)
>>> +{
>>> +	rte_cpuset_t cpuset;
>>> +	unsigned cpu;
>>> +	int ret;
>>> +	unsigned int out = 0;
>>> +
>>> +	if (rte_thread_get_affinity(&cpuset) < 0) {
>>> +		str[0] = '\0';
>>> +		return;
>>> +	}
>>
>> This one could be removed it the (== NULL) test is removed.
>>
>>> +
>>> +	for (cpu = 0; cpu < RTE_MAX_LCORE; cpu++) {
>>> +		if (!CPU_ISSET(cpu, &cpuset))
>>> +			continue;
>>> +
>>> +		ret = snprintf(str + out,
>>> +			       size - out, "%u,", cpu);
>>> +		if (ret < 0 || (unsigned)ret >= size - out)
>>> +			break;
>>
>> On the contrary, I think here returning an error to the user
>> would be useful so he can knows that the dump is not complete.
> [LCM] accept.
>>
>>
>> Regards,
>> Olivier