From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.warmcat.com (mail.warmcat.com [163.172.24.82]) by dpdk.org (Postfix) with ESMTP id 8B52C1C624 for ; Mon, 14 May 2018 06:54:59 +0200 (CEST) To: Matan Azrad , "dev@dpdk.org" References: <152608956198.121204.14844325841690943774.stgit@localhost.localdomain> <152608972460.121204.11116032801548715406.stgit@localhost.localdomain> From: Andy Green Message-ID: Date: Mon, 14 May 2018 12:54:25 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v5 12/16] net/vdev_netvsc: readlink inputs cannot be aliased X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 May 2018 04:54:59 -0000 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 >> 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 >> --- >> 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) >