diff mbox

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

Message ID 152608972460.121204.11116032801548715406.stgit@localhost.localdomain (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Andy Green May 12, 2018, 1:48 a.m. UTC 
  /home/agreen/projects/dpdk/drivers/net/vdev_netvsc/
vdev_netvsc.c:335:2:error: passing argument 2 to restrict-
qualified parameter aliases with argument 1 [-Werror=restrict]
  ret = readlink(buf, buf, size);
  ^~~

Signed-off-by: Andy Green <andy@warmcat.com>
Fixes: e7dc5d7becc5 ("net/vdev_netvsc: implement core functionality")
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(-)

Comments

Matan Azrad May 13, 2018, 7:20 a.m. UTC | #1 
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?
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?

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.
 
> 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?

>  	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.
 
>  		return -ENOBUFS;

> -	ret = readlink(buf, buf, size);

> +	ret = readlink(in, buf, size);

>  	if (ret == -1)

>  		return -errno;

>  	if ((size_t)ret >= size - 1)
Andy Green May 14, 2018, 4:54 a.m. UTC | #2 
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)
>
diff mbox

Patch

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];
 	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)
 		return -ENOBUFS;
-	ret = readlink(buf, buf, size);
+	ret = readlink(in, buf, size);
 	if (ret == -1)
 		return -errno;
 	if ((size_t)ret >= size - 1)