Re: [PATCH v1 02/31] net/ntnic: add thread check return code

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 21 Jan 2025 18:07:40 +0100
Serhii Iliushyk <sil-plv@napatech.com> wrote:

> From: Danylo Vodopianov <dvo-plv@napatech.com>
> 
> CI found couple coverity problems which were fixed in this commit.
> 
> CID: 448965 Error handling issues (CHECKED_RETURN).
> 
> Thread return code check was added.
> 
> Coverity issue: 448965
> Fixes: a1ba8c473f5c ("net/ntnic: add statistics poll")
> 
> Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
> ---
>  drivers/net/ntnic/ntnic_ethdev.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ntnic/ntnic_ethdev.c b/drivers/net/ntnic/ntnic_ethdev.c
> index 2a2643a106..620d023a71 100644
> --- a/drivers/net/ntnic/ntnic_ethdev.c
> +++ b/drivers/net/ntnic/ntnic_ethdev.c
> @@ -2516,8 +2516,11 @@ static int init_shutdown(void)
>  	NT_LOG(DBG, NTNIC, "Starting shutdown handler");
>  	kill_pmd = 0;
>  	previous_handler = signal(SIGINT, signal_handler_func_int);
> -	THREAD_CREATE(&shutdown_tid, shutdown_thread, NULL);
> -
> +	int ret = THREAD_CREATE(&shutdown_tid, shutdown_thread, NULL);
> +	if (ret != 0) {
> +		NT_LOG(ERR, NTNIC, "Failed to create shutdown thread, error code: %d", ret);
> +		return -1;
> +	}
>  	/*
>  	 * 1 time calculation of 1 sec stat update rtc cycles to prevent stat poll
>  	 * flooding by OVS from multiple virtual port threads - no need to be precise
> -- 

The whole thread, SIGINT stuff is a big mistake.
You are making all sorts of assumptions about application environment
such as SIGINT won't be used by other things, drivers creating internal threads
is a bad idea.

Also access to the flag kill_pmd is not being done in a thread safe manner.

What is it really trying to do?

IMHO, The driver should have never been merged with this stuff.