diff mbox

[dpdk-dev] net/failsafe: fix for missing pclose after popen

Message ID 1501765798-14200-1-git-send-email-rasland@mellanox.com (mailing list archive)
State Accepted, archived
Headers

Checks

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

Commit Message

Raslan Darawsheh Aug. 3, 2017, 1:09 p.m. UTC 
From: Raslan Darawsheh <rasland@mellanox.com>

When there is no prefered device, failsafe will always
try to scan for prefered device. And if there is no device
found with the exec option, popen() will get an empty output.
In this case, it was forgotten to close the file descriptor.o
it is fixed by closing the file descriptor even if the output is emtpy.

Fixes: a0194d82 ("net/failsafe: add flexible device definition")

Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
---
 drivers/net/failsafe/failsafe_args.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Gaëtan Rivet Aug. 3, 2017, 1:58 p.m. UTC | #1 
Hi Raslan,

I sent a patch earlier regarding this but we will use yours.
Two small nits below.

On Thu, Aug 03, 2017 at 04:09:58PM +0300, rasland@mellanox.com wrote:
> From: Raslan Darawsheh <rasland@mellanox.com>
> 
> When there is no prefered device, failsafe will always
> try to scan for prefered device. And if there is no device
> found with the exec option, popen() will get an empty output.
> In this case, it was forgotten to close the file descriptor.o
> it is fixed by closing the file descriptor even if the output is emtpy.
> 

Good job on finding the issue and fixing it.
It has been assigned a coverity ID:

Coverity issue: 158633
> Fixes: a0194d82 ("net/failsafe: add flexible device definition")
> 

The fixline should be 12-char long I think:
Fixes: a0194d828100 ("net/failsafe: add flexible device definition")

> Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>

Otherwise:

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  drivers/net/failsafe/failsafe_args.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
> index 932e371..3f92a77 100644
> --- a/drivers/net/failsafe/failsafe_args.c
> +++ b/drivers/net/failsafe/failsafe_args.c
> @@ -115,7 +115,7 @@ fs_execute_cmd(struct sub_device *sdev, char *cmdline)
>  	char output[DEVARGS_MAXLEN + 1];
>  	size_t len;
>  	int old_err;
> -	int ret;
> +	int ret, pclose_ret;
>  
>  	RTE_ASSERT(cmdline != NULL || sdev->cmdline != NULL);
>  	if (sdev->cmdline == NULL) {
> @@ -145,7 +145,8 @@ fs_execute_cmd(struct sub_device *sdev, char *cmdline)
>  	/* We only read one line */
>  	if (fgets(output, sizeof(output) - 1, fp) == NULL) {
>  		DEBUG("Could not read command output");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto ret_pclose;
>  	}
>  	fs_sanitize_cmdline(output);
>  	ret = fs_parse_device(sdev, output);
> @@ -154,12 +155,12 @@ fs_execute_cmd(struct sub_device *sdev, char *cmdline)
>  		goto ret_pclose;
>  	}
>  ret_pclose:
> -	ret = pclose(fp);
> -	if (ret) {
> -		ret = errno;
> +	pclose_ret = pclose(fp);
> +	if (pclose_ret) {
> +		pclose_ret = errno;
>  		ERROR("pclose: %s", strerror(errno));
>  		errno = old_err;
> -		return ret;
> +		return pclose_ret;
>  	}
>  	return ret;
>  }
> -- 
> 2.7.4
>
Thomas Monjalon Aug. 3, 2017, 8:32 p.m. UTC | #2 
03/08/2017 15:58, Gaëtan Rivet:
> Hi Raslan,
> 
> I sent a patch earlier regarding this but we will use yours.
> Two small nits below.
> 
> On Thu, Aug 03, 2017 at 04:09:58PM +0300, rasland@mellanox.com wrote:
> > From: Raslan Darawsheh <rasland@mellanox.com>
> > 
> > When there is no prefered device, failsafe will always
> > try to scan for prefered device. And if there is no device
> > found with the exec option, popen() will get an empty output.
> > In this case, it was forgotten to close the file descriptor.o
> > it is fixed by closing the file descriptor even if the output is emtpy.
> > 
> 
> Good job on finding the issue and fixing it.
> It has been assigned a coverity ID:
> 
> Coverity issue: 158633
> > Fixes: a0194d82 ("net/failsafe: add flexible device definition")
> > 
> 
> The fixline should be 12-char long I think:
> Fixes: a0194d828100 ("net/failsafe: add flexible device definition")
> 
> > Signed-off-by: Raslan Darawsheh <rasland@mellanox.com>
> 
> Otherwise:
> 
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Applied, thanks
diff mbox

Patch

diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
index 932e371..3f92a77 100644
--- a/drivers/net/failsafe/failsafe_args.c
+++ b/drivers/net/failsafe/failsafe_args.c
@@ -115,7 +115,7 @@  fs_execute_cmd(struct sub_device *sdev, char *cmdline)
 	char output[DEVARGS_MAXLEN + 1];
 	size_t len;
 	int old_err;
-	int ret;
+	int ret, pclose_ret;
 
 	RTE_ASSERT(cmdline != NULL || sdev->cmdline != NULL);
 	if (sdev->cmdline == NULL) {
@@ -145,7 +145,8 @@  fs_execute_cmd(struct sub_device *sdev, char *cmdline)
 	/* We only read one line */
 	if (fgets(output, sizeof(output) - 1, fp) == NULL) {
 		DEBUG("Could not read command output");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto ret_pclose;
 	}
 	fs_sanitize_cmdline(output);
 	ret = fs_parse_device(sdev, output);
@@ -154,12 +155,12 @@  fs_execute_cmd(struct sub_device *sdev, char *cmdline)
 		goto ret_pclose;
 	}
 ret_pclose:
-	ret = pclose(fp);
-	if (ret) {
-		ret = errno;
+	pclose_ret = pclose(fp);
+	if (pclose_ret) {
+		pclose_ret = errno;
 		ERROR("pclose: %s", strerror(errno));
 		errno = old_err;
-		return ret;
+		return pclose_ret;
 	}
 	return ret;
 }