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

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

[SebastianX Basierski]

Report error message if clearing O_NONBLOCK flag will fail,
then return from function.

Coverity issue: 143439

Fixes: ef53b6030039 ("net/virtio-user: support LSC")
Cc: jianfeng.tan@intel.com
Cc: yliu@fridaylinux.org
cc: dev@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 7be57ce..c1f7a64 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -109,7 +109,11 @@ 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) {
+				PMD_DRV_LOG(ERR, "error clearing O_NONBLOCK flag");
+				return;
+			}
 		}
 		*(uint16_t *)dst = dev->status;
 	}
-- 
2.7.4

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

[Tan, Jianfeng]

Hi Sebastian,

The title can be refined as:

     net/virtio-user: fix unchecked return value

Other than the above comment, it seems good to me.

On 11/13/2017 9:38 PM, SebastianX Basierski wrote:
> Report error message if clearing O_NONBLOCK flag will fail,
> then return from function.
>
> Coverity issue: 143439
>
> Fixes: ef53b6030039 ("net/virtio-user: support LSC")
> Cc: jianfeng.tan@intel.com
> Cc: yliu@fridaylinux.org
> cc: dev@dpdk.org
>
> Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>

Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>

Thank you for the fix.

Thanks,
Jianfeng

> ---
>   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 7be57ce..c1f7a64 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -109,7 +109,11 @@ 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) {
> +				PMD_DRV_LOG(ERR, "error clearing O_NONBLOCK flag");
> +				return;
> +			}
>   		}
>   		*(uint16_t *)dst = dev->status;
>   	}

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

[SebastianX Basierski]

Report error message if clearing O_NONBLOCK flag will fail,
then return from function.

---
v2:
	Patch title changed.
---

Coverity issue: 143439

Fixes: ef53b6030039 ("net/virtio-user: support LSC")
Cc: jianfeng.tan@intel.com
Cc: yliu@fridaylinux.org
cc: dev@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 7be57ce..c1f7a64 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -109,7 +109,11 @@ 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) {
+				PMD_DRV_LOG(ERR, "error clearing O_NONBLOCK flag");
+				return;
+			}
 		}
 		*(uint16_t *)dst = dev->status;
 	}
-- 
2.7.4

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

[Maxime Coquelin]

Hi Sebastian,

On 11/14/2017 10:27 AM, SebastianX Basierski wrote:
> Report error message if clearing O_NONBLOCK flag will fail,
> then return from function.
> 
> ---
> v2:
> 	Patch title changed.
> ---

^^^^The above note should be placed just before diffstats to be removed
at apply time.

> Coverity issue: 143439
> 
> Fixes: ef53b6030039 ("net/virtio-user: support LSC")
> Cc: jianfeng.tan@intel.com
> Cc: yliu@fridaylinux.org
> cc: dev@dpdk.org
> 
> Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
> Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---

Here

>   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 7be57ce..c1f7a64 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -109,7 +109,11 @@ 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) {
> +				PMD_DRV_LOG(ERR, "error clearing O_NONBLOCK flag");
> +				return;
> +			}
>   		}
>   		*(uint16_t *)dst = dev->status;
>   	}
> 

Other than that:
Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

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

[SebastianX Basierski]

Report error message if clearing O_NONBLOCK flag will fail,
then return from function.

Coverity issue: 143439

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

Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>
Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
v2:
        Patch title changed.
---
 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 7be57ce..c1f7a64 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -109,7 +109,11 @@ 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) {
+				PMD_DRV_LOG(ERR, "error clearing O_NONBLOCK flag");
+				return;
+			}
 		}
 		*(uint16_t *)dst = dev->status;
 	}
-- 
2.7.4

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

[Thomas Monjalon]

14/11/2017 11:39, SebastianX Basierski:
> Report error message if clearing O_NONBLOCK flag will fail,
> then return from function.
> 
> Coverity issue: 143439
> 
> Fixes: ef53b6030039 ("net/virtio-user: support LSC")
> Cc: jianfeng.tan@intel.com
> Cc: yliu@fridaylinux.org
> cc: dev@dpdk.org
> 
> Signed-off-by: SebastianX Basierski <sebastianx.basierski@intel.com>
> Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>
> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I don't know why this patch was forgotten in the virtio tree.

Applied, thanks

[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