On 3/18/24 14:31, Wei Gong wrote:

when the thread pool is dynamically expanded, threads should not be created from the existing workers; they should be created from the newly expanded workers

Signed-off-by: Wei Gong <gongwei833x@gmail.com> --- src/util/virthreadpool.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index d45fa92061..de8113df81 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -184,7 +184,8 @@ virThreadPoolExpand(virThreadPool *pool, size_t gain, bool priority) size_t i = 0; struct virThreadPoolWorkerData *data = NULL;

- VIR_EXPAND_N(*workers, *curWorkers, gain); + VIR_REALLOC_N(*workers, *curWorkers + gain);

+ memset(&(*workers)[*curWorkers], 0, sizeof(virThread) * gain);

I don't think it's safe to do this ^^^. Firstly, pthread_t is an opaque type. We don't know if all zeroes are valid value (that's why there are functions like pthread_equal()) or if it's a valid init value. Secondly, since *curWorkers is incremented at only after successful thread creation, we can no longer get it to point to an uninitialized array item. I'll drop this line before pushing.

for (i = 0; i < gain; i++) { g_autofree char *name = NULL; @@ -199,7 +200,7 @@ virThreadPoolExpand(virThreadPool *pool, size_t gain, bool priority) else name = g_strdup(pool->jobName);

- if (virThreadCreateFull(&(*workers)[i], + if (virThreadCreateFull(&(*workers)[*curWorkers], false, virThreadPoolWorker, name, @@ -207,15 +208,13 @@ virThreadPoolExpand(virThreadPool *pool, size_t gain, bool priority) data) < 0) { VIR_FREE(data); virReportSystemError(errno, "%s", _("Failed to create thread")); - goto error; + return -1; } + + (*curWorkers)++; }

return 0; - - error: - *curWorkers -= gain - i; - return -1; }

virThreadPool *

Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and merged. Congratulations on your first libvirt contribution! Michal