There exist some thread creation function without result check. This set of patches add result check and message print out after failure. Chengwen Feng (2): telemetry: fix missing check for thread creation test: fix missing check for thread creation app/test/process.h | 8 ++++++-- lib/librte_telemetry/telemetry.c | 30 +++++++++++++++++++++++++++--- lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++-- 3 files changed, 41 insertions(+), 7 deletions(-) -- 2.7.4
From: Chengwen Feng <fengchengwen@huawei.com> Add result check and message print out for thread creation after failure. Fixes: b80fe1805eee ("telemetry: introduce backward compatibility") Cc: stable@dpdk.org Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> --- lib/librte_telemetry/telemetry.c | 30 +++++++++++++++++++++++++++--- lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++-- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c index 7e08afd..b039087 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,16 @@ 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) { + snprintf(telemetry_log_error, + sizeof(telemetry_log_error), + "Error with create thread, telemetry thread " + "quitting"); + close(s_accepted); + return NULL; + } pthread_detach(th); } return NULL; @@ -425,6 +435,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 +451,13 @@ telemetry_legacy_init(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) { + snprintf(telemetry_log_error, sizeof(telemetry_log_error), + "Error with create thread"); + unlink_sockets(); + return -1; + } pthread_setaffinity_np(t_old, sizeof(*thread_cpuset), thread_cpuset); TMTY_LOG(DEBUG, "Legacy telemetry socket initialized ok\n"); @@ -451,6 +468,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 +487,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) { + snprintf(telemetry_log_error, sizeof(telemetry_log_error), + "Error with create thread"); + unlink_sockets(); + return -1; + } 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
From: Chengwen Feng <fengchengwen@huawei.com> There was a call for thread create function without result check. Add result check and message print out after failure. Fixes: 086eb64db39e ("test/pdump: add unit test for pdump library") Cc: stable@dpdk.org Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> --- app/test/process.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/test/process.h b/app/test/process.h index 27f1b1c..9b6b85a 100644 --- a/app/test/process.h +++ b/app/test/process.h @@ -48,6 +48,7 @@ process_dup(const char *const argv[], int numargs, const char *env_value) #ifdef RTE_LIB_PDUMP #ifdef RTE_NET_RING pthread_t thread; + int rc; #endif #endif @@ -126,8 +127,11 @@ process_dup(const char *const argv[], int numargs, const char *env_value) /* parent process does a wait */ #ifdef RTE_LIB_PDUMP #ifdef RTE_NET_RING - if ((strcmp(env_value, "run_pdump_server_tests") == 0)) - pthread_create(&thread, NULL, &send_pkts, NULL); + if ((strcmp(env_value, "run_pdump_server_tests") == 0)) { + rc = pthread_create(&thread, NULL, &send_pkts, NULL); + if (rc) + rte_panic("Cannot start send pkts thread\n"); + } #endif #endif -- 2.7.4
There exist some thread creation function without result check. This set of patches add result check and message print out after failure. Chengwen Feng (2): telemetry: fix missing check for thread creation test: fix missing check for thread creation --- v2: * fix compiling bugs of missig value. app/test/process.h | 8 ++++++-- lib/librte_telemetry/telemetry.c | 28 +++++++++++++++++++++++++--- lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++-- 3 files changed, 39 insertions(+), 7 deletions(-) -- 2.7.4
From: Chengwen Feng <fengchengwen@huawei.com> Add result check and message print out for thread creation after failure. Fixes: b80fe1805eee ("telemetry: introduce backward compatibility") Cc: stable@dpdk.org Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> --- 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) { + 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 @@ telemetry_legacy_init(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(); + 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; + } + 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
From: Chengwen Feng <fengchengwen@huawei.com> There was a call for thread create function without result check. Add result check and message print out after failure. Fixes: 086eb64db39e ("test/pdump: add unit test for pdump library") Cc: stable@dpdk.org Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> --- app/test/process.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/test/process.h b/app/test/process.h index 27f1b1c..9b6b85a 100644 --- a/app/test/process.h +++ b/app/test/process.h @@ -48,6 +48,7 @@ process_dup(const char *const argv[], int numargs, const char *env_value) #ifdef RTE_LIB_PDUMP #ifdef RTE_NET_RING pthread_t thread; + int rc; #endif #endif @@ -126,8 +127,11 @@ process_dup(const char *const argv[], int numargs, const char *env_value) /* parent process does a wait */ #ifdef RTE_LIB_PDUMP #ifdef RTE_NET_RING - if ((strcmp(env_value, "run_pdump_server_tests") == 0)) - pthread_create(&thread, NULL, &send_pkts, NULL); + if ((strcmp(env_value, "run_pdump_server_tests") == 0)) { + rc = pthread_create(&thread, NULL, &send_pkts, NULL); + if (rc) + rte_panic("Cannot start send pkts thread\n"); + } #endif #endif -- 2.7.4
On Mon, Apr 12, 2021 at 2:32 AM Min Hu (Connor) <humin29@huawei.com> wrote: > > From: Chengwen Feng <fengchengwen@huawei.com> > > 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 <fengchengwen@huawei.com> > Signed-off-by: Min Hu (Connor) <humin29@huawei.com> > --- > 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; + } > + 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 > -- David Marchand
There exist some thread creation function without result check. This set of patches add result check and message print out after failure. Chengwen Feng (2): telemetry: fix missing check for thread creation test: fix missing check for thread creation --- v3: * modify return value check and error logging. v2: * fix compiling bugs of missig value. app/test/process.h | 8 ++++++-- lib/librte_telemetry/telemetry.c | 30 +++++++++++++++++++++++++++--- lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++-- 3 files changed, 41 insertions(+), 7 deletions(-) -- 2.7.4
From: Chengwen Feng <fengchengwen@huawei.com> Add result check and message print out for thread creation after failure. Fixes: b80fe1805eee ("telemetry: introduce backward compatibility") Cc: stable@dpdk.org Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> --- lib/librte_telemetry/telemetry.c | 30 +++++++++++++++++++++++++++--- lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++-- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c index 7e08afd..e6a99f3 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,12 @@ 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 != 0) { + TMTY_LOG(ERR, "Error with create client thread\n"); + close(s_accepted); + return NULL; + } pthread_detach(th); } return NULL; @@ -425,6 +431,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 +447,15 @@ telemetry_legacy_init(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 != 0) { + TMTY_LOG(ERR, "Error with create thread for legacy socket\n"); + close(v1_socket.sock); + v1_socket.sock = -1; + unlink(v1_socket.path); + v1_socket.path[0] = '\0'; + 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,15 @@ 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 != 0) { + TMTY_LOG(ERR, "Error with create thread for socket"); + close(v2_socket.sock); + v2_socket.sock = -1; + unlink(v2_socket.path); + v2_socket.path[0] = '\0'; + return -1; + } 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..fd242bf 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 != 0) { + perror("Failed to create legacy client thread\n"); + close(fd); + return -1; + } #endif /* !RTE_EXEC_ENV_WINDOWS */ return 0; } -- 2.7.4
From: Chengwen Feng <fengchengwen@huawei.com> There was a call for thread create function without result check. Add result check and message print out after failure. Fixes: 086eb64db39e ("test/pdump: add unit test for pdump library") Cc: stable@dpdk.org Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> --- app/test/process.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/test/process.h b/app/test/process.h index 27f1b1c..2a0a8da 100644 --- a/app/test/process.h +++ b/app/test/process.h @@ -48,6 +48,7 @@ process_dup(const char *const argv[], int numargs, const char *env_value) #ifdef RTE_LIB_PDUMP #ifdef RTE_NET_RING pthread_t thread; + int rc; #endif #endif @@ -126,8 +127,11 @@ process_dup(const char *const argv[], int numargs, const char *env_value) /* parent process does a wait */ #ifdef RTE_LIB_PDUMP #ifdef RTE_NET_RING - if ((strcmp(env_value, "run_pdump_server_tests") == 0)) - pthread_create(&thread, NULL, &send_pkts, NULL); + if ((strcmp(env_value, "run_pdump_server_tests") == 0)) { + rc = pthread_create(&thread, NULL, &send_pkts, NULL); + if (rc != 0) + rte_panic("Cannot start send pkts thread\n"); + } #endif #endif -- 2.7.4
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) <humin29@huawei.com> wrote: >> >> From: Chengwen Feng <fengchengwen@huawei.com> >> >> 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 <fengchengwen@huawei.com> >> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> >> --- >> 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 >> > > >
On Thu, Apr 15, 2021 at 1:50 PM Min Hu (Connor) <humin29@huawei.com> wrote: > diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c > index 7e08afd..e6a99f3 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,12 @@ 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 != 0) { > + TMTY_LOG(ERR, "Error with create client thread\n"); > + close(s_accepted); > + return NULL; > + } Repeating my comment: Either you use this rc variable (adding strerror(rc) in the log message in TMTY_LOG()) or you remove the variable and simply test if (pthread_create(...) != 0). This comment applies to other changes in this patch. And this applies to patch 2 too. > pthread_detach(th); > } > return NULL; Thanks. -- David Marchand
Hi Connor, Thanks for looking at this. Left some comments inline. Ciara >-----Original Message----- >From: Min Hu (Connor) <humin29@huawei.com> >Sent: Thursday 15 April 2021 12:50 >To: dev@dpdk.org >Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Power, Ciara ><ciara.power@intel.com>; Pattan, Reshma <reshma.pattan@intel.com>; >david.marchand@redhat.com >Subject: [PATCH v3 1/2] telemetry: fix missing check for thread creation > >From: Chengwen Feng <fengchengwen@huawei.com> > >Add result check and message print out for thread creation after failure. > >Fixes: b80fe1805eee ("telemetry: introduce backward compatibility") >Cc: stable@dpdk.org > >Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> >Signed-off-by: Min Hu (Connor) <humin29@huawei.com> >--- > lib/librte_telemetry/telemetry.c | 30 +++++++++++++++++++++++++++- >-- > lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++-- > 2 files changed, 35 insertions(+), 5 deletions(-) > >diff --git a/lib/librte_telemetry/telemetry.c >b/lib/librte_telemetry/telemetry.c >index 7e08afd..e6a99f3 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,12 @@ 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 != 0) { >+ TMTY_LOG(ERR, "Error with create client thread\n"); >+ close(s_accepted); >+ return NULL; I think this should be "continue" instead of "return NULL" - if one client connection thread fails, we probably want to keep listening for more clients rather than stop completely. The "s->num_clients" counter should be decremented here too, it gets incremented above when the connection is accepted, but if we are closing the connection, the counter should reflect that. >+ } > pthread_detach(th); > } > return NULL; >@@ -425,6 +431,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 +447,15 @@ telemetry_legacy_init(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 != 0) { >+ TMTY_LOG(ERR, "Error with create thread for legacy >socket\n"); >+ close(v1_socket.sock); >+ v1_socket.sock = -1; >+ unlink(v1_socket.path); >+ v1_socket.path[0] = '\0'; >+ 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,15 >@@ 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 != 0) { >+ TMTY_LOG(ERR, "Error with create thread for socket"); >+ close(v2_socket.sock); >+ v2_socket.sock = -1; >+ unlink(v2_socket.path); >+ v2_socket.path[0] = '\0'; >+ return -1; >+ } > 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..fd242bf 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 != 0) { >+ perror("Failed to create legacy client thread\n"); >+ close(fd); >+ return -1; >+ } > #endif /* !RTE_EXEC_ENV_WINDOWS */ > return 0; > } >-- >2.7.4
> -----Original Message----- > From: Min Hu (Connor) <humin29@huawei.com> > + if ((strcmp(env_value, "run_pdump_server_tests") == 0)) { > + rc = pthread_create(&thread, NULL, &send_pkts, NULL); > + if (rc != 0) > + rte_panic("Cannot start send pkts thread\n"); > + } I think you still have not addressed the David comment on previous version of the patch. So , can you change this to something like below Option1) rc = pthread_create(&thread, NULL, &send_pkts, NULL); If ( rc ! = 0 ) { rte_panic("Cannot start send pkts thread : %s\n", strerror(rc)); } Or Option2) If ( pthread_create(&thread, NULL, &send_pkts, NULL) !=0 ) rte_panic("Cannot start send pkts thread\n"); Also, >> test: fix missing check for thread creation Change this subject line to "test/pdump: fix missing check for thread creation" Thanks, Reshma
There exist some thread creation function without result check. This set of patches add result check and message print out after failure. Chengwen Feng (2): telemetry: fix missing check for thread creation test: fix missing check for thread creation --- v4: * change error logging. v3: * modify return value check and error logging. v2: * fix compiling bugs of missig value. app/test/process.h | 10 +++++++-- lib/librte_telemetry/telemetry.c | 37 ++++++++++++++++++++++++++++++--- lib/librte_telemetry/telemetry_legacy.c | 11 ++++++++-- 3 files changed, 51 insertions(+), 7 deletions(-) -- 2.7.4
From: Chengwen Feng <fengchengwen@huawei.com> Add result check and message print out for thread creation after failure. Fixes: b80fe1805eee ("telemetry: introduce backward compatibility") Cc: stable@dpdk.org Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> --- lib/librte_telemetry/telemetry.c | 37 ++++++++++++++++++++++++++++++--- lib/librte_telemetry/telemetry_legacy.c | 11 ++++++++-- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c index 7e08afd..08ce189 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,17 @@ 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 != 0) { + TMTY_LOG(ERR, "Error with create client thread: %s\n", + strerror(rc)); + close(s_accepted); + if (s->num_clients != NULL) + __atomic_sub_fetch(s->num_clients, 1, + __ATOMIC_RELAXED); + continue; + } pthread_detach(th); } return NULL; @@ -425,6 +436,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 +452,16 @@ telemetry_legacy_init(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 != 0) { + TMTY_LOG(ERR, "Error with create legcay socket thread: %s\n", + strerror(rc)); + close(v1_socket.sock); + v1_socket.sock = -1; + unlink(v1_socket.path); + v1_socket.path[0] = '\0'; + return -1; + } pthread_setaffinity_np(t_old, sizeof(*thread_cpuset), thread_cpuset); TMTY_LOG(DEBUG, "Legacy telemetry socket initialized ok\n"); @@ -451,6 +472,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 +491,16 @@ 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 != 0) { + TMTY_LOG(ERR, "Error with create socket thread: %s\n", + strerror(rc)); + close(v2_socket.sock); + v2_socket.sock = -1; + unlink(v2_socket.path); + v2_socket.path[0] = '\0'; + return -1; + } 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..b7cd1bd 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,14 @@ 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 != 0) { + fprintf(stderr, "Failed to create legacy client thread: %s\n", + strerror(rc)); + close(fd); + return -1; + } #endif /* !RTE_EXEC_ENV_WINDOWS */ return 0; } -- 2.7.4
From: Chengwen Feng <fengchengwen@huawei.com> There was a call for thread create function without result check. Add result check and message print out after failure. Fixes: 086eb64db39e ("test/pdump: add unit test for pdump library") Cc: stable@dpdk.org Signed-off-by: Chengwen Feng <fengchengwen@huawei.com> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> --- app/test/process.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/app/test/process.h b/app/test/process.h index 27f1b1c..a09a088 100644 --- a/app/test/process.h +++ b/app/test/process.h @@ -48,6 +48,7 @@ process_dup(const char *const argv[], int numargs, const char *env_value) #ifdef RTE_LIB_PDUMP #ifdef RTE_NET_RING pthread_t thread; + int rc; #endif #endif @@ -126,8 +127,13 @@ process_dup(const char *const argv[], int numargs, const char *env_value) /* parent process does a wait */ #ifdef RTE_LIB_PDUMP #ifdef RTE_NET_RING - if ((strcmp(env_value, "run_pdump_server_tests") == 0)) - pthread_create(&thread, NULL, &send_pkts, NULL); + if ((strcmp(env_value, "run_pdump_server_tests") == 0)) { + rc = pthread_create(&thread, NULL, &send_pkts, NULL); + if (rc != 0) { + rte_panic("Cannot start send pkts thread: %s\n", + strerror(rc)); + } + } #endif #endif -- 2.7.4
Thanks Power, v4 has been sent, please check it out.
在 2021/4/16 0:11, Power, Ciara 写道:
> Hi Connor,
>
> Thanks for looking at this. Left some comments inline.
>
> Ciara
>
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
>> Sent: Thursday 15 April 2021 12:50
>> To: dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Power, Ciara
>> <ciara.power@intel.com>; Pattan, Reshma <reshma.pattan@intel.com>;
>> david.marchand@redhat.com
>> Subject: [PATCH v3 1/2] telemetry: fix missing check for thread creation
>>
>> From: Chengwen Feng <fengchengwen@huawei.com>
>>
>> Add result check and message print out for thread creation after failure.
>>
>> Fixes: b80fe1805eee ("telemetry: introduce backward compatibility")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> lib/librte_telemetry/telemetry.c | 30 +++++++++++++++++++++++++++-
>> --
>> lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++--
>> 2 files changed, 35 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/librte_telemetry/telemetry.c
>> b/lib/librte_telemetry/telemetry.c
>> index 7e08afd..e6a99f3 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,12 @@ 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 != 0) {
>> + TMTY_LOG(ERR, "Error with create client thread\n");
>> + close(s_accepted);
>> + return NULL;
>
>
> I think this should be "continue" instead of "return NULL" - if one client connection thread fails,
> we probably want to keep listening for more clients rather than stop completely.
>
> The "s->num_clients" counter should be decremented here too,
> it gets incremented above when the connection is accepted, but if we are closing the connection,
> the counter should reflect that.
>
>
>> + }
>> pthread_detach(th);
>> }
>> return NULL;
>> @@ -425,6 +431,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 +447,15 @@ telemetry_legacy_init(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 != 0) {
>> + TMTY_LOG(ERR, "Error with create thread for legacy
>> socket\n");
>> + close(v1_socket.sock);
>> + v1_socket.sock = -1;
>> + unlink(v1_socket.path);
>> + v1_socket.path[0] = '\0';
>> + 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,15
>> @@ 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 != 0) {
>> + TMTY_LOG(ERR, "Error with create thread for socket");
>> + close(v2_socket.sock);
>> + v2_socket.sock = -1;
>> + unlink(v2_socket.path);
>> + v2_socket.path[0] = '\0';
>> + return -1;
>> + }
>> 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..fd242bf 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 != 0) {
>> + perror("Failed to create legacy client thread\n");
>> + close(fd);
>> + return -1;
>> + }
>> #endif /* !RTE_EXEC_ENV_WINDOWS */
>> return 0;
>> }
>> --
>> 2.7.4
>
> .
>
在 2021/4/16 1:05, Pattan, Reshma 写道: > > >> -----Original Message----- >> From: Min Hu (Connor) <humin29@huawei.com> >> + if ((strcmp(env_value, "run_pdump_server_tests") == 0)) { >> + rc = pthread_create(&thread, NULL, &send_pkts, NULL); >> + if (rc != 0) >> + rte_panic("Cannot start send pkts thread\n"); >> + } > > > I think you still have not addressed the David comment on previous version of the patch. > So , can you change this to something like below > > Option1) > rc = pthread_create(&thread, NULL, &send_pkts, NULL); > If ( rc ! = 0 ) > { > rte_panic("Cannot start send pkts thread : %s\n", strerror(rc)); > } > > Or > > Option2) > If ( pthread_create(&thread, NULL, &send_pkts, NULL) !=0 ) > rte_panic("Cannot start send pkts thread\n"); > > > Also, > >>> test: fix missing check for thread creation > Change this subject line to "test/pdump: fix missing check for thread creation" > Hi, Pattan, fixed in v4, except for "test/pdump", if v4 is OK, please modify it for me, @Ferruh, thanks. > Thanks, > Reshma > > . >
Hi, David, fixed in v4. Thanks.
在 2021/4/15 22:55, David Marchand 写道:
> On Thu, Apr 15, 2021 at 1:50 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>> diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c
>> index 7e08afd..e6a99f3 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,12 @@ 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 != 0) {
>> + TMTY_LOG(ERR, "Error with create client thread\n");
>> + close(s_accepted);
>> + return NULL;
>> + }
>
> Repeating my comment:
>
> Either you use this rc variable (adding strerror(rc) in the log
> message in TMTY_LOG()) or you remove the variable and simply test if
> (pthread_create(...) != 0).
>
> This comment applies to other changes in this patch.
> And this applies to patch 2 too.
>
>> pthread_detach(th);
>> }
>> return NULL;
>
> Thanks.
>
>
> --
> David Marchand
>
> .
>
On 4/16/2021 9:21 AM, Min Hu (Connor) wrote:
>
>
> 在 2021/4/16 1:05, Pattan, Reshma 写道:
>>
>>
>>> -----Original Message-----
>>> From: Min Hu (Connor) <humin29@huawei.com>
>>> + if ((strcmp(env_value, "run_pdump_server_tests") == 0)) {
>>> + rc = pthread_create(&thread, NULL, &send_pkts, NULL);
>>> + if (rc != 0)
>>> + rte_panic("Cannot start send pkts thread\n");
>>> + }
>>
>>
>> I think you still have not addressed the David comment on previous version of
>> the patch.
>> So , can you change this to something like below
>>
>> Option1)
>> rc = pthread_create(&thread, NULL, &send_pkts, NULL);
>> If ( rc ! = 0 )
>> {
>> rte_panic("Cannot start send pkts thread : %s\n", strerror(rc));
>> }
>>
>> Or
>>
>> Option2)
>> If ( pthread_create(&thread, NULL, &send_pkts, NULL) !=0 )
>> rte_panic("Cannot start send pkts thread\n");
>>
>>
>> Also,
>>
>>>> test: fix missing check for thread creation
>> Change this subject line to "test/pdump: fix missing check for thread creation"
>>
> Hi, Pattan, fixed in v4, except for "test/pdump", if v4 is OK, please
> modify it for me, @Ferruh, thanks.
Hi Connor, this patch is on David's domain, he will commit the patch, not me.
在 2021/4/16 16:34, Ferruh Yigit 写道: > On 4/16/2021 9:21 AM, Min Hu (Connor) wrote: >> >> >> 在 2021/4/16 1:05, Pattan, Reshma 写道: >>> >>> >>>> -----Original Message----- >>>> From: Min Hu (Connor) <humin29@huawei.com> >>>> + if ((strcmp(env_value, "run_pdump_server_tests") == 0)) { >>>> + rc = pthread_create(&thread, NULL, &send_pkts, NULL); >>>> + if (rc != 0) >>>> + rte_panic("Cannot start send pkts thread\n"); >>>> + } >>> >>> >>> I think you still have not addressed the David comment on previous >>> version of the patch. >>> So , can you change this to something like below >>> >>> Option1) >>> rc = pthread_create(&thread, NULL, &send_pkts, NULL); >>> If ( rc ! = 0 ) >>> { >>> rte_panic("Cannot start send pkts thread : %s\n", strerror(rc)); >>> } >>> >>> Or >>> >>> Option2) >>> If ( pthread_create(&thread, NULL, &send_pkts, NULL) !=0 ) >>> rte_panic("Cannot start send pkts thread\n"); >>> >>> >>> Also, >>> >>>>> test: fix missing check for thread creation >>> Change this subject line to "test/pdump: fix missing check for >>> thread creation" >>> >> Hi, Pattan, fixed in v4, except for "test/pdump", if v4 is OK, please >> modify it for me, @Ferruh, thanks. > > Hi Connor, this patch is on David's domain, he will commit the patch, > not me. Sorry for that, Ferruh, @David, thanks. > .
Hi Connor,
>-----Original Message-----
>From: Min Hu (Connor) <humin29@huawei.com>
>Sent: Friday 16 April 2021 09:18
>To: dev@dpdk.org
>Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Power, Ciara
><ciara.power@intel.com>; Pattan, Reshma <reshma.pattan@intel.com>;
>david.marchand@redhat.com
>Subject: [PATCH v4 1/2] telemetry: fix missing check for thread creation
>
>From: Chengwen Feng <fengchengwen@huawei.com>
>
>Add result check and message print out for thread creation after failure.
>
>Fixes: b80fe1805eee ("telemetry: introduce backward compatibility")
>Cc: stable@dpdk.org
>
>Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>---
> lib/librte_telemetry/telemetry.c | 37
>++++++++++++++++++++++++++++++---
> lib/librte_telemetry/telemetry_legacy.c | 11 ++++++++--
> 2 files changed, 43 insertions(+), 5 deletions(-)
>
>diff --git a/lib/librte_telemetry/telemetry.c
>b/lib/librte_telemetry/telemetry.c
>index 7e08afd..08ce189 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,17 @@ 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 != 0) {
>+ TMTY_LOG(ERR, "Error with create client thread:
>%s\n",
>+ strerror(rc));
>+ close(s_accepted);
>+ if (s->num_clients != NULL)
>+ __atomic_sub_fetch(s->num_clients, 1,
>+ __ATOMIC_RELAXED);
>+ continue;
>+ }
> pthread_detach(th);
> }
> return NULL;
>@@ -425,6 +436,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 +452,16 @@ telemetry_legacy_init(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 != 0) {
>+ TMTY_LOG(ERR, "Error with create legcay socket thread:
>%s\n",
>+ strerror(rc));
>+ close(v1_socket.sock);
>+ v1_socket.sock = -1;
>+ unlink(v1_socket.path);
>+ v1_socket.path[0] = '\0';
>+ return -1;
>+ }
> pthread_setaffinity_np(t_old, sizeof(*thread_cpuset), thread_cpuset);
>
> TMTY_LOG(DEBUG, "Legacy telemetry socket initialized ok\n"); @@
>-451,6 +472,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 +491,16
>@@ 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 != 0) {
>+ TMTY_LOG(ERR, "Error with create socket thread: %s\n",
>+ strerror(rc));
>+ close(v2_socket.sock);
>+ v2_socket.sock = -1;
>+ unlink(v2_socket.path);
>+ v2_socket.path[0] = '\0';
>+ return -1;
>+ }
> 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..b7cd1bd 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,14 @@ 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 != 0) {
>+ fprintf(stderr, "Failed to create legacy client thread: %s\n",
>+ strerror(rc));
>+ close(fd);
>+ return -1;
>+ }
> #endif /* !RTE_EXEC_ENV_WINDOWS */
> return 0;
> }
>--
>2.7.4
I think there is a Fixes tag missing for commit:
Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
With that added in, the code changes look good to me,
Acked-by: Ciara Power <ciara.power@intel.com>
Thanks!
> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> Subject: [PATCH v4 2/2] test: fix missing check for thread creation
>
> Fixes: 086eb64db39e ("test/pdump: add unit test for pdump library")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
Acked-by: Reshma Pattan <reshma.pattan@intel.com>
> Chengwen Feng (2):
> telemetry: fix missing check for thread creation
> test: fix missing check for thread creation
Applied, thanks