| Message ID | 20220422235944.2808227-6-dominique.martinet@atmark-techno.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/6] sigchld_handler: report child exit status correctly | expand |
On 23.04.22 01:59, Dominique Martinet wrote: > async_thread_id was set but never used, > start_ipc_thread returning -1 which isn't a valid pthread_t, > and handle used as a bool with a weird ++ pattern: > clean this all up by replacing handle with a proper state enum > (init, started, done), and join when we try to start thread again > after it's been done successfully > We still never join the thread when exiting after it's done once, > nor do we care about its pthread_exit return value, but at least > we don't leak resources everytime a new thread is started > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > > This is mostly a cosmetic change, as I saw a warning for the return -1 > on musl where pthread_t isn't an int. > It doesn't solve any hard problem for me. > > > ipc/network_ipc-if.c | 35 +++++++++++++++++++++++------------ > 1 file changed, 23 insertions(+), 12 deletions(-) > > diff --git a/ipc/network_ipc-if.c b/ipc/network_ipc-if.c > index bf946648edcf..ed0993929834 100644 > --- a/ipc/network_ipc-if.c > +++ b/ipc/network_ipc-if.c > @@ -24,7 +24,11 @@ struct async_lib { > terminated end; > }; > > -static int handle = 0; > +static enum async_thread_state { > + ASYNC_THREAD_INIT, > + ASYNC_THREAD_RUNNING, > + ASYNC_THREAD_DONE > +} running = ASYNC_THREAD_INIT; > > static struct async_lib request; > > @@ -83,7 +87,7 @@ static void *swupdate_async_thread(void *data) > } > > out: > - handle = 0; > + running = ASYNC_THREAD_DONE; > if (rq->end) > rq->end((RECOVERY_STATUS)swupdate_result); > > @@ -95,20 +99,21 @@ static void *swupdate_async_thread(void *data) > * to let build the ipc library without > * linking pctl code > */ > -static pthread_t start_ipc_thread(void *(* start_routine) (void *), void *arg) > +static void start_ipc_thread(void *(* start_routine) (void *), void *arg) > { > int ret; > - pthread_t id; > pthread_attr_t attr; > > pthread_attr_init(&attr); > pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); > > - ret = pthread_create(&id, &attr, start_routine, arg); > + ret = pthread_create(&async_thread_id, &attr, start_routine, arg); > if (ret) { > - return -1; > + perror("ipc thread creation failed"); > + return; > } > - return id; > + > + running = ASYNC_THREAD_RUNNING; > } > > /* > @@ -121,8 +126,16 @@ int swupdate_async_start(writedata wr_func, getstatus status_func, > struct async_lib *rq; > int connfd; > > - if (handle) > + switch (running) { > + case ASYNC_THREAD_INIT: > + break; > + case ASYNC_THREAD_DONE: > + pthread_join(async_thread_id, NULL); > + running = ASYNC_THREAD_INIT; > + break; > + default: > return -EBUSY; > + } > > rq = get_request(); > > @@ -137,11 +150,9 @@ int swupdate_async_start(writedata wr_func, getstatus status_func, > > rq->connfd = connfd; > > - async_thread_id = start_ipc_thread(swupdate_async_thread, rq); > + start_ipc_thread(swupdate_async_thread, rq); > > - handle++; > - > - return handle; > + return running != ASYNC_THREAD_INIT; > } > > int swupdate_image_write(char *buf, int size) Reviewed-by: Stefano babic <sbabic@denx.de> Best regards, Stefano Babic
On 23.04.22 01:59, Dominique Martinet wrote: > async_thread_id was set but never used, > start_ipc_thread returning -1 which isn't a valid pthread_t, > and handle used as a bool with a weird ++ pattern: > clean this all up by replacing handle with a proper state enum > (init, started, done), and join when we try to start thread again > after it's been done successfully > We still never join the thread when exiting after it's done once, > nor do we care about its pthread_exit return value, but at least > we don't leak resources everytime a new thread is started > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > > This is mostly a cosmetic change, as I saw a warning for the return -1 > on musl where pthread_t isn't an int. > It doesn't solve any hard problem for me. > > > ipc/network_ipc-if.c | 35 +++++++++++++++++++++++------------ > 1 file changed, 23 insertions(+), 12 deletions(-) > > diff --git a/ipc/network_ipc-if.c b/ipc/network_ipc-if.c > index bf946648edcf..ed0993929834 100644 > --- a/ipc/network_ipc-if.c > +++ b/ipc/network_ipc-if.c > @@ -24,7 +24,11 @@ struct async_lib { > terminated end; > }; > > -static int handle = 0; > +static enum async_thread_state { > + ASYNC_THREAD_INIT, > + ASYNC_THREAD_RUNNING, > + ASYNC_THREAD_DONE > +} running = ASYNC_THREAD_INIT; > > static struct async_lib request; > > @@ -83,7 +87,7 @@ static void *swupdate_async_thread(void *data) > } > > out: > - handle = 0; > + running = ASYNC_THREAD_DONE; > if (rq->end) > rq->end((RECOVERY_STATUS)swupdate_result); > > @@ -95,20 +99,21 @@ static void *swupdate_async_thread(void *data) > * to let build the ipc library without > * linking pctl code > */ > -static pthread_t start_ipc_thread(void *(* start_routine) (void *), void *arg) > +static void start_ipc_thread(void *(* start_routine) (void *), void *arg) > { > int ret; > - pthread_t id; > pthread_attr_t attr; > > pthread_attr_init(&attr); > pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); > > - ret = pthread_create(&id, &attr, start_routine, arg); > + ret = pthread_create(&async_thread_id, &attr, start_routine, arg); > if (ret) { > - return -1; > + perror("ipc thread creation failed"); > + return; > } > - return id; > + > + running = ASYNC_THREAD_RUNNING; > } > > /* > @@ -121,8 +126,16 @@ int swupdate_async_start(writedata wr_func, getstatus status_func, > struct async_lib *rq; > int connfd; > > - if (handle) > + switch (running) { > + case ASYNC_THREAD_INIT: > + break; > + case ASYNC_THREAD_DONE: > + pthread_join(async_thread_id, NULL); > + running = ASYNC_THREAD_INIT; > + break; > + default: > return -EBUSY; > + } > > rq = get_request(); > > @@ -137,11 +150,9 @@ int swupdate_async_start(writedata wr_func, getstatus status_func, > > rq->connfd = connfd; > > - async_thread_id = start_ipc_thread(swupdate_async_thread, rq); > + start_ipc_thread(swupdate_async_thread, rq); > > - handle++; > - > - return handle; > + return running != ASYNC_THREAD_INIT; > } > > int swupdate_image_write(char *buf, int size) Applied to -master, thanks ! Best regards, Stefano Babic
diff --git a/ipc/network_ipc-if.c b/ipc/network_ipc-if.c index bf946648edcf..ed0993929834 100644 --- a/ipc/network_ipc-if.c +++ b/ipc/network_ipc-if.c @@ -24,7 +24,11 @@ struct async_lib { terminated end; }; -static int handle = 0; +static enum async_thread_state { + ASYNC_THREAD_INIT, + ASYNC_THREAD_RUNNING, + ASYNC_THREAD_DONE +} running = ASYNC_THREAD_INIT; static struct async_lib request; @@ -83,7 +87,7 @@ static void *swupdate_async_thread(void *data) } out: - handle = 0; + running = ASYNC_THREAD_DONE; if (rq->end) rq->end((RECOVERY_STATUS)swupdate_result); @@ -95,20 +99,21 @@ static void *swupdate_async_thread(void *data) * to let build the ipc library without * linking pctl code */ -static pthread_t start_ipc_thread(void *(* start_routine) (void *), void *arg) +static void start_ipc_thread(void *(* start_routine) (void *), void *arg) { int ret; - pthread_t id; pthread_attr_t attr; pthread_attr_init(&attr); pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); - ret = pthread_create(&id, &attr, start_routine, arg); + ret = pthread_create(&async_thread_id, &attr, start_routine, arg); if (ret) { - return -1; + perror("ipc thread creation failed"); + return; } - return id; + + running = ASYNC_THREAD_RUNNING; } /* @@ -121,8 +126,16 @@ int swupdate_async_start(writedata wr_func, getstatus status_func, struct async_lib *rq; int connfd; - if (handle) + switch (running) { + case ASYNC_THREAD_INIT: + break; + case ASYNC_THREAD_DONE: + pthread_join(async_thread_id, NULL); + running = ASYNC_THREAD_INIT; + break; + default: return -EBUSY; + } rq = get_request(); @@ -137,11 +150,9 @@ int swupdate_async_start(writedata wr_func, getstatus status_func, rq->connfd = connfd; - async_thread_id = start_ipc_thread(swupdate_async_thread, rq); + start_ipc_thread(swupdate_async_thread, rq); - handle++; - - return handle; + return running != ASYNC_THREAD_INIT; } int swupdate_image_write(char *buf, int size)
async_thread_id was set but never used, start_ipc_thread returning -1 which isn't a valid pthread_t, and handle used as a bool with a weird ++ pattern: clean this all up by replacing handle with a proper state enum (init, started, done), and join when we try to start thread again after it's been done successfully We still never join the thread when exiting after it's done once, nor do we care about its pthread_exit return value, but at least we don't leak resources everytime a new thread is started Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- This is mostly a cosmetic change, as I saw a warning for the return -1 on musl where pthread_t isn't an int. It doesn't solve any hard problem for me. ipc/network_ipc-if.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)