Re: [dpdk-dev] [PATCH v5 12/16] net/vdev_netvsc: readlink inputs cannot be aliased

[Andy Green <andy@warmcat.com>]

On 05/13/2018 03:20 PM, Matan Azrad wrote:
> Hi Andy
> 
> From: Andy Green
>> /home/agreen/projects/dpdk/drivers/net/vdev_netvsc/
> 
> Please replace "/home/agreen/projects/dpdk" in $DPDK_DIR,
> I think this is relevant for all the series.
> 
>> vdev_netvsc.c:335:2:error: passing argument 2 to restrict- qualified parameter
>> aliases with argument 1 [-Werror=restrict]
>>    ret = readlink(buf, buf, size);
>>    ^~~
> 
> Where this compilation error does come from?
> What is the ARCH\gcc version? Why was it compiled well and now it not?

Because as the series cover page says, these problems are all coming 
from Fedora 28 + gcc8.0.1 (on x86_64) which has some very cool new 
static analysis features.

In each case gcc8 has complained, the code was broken actually, the 
tools until now were not good enough to tell us the stuff needed fixing 
is why it's coming now.

> And the title should be something like, fix compilation issue in [distro X][arch Y] [gcc Z] [any other compilation specifications]
> Please specify only the specification which causes the error.
> I think this is relevant for all the series too.
> 
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> Fixes: e7dc5d7becc5 ("net/vdev_netvsc: implement core functionality")
> What's about backporting it to stable?

That's something for the maintainer(s) to take care about.

> The fixes line (and Cc lines) should be before the Signed-off-by line and an empty line should be between them,
> I think this is relevant for all the series too.

 From my perspective, which is not that of a paid dev for this project 
who has now spent a week on these patches, functionally it doesn't 
really matter which order those things are in.

If the maintainer really cares he can munge the patches to be what he 
prefers.

If this turns into a job for me and I have more patches later, I will 
try to align with the project-specific requirements better.

>> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>> ---
>>   drivers/net/vdev_netvsc/vdev_netvsc.c |    8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c
>> b/drivers/net/vdev_netvsc/vdev_netvsc.c
>> index c321a9f1b..dca25761d 100644
>> --- a/drivers/net/vdev_netvsc/vdev_netvsc.c
>> +++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
>> @@ -327,12 +327,14 @@ static int
>>   vdev_netvsc_sysfs_readlink(char *buf, size_t size, const char *if_name,
>>   			   const char *relpath)
>>   {
>> +	char in[160];
> 
> Where the number 160 is come from?
> Why not RTE_MAX(sizeof(ctx->yield), 256u) as defined for buf?

You're right, it's just a random pathlength-like number.
I borrowed the sizing code you mentioned from the function below instead 
(whose random pathlength-like number is at least consistent with the 
rest of the code).

>>   	int ret;
>>
>> -	ret = snprintf(buf, size, "/sys/class/net/%s/%s", if_name, relpath);
>> -	if (ret == -1 || (size_t)ret >= size)
>> +	ret = snprintf(in, sizeof(in) - 1, "/sys/class/net/%s/%s",
>> +		       if_name, relpath);
>> +	if (ret == -1 || (size_t)ret >= sizeof(in) - 1)
> 
> I don’t think you need the " - 1" here.

Yes, I changed it thanks.

-Andy

>>   		return -ENOBUFS;
>> -	ret = readlink(buf, buf, size);
>> +	ret = readlink(in, buf, size);
>>   	if (ret == -1)
>>   		return -errno;
>>   	if ((size_t)ret >= size - 1)
>