From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 98692A0C3F; Thu, 15 Apr 2021 13:53:53 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5714E162176; Thu, 15 Apr 2021 13:53:53 +0200 (CEST) Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mails.dpdk.org (Postfix) with ESMTP id C6D07162172 for ; Thu, 15 Apr 2021 13:53:51 +0200 (CEST) Received: from DGGEMS410-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4FLd2l0Sp2zpXRb; Thu, 15 Apr 2021 19:50:55 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS410-HUB.china.huawei.com (10.3.19.210) with Microsoft SMTP Server id 14.3.498.0; Thu, 15 Apr 2021 19:53:47 +0800 To: David Marchand CC: dev , "Yigit, Ferruh" , Ciara Power , "Pattan, Reshma" References: <1618051453-21264-1-git-send-email-humin29@huawei.com> <1618187540-55614-1-git-send-email-humin29@huawei.com> <1618187540-55614-2-git-send-email-humin29@huawei.com> From: "Min Hu (Connor)" Message-ID: Date: Thu, 15 Apr 2021 19:53:48 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v2 1/2] telemetry: fix missing check for thread creation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, David, ALl has been fixed in v3, please check it out, thanks. 在 2021/4/12 15:48, David Marchand 写道: > On Mon, Apr 12, 2021 at 2:32 AM Min Hu (Connor) wrote: >> >> From: Chengwen Feng >> >> Add result check and message print out for thread creation after >> failure. > > Ah, I was looking at this code too, while looking at your other series :-). > > >> >> Fixes: b80fe1805eee ("telemetry: introduce backward compatibility") >> Cc: stable@dpdk.org >> >> Signed-off-by: Chengwen Feng >> Signed-off-by: Min Hu (Connor) >> --- >> lib/librte_telemetry/telemetry.c | 28 +++++++++++++++++++++++++--- >> lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++-- >> 2 files changed, 33 insertions(+), 5 deletions(-) >> >> diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c >> index 7e08afd..3f1a4c4 100644 >> --- a/lib/librte_telemetry/telemetry.c >> +++ b/lib/librte_telemetry/telemetry.c >> @@ -350,6 +350,7 @@ socket_listener(void *socket) >> { >> while (1) { >> pthread_t th; >> + int rc; >> struct socket *s = (struct socket *)socket; >> int s_accepted = accept(s->sock, NULL, NULL); >> if (s_accepted < 0) { >> @@ -366,7 +367,14 @@ socket_listener(void *socket) >> __atomic_add_fetch(s->num_clients, 1, >> __ATOMIC_RELAXED); >> } >> - pthread_create(&th, NULL, s->fn, (void *)(uintptr_t)s_accepted); >> + rc = pthread_create(&th, NULL, s->fn, >> + (void *)(uintptr_t)s_accepted); >> + if (rc) { > > Either you use this rc variable (adding strerror(rc) in the log > message below) or you can remove it. > This comment applies to others updates below. > > >> + TMTY_LOG(ERR, "Error with create thread, telemetry thread quitting\n"); >> + close(s_accepted); >> + return NULL; >> + } >> + >> pthread_detach(th); >> } >> return NULL; >> @@ -425,6 +433,7 @@ static int >> telemetry_legacy_init(void) >> { >> pthread_t t_old; >> + int rc; >> >> if (num_legacy_callbacks == 1) { >> TMTY_LOG(WARNING, "No legacy callbacks, legacy socket not created\n"); >> @@ -440,7 +449,13 @@ Fixes: 3b3757bda3c3 ("net/ice: get VF hardware index in DCF") > (void) >> v1_socket.sock = create_socket(v1_socket.path); >> if (v1_socket.sock < 0) >> return -1; >> - pthread_create(&t_old, NULL, socket_listener, &v1_socket); >> + rc = pthread_create(&t_old, NULL, socket_listener, &v1_socket); >> + if (rc) { >> + TMTY_LOG(ERR, "Error with create thread\n"); >> + unlink_sockets(); > > Before this patch, v2 mode could be working fine with v1 ko. > I think it was intended since telemetry_legacy_init return value is > not checked and unlink_sockets() checks whether the v1 and v2 sockets > have been created. > > Plus, in this error handling, the v1 socket fd is left open. > > How about: > > + ret = pthread_create(&t_old, NULL, socket_listener, &v1_socket); > + if (ret != 0) { > + TMTY_LOG(DEBUG, "Failed to create handler for legacy > socket: %s\n", > + strerror(ret)); > + close(v1_socket.sock); > + v1_socket.sock = -1; > + if (v1_socket.path[0]) { > + unlink(v1_socket.path); > + v1_socket.path[0] = '\0'; > + } > + return -1; > + } > ok, but there is no need to judge v1_socket.path[0] because previous setup will ensure it's validation. > >> + return -1; >> + } >> + >> pthread_setaffinity_np(t_old, sizeof(*thread_cpuset), thread_cpuset); >> >> TMTY_LOG(DEBUG, "Legacy telemetry socket initialized ok\n"); >> @@ -451,6 +466,7 @@ static int >> telemetry_v2_init(void) >> { >> pthread_t t_new; >> + int rc; >> >> v2_socket.num_clients = &v2_clients; >> rte_telemetry_register_cmd("/", list_commands, >> @@ -469,7 +485,13 @@ telemetry_v2_init(void) >> v2_socket.sock = create_socket(v2_socket.path); >> if (v2_socket.sock < 0) >> return -1; >> - pthread_create(&t_new, NULL, socket_listener, &v2_socket); >> + rc = pthread_create(&t_new, NULL, socket_listener, &v2_socket); >> + if (rc) { >> + TMTY_LOG(ERR, "Error with create thread\n"); >> + unlink_sockets(); >> + return -1; >> + } >> + > > Missing a close on v2 socket, > > >> pthread_setaffinity_np(t_new, sizeof(*thread_cpuset), thread_cpuset); >> atexit(unlink_sockets); >> >> diff --git a/lib/librte_telemetry/telemetry_legacy.c b/lib/librte_telemetry/telemetry_legacy.c >> index 5e9af37..9587bfe 100644 >> --- a/lib/librte_telemetry/telemetry_legacy.c >> +++ b/lib/librte_telemetry/telemetry_legacy.c >> @@ -83,6 +83,7 @@ register_client(const char *cmd __rte_unused, const char *params, >> pthread_t th; >> char data[BUF_SIZE]; >> int fd; >> + int rc; >> struct sockaddr_un addrs; >> #endif /* !RTE_EXEC_ENV_WINDOWS */ >> >> @@ -112,8 +113,13 @@ register_client(const char *cmd __rte_unused, const char *params, >> close(fd); >> return -1; >> } >> - pthread_create(&th, NULL, &legacy_client_handler, >> - (void *)(uintptr_t)fd); >> + rc = pthread_create(&th, NULL, &legacy_client_handler, >> + (void *)(uintptr_t)fd); >> + if (rc) { >> + fprintf(stderr, "Failed to create thread\n"); >> + close(fd); >> + return -1; >> + } >> #endif /* !RTE_EXEC_ENV_WINDOWS */ >> return 0; >> } >> -- >> 2.7.4 >> > > >