[dpdk-dev] [PATCH] pmd_virtio: Unchecked return value from library

[dpdk-dev] [PATCH] pmd_virtio: Unchecked return value from library

[SebastianX Basierski]

Check return value from library in order to prevent
potential fail.

Coverity issue: 143439

Fixes: ef53b6030039 ("net/virtio-user: support LSC")
Cc: jianfeng.tan@intel.com
cc: dev@dpdk.org

Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index c961444..16aa350 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -86,7 +86,10 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 			int flags;
 
 			flags = fcntl(dev->vhostfd, F_GETFL);
-			fcntl(dev->vhostfd, F_SETFL, flags | O_NONBLOCK);
+			if (fcntl(dev->vhostfd, F_SETFL,
+					flags | O_NONBLOCK) == -1)
+				return;
+
 			r = recv(dev->vhostfd, buf, 128, MSG_PEEK);
 			if (r == 0 || (r < 0 && errno != EAGAIN)) {
 				dev->status &= (~VIRTIO_NET_S_LINK_UP);
@@ -105,7 +108,10 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 			} else {
 				dev->status |= VIRTIO_NET_S_LINK_UP;
 			}
-			fcntl(dev->vhostfd, F_SETFL, flags & (~O_NONBLOCK));
+			if (fcntl(dev->vhostfd, F_SETFL,
+					flags & ~O_NONBLOCK) == -1)
+				return;
+
 		}
 		*(uint16_t *)dst = dev->status;
 	}
-- 
2.7.4

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

Re: [dpdk-dev] [PATCH] pmd_virtio: Unchecked return value from library

[Tan, Jianfeng]

Hi,

Thank you for those fixes.

> -----Original Message-----
> From: Basierski, SebastianX
> Sent: Tuesday, September 19, 2017 7:47 PM
> To: skhare@vmware.com
> Cc: Basierski, SebastianX; Tan, Jianfeng; dev@dpdk.org
> Subject: [PATCH] pmd_virtio: Unchecked return value from library
> 
> Check return value from library in order to prevent
> potential fail.
> 
> Coverity issue: 143439
> 
> Fixes: ef53b6030039 ("net/virtio-user: support LSC")
> Cc: jianfeng.tan@intel.com
> cc: dev@dpdk.org
> 
> Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
> ---
>  drivers/net/virtio/virtio_user_ethdev.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index c961444..16aa350 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -86,7 +86,10 @@ virtio_user_read_dev_config(struct virtio_hw *hw,
> size_t offset,
>  			int flags;
> 
>  			flags = fcntl(dev->vhostfd, F_GETFL);
> -			fcntl(dev->vhostfd, F_SETFL, flags | O_NONBLOCK);
> +			if (fcntl(dev->vhostfd, F_SETFL,
> +					flags | O_NONBLOCK) == -1)
> +				return;

Actually, even it fails, I still prefer to continue instead of "return" here. Maybe, we can report an error message here.

Thanks,
Jianfeng

Re: [dpdk-dev] [PATCH] pmd_virtio: Unchecked return value from library

[Basierski, SebastianX]

Hi,

Thank you for those fixes.

> -----Original Message-----
> From: Basierski, SebastianX
> Sent: Tuesday, September 19, 2017 7:47 PM
> To: skhare@vmware.com
> Cc: Basierski, SebastianX; Tan, Jianfeng; dev@dpdk.org
> Subject: [PATCH] pmd_virtio: Unchecked return value from library
> 
> Check return value from library in order to prevent potential fail.
> 
> Coverity issue: 143439
> 
> Fixes: ef53b6030039 ("net/virtio-user: support LSC")
> Cc: jianfeng.tan@intel.com
> cc: dev@dpdk.org
> 
> Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
> ---
>  drivers/net/virtio/virtio_user_ethdev.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index c961444..16aa350 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -86,7 +86,10 @@ virtio_user_read_dev_config(struct virtio_hw *hw, 
> size_t offset,
>  			int flags;
> 
>  			flags = fcntl(dev->vhostfd, F_GETFL);
> -			fcntl(dev->vhostfd, F_SETFL, flags | O_NONBLOCK);
> +			if (fcntl(dev->vhostfd, F_SETFL,
> +					flags | O_NONBLOCK) == -1)
> +				return;

Actually, even it fails, I still prefer to continue instead of "return" here. Maybe, we can report an error message here.

Thanks,
Jianfeng

Hi,
I think, we should at least report an error.
Also returning after fcntl failed could save us from operating on corrupted data.

Regards,
Sebastian
--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

Re: [dpdk-dev] [PATCH] pmd_virtio: Unchecked return value from library

[Basierski, SebastianX]

-----Original Message-----
From: Tan, Jianfeng 
Sent: Wednesday, September 20, 2017 2:56 AM
To: Basierski, SebastianX <sebastianx.basierski@intel.com>; skhare@vmware.com
Cc: dev@dpdk.org
Subject: RE: [PATCH] pmd_virtio: Unchecked return value from library

Hi,

Thank you for those fixes.

> -----Original Message-----
> From: Basierski, SebastianX
> Sent: Tuesday, September 19, 2017 7:47 PM
> To: skhare@vmware.com
> Cc: Basierski, SebastianX; Tan, Jianfeng; dev@dpdk.org
> Subject: [PATCH] pmd_virtio: Unchecked return value from library
> 
> Check return value from library in order to prevent potential fail.
> 
> Coverity issue: 143439
> 
> Fixes: ef53b6030039 ("net/virtio-user: support LSC")
> Cc: jianfeng.tan@intel.com
> cc: dev@dpdk.org
> 
> Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
> ---
>  drivers/net/virtio/virtio_user_ethdev.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index c961444..16aa350 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -86,7 +86,10 @@ virtio_user_read_dev_config(struct virtio_hw *hw, 
> size_t offset,
>  			int flags;
> 
>  			flags = fcntl(dev->vhostfd, F_GETFL);
> -			fcntl(dev->vhostfd, F_SETFL, flags | O_NONBLOCK);
> +			if (fcntl(dev->vhostfd, F_SETFL,
> +					flags | O_NONBLOCK) == -1)
> +				return;

Actually, even it fails, I still prefer to continue instead of "return" here. Maybe, we can report an error message here.

Thanks,
Jianfeng



Hi Jianfeng,

I'like to continue with the task.
So please let me know if You are certain with Your option:
continue instead of return (and add error report), despite possibility of returning error from fcntl.

Regards,
Sebastian Basierski

Re: [dpdk-dev] [PATCH] pmd_virtio: Unchecked return value from library

[Basierski, SebastianX]

-----Original Message-----
From: Basierski, SebastianX 
Sent: Friday, September 29, 2017 9:44 AM
To: Tan, Jianfeng <jianfeng.tan@intel.com>; skhare@vmware.com
Cc: dev@dpdk.org
Subject: RE: [PATCH] pmd_virtio: Unchecked return value from library



-----Original Message-----
From: Tan, Jianfeng
Sent: Wednesday, September 20, 2017 2:56 AM
To: Basierski, SebastianX <sebastianx.basierski@intel.com>; skhare@vmware.com
Cc: dev@dpdk.org
Subject: RE: [PATCH] pmd_virtio: Unchecked return value from library

Hi,

Thank you for those fixes.

> -----Original Message-----
> From: Basierski, SebastianX
> Sent: Tuesday, September 19, 2017 7:47 PM
> To: skhare@vmware.com
> Cc: Basierski, SebastianX; Tan, Jianfeng; dev@dpdk.org
> Subject: [PATCH] pmd_virtio: Unchecked return value from library
> 
> Check return value from library in order to prevent potential fail.
> 
> Coverity issue: 143439
> 
> Fixes: ef53b6030039 ("net/virtio-user: support LSC")
> Cc: jianfeng.tan@intel.com
> cc: dev@dpdk.org
> 
> Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
> ---
>  drivers/net/virtio/virtio_user_ethdev.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index c961444..16aa350 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -86,7 +86,10 @@ virtio_user_read_dev_config(struct virtio_hw *hw, 
> size_t offset,
>  			int flags;
> 
>  			flags = fcntl(dev->vhostfd, F_GETFL);
> -			fcntl(dev->vhostfd, F_SETFL, flags | O_NONBLOCK);
> +			if (fcntl(dev->vhostfd, F_SETFL,
> +					flags | O_NONBLOCK) == -1)
> +				return;

Actually, even it fails, I still prefer to continue instead of "return" here. Maybe, we can report an error message here.

Thanks,
Jianfeng



Hi Jianfeng,

I'like to continue with the task.
So please let me know if You are certain with Your option:
continue instead of return (and add error report), despite possibility of returning error from fcntl.

Regards,
Sebastian Basierski

Hi,

I understand You may be busy, but I really have to finish my task.

Please let me know, what is Your decision about patch.

Regards,
Sebastian Basierski

Re: [dpdk-dev] [PATCH] pmd_virtio: Unchecked return value from library

[Tan, Jianfeng]

Hi Sebastian,

Sorry for the late response. I missed the previous emails.

On 10/16/2017 4:32 PM, Basierski, SebastianX wrote:
>>   			flags = fcntl(dev->vhostfd, F_GETFL);
>> -			fcntl(dev->vhostfd, F_SETFL, flags | O_NONBLOCK);
>> +			if (fcntl(dev->vhostfd, F_SETFL,
>> +					flags | O_NONBLOCK) == -1)
>> +				return;
> Actually, even it fails, I still prefer to continue instead of "return" here. Maybe, we can report an error message here.
>
> Thanks,
> Jianfeng
>
>
>
> Hi Jianfeng,
>
> I'like to continue with the task.
> So please let me know if You are certain with Your option:
> continue instead of return (and add error report), despite possibility of returning error from fcntl.

Even "return" directly, please add an error message here so that users 
can notice such error.

Besides, the subject can be changed to:
   "net/virtio: fix unchecked return value"

Thanks,
Jianfeng

[dpdk-dev] [PATCH v2] Report an error message if the flag O_NONBLOCK setting fails, then return from function.

[SebastianX Basierski]

Coverity issue: 143439

Fixes: ef53b6030039 ("net/virtio-user: support LSC")
Cc: jianfeng.tan@intel.com
cc: stable@dpdk.org

Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 57c964d..fe3a7be 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -86,7 +86,11 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 			int flags;
 
 			flags = fcntl(dev->vhostfd, F_GETFL);
-			fcntl(dev->vhostfd, F_SETFL, flags | O_NONBLOCK);
+			if (fcntl(dev->vhostfd, F_SETFL,
+					flags | O_NONBLOCK) == -1){
+				PMD_DRV_LOG(ERR, "error setting O_NONBLOCK flag");
+				return;
+			}
 			r = recv(dev->vhostfd, buf, 128, MSG_PEEK);
 			if (r == 0 || (r < 0 && errno != EAGAIN)) {
 				dev->status &= (~VIRTIO_NET_S_LINK_UP);
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] Report an error message if the flag O_NONBLOCK setting fails, then return from function.

[Tan, Jianfeng]

> -----Original Message-----
> From: Basierski, SebastianX
> Sent: Tuesday, October 24, 2017 1:11 PM
> To: yliu@fridaylinux.org
> Cc: dev@dpdk.org; Basierski, SebastianX; Tan, Jianfeng; stable@dpdk.org
> Subject: [PATCH v2] Report an error message if the flag O_NONBLOCK
> setting fails, then return from function.
> 
> Coverity issue: 143439
> 
> Fixes: ef53b6030039 ("net/virtio-user: support LSC")
> Cc: jianfeng.tan@intel.com
> cc: stable@dpdk.org
> 
> Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>

Except a nit below,
Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>


> ---
>  drivers/net/virtio/virtio_user_ethdev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index 57c964d..fe3a7be 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -86,7 +86,11 @@ virtio_user_read_dev_config(struct virtio_hw *hw,
> size_t offset,
>  			int flags;
> 
>  			flags = fcntl(dev->vhostfd, F_GETFL);
> -			fcntl(dev->vhostfd, F_SETFL, flags | O_NONBLOCK);
> +			if (fcntl(dev->vhostfd, F_SETFL,
> +					flags | O_NONBLOCK) == -1){

There should be a space between ")" and "{".


> +				PMD_DRV_LOG(ERR, "error setting
> O_NONBLOCK flag");
> +				return;
> +			}
>  			r = recv(dev->vhostfd, buf, 128, MSG_PEEK);
>  			if (r == 0 || (r < 0 && errno != EAGAIN)) {
>  				dev->status &= (~VIRTIO_NET_S_LINK_UP);
> --
> 2.7.4

[dpdk-dev] [PATCH v3] net/virtio: fix unchecked return value

[SebastianX Basierski]

Report an error message if the flag O_NONBLOCK setting fails,
then return from function.

Coverity issue: 143439

Fixes: ef53b6030039 ("net/virtio-user: support LSC")
Cc: jianfeng.tan@intel.com
cc: stable@dpdk.org

Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 57c964d..20b7292 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -86,7 +86,11 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 			int flags;
 
 			flags = fcntl(dev->vhostfd, F_GETFL);
-			fcntl(dev->vhostfd, F_SETFL, flags | O_NONBLOCK);
+			if (fcntl(dev->vhostfd, F_SETFL,
+					flags | O_NONBLOCK) == -1) {
+				PMD_DRV_LOG(ERR, "error setting O_NONBLOCK flag");
+				return;
+			}
 			r = recv(dev->vhostfd, buf, 128, MSG_PEEK);
 			if (r == 0 || (r < 0 && errno != EAGAIN)) {
 				dev->status &= (~VIRTIO_NET_S_LINK_UP);
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3] net/virtio: fix unchecked return value

[Yuanhan Liu]

On Tue, Oct 24, 2017 at 10:17:38AM +0200, SebastianX Basierski wrote:
> Report an error message if the flag O_NONBLOCK setting fails,
> then return from function.
> 
> Coverity issue: 143439
> 
> Fixes: ef53b6030039 ("net/virtio-user: support LSC")
> Cc: jianfeng.tan@intel.com
> cc: stable@dpdk.org
> 
> Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
> Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>

Applied to dpdk-next-virtio.

Thanks.

	--yliu

[dpdk-dev] [PATCH v3] net/virtio: fix unchecked return value

[SebastianX Basierski]

Report an error message if the flag O_NONBLOCK setting fails,
then return from function.

Coverity issue: 143439

Fixes: ef53b6030039 ("net/virtio-user: support LSC")
Cc: jianfeng.tan@intel.com
cc: stable@dpdk.org

Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 57c964d..20b7292 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -86,7 +86,11 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 			int flags;
 
 			flags = fcntl(dev->vhostfd, F_GETFL);
-			fcntl(dev->vhostfd, F_SETFL, flags | O_NONBLOCK);
+			if (fcntl(dev->vhostfd, F_SETFL,
+					flags | O_NONBLOCK) == -1) {
+				PMD_DRV_LOG(ERR, "error setting O_NONBLOCK flag");
+				return;
+			}
 			r = recv(dev->vhostfd, buf, 128, MSG_PEEK);
 			if (r == 0 || (r < 0 && errno != EAGAIN)) {
 				dev->status &= (~VIRTIO_NET_S_LINK_UP);
-- 
2.7.4