Re: [libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well

On 07/03/2018 07:37 AM, Marc Hartmayer wrote: > Semantically, there is no difference between an uninitialized worker > pool and an initialized worker pool with zero workers. Let's allow the > worker pool to be initialized for max_workers=0 as well then which > makes the API more symmetric and simplifies code. Validity of the > worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead. > > This patch fixes segmentation faults in > virNetServerGetThreadPoolParameters and > virNetServerSetThreadPoolParameters for the case when no worker pool > is actually initialized (max_workers=0). > > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com&gt; > --- > src/libvirt_private.syms | 4 +++ > src/rpc/virnetserver.c | 13 ++++----- > src/util/virthreadpool.c | 73 ++++++++++++++++++++++++++++++++++++------------ > src/util/virthreadpool.h | 8 ++++++ > 4 files changed, 73 insertions(+), 25 deletions(-) > So it seems there's multiple things going on in this patch - maybe they're semantically tied and I'm missing it ;-)... The virNetServerNew to not inhibit the call to virThreadPoolNew if max_workers == 0 would seem to be easily separable with the other places that would check if srv->workers == NULL before continuing.

I don't understand why virNetServerDispatchNewMessage needs anything more than if (virThreadPoolGetMaxWorkers(srv->workers) If I think back to the previous patch, then why not: size_t workers = 0; ... if (srv->workers) workers = virThreadPoolGetMaxWorkers(srv->workers);

/* we can unlock @srv since @prog can only become invalid in case * of disposing @srv, but let's grab a ref first to ensure nothing * else disposes of it before we use it. */ virObjectRef(srv); virObjectUnlock(srv); if (workers > 0) { ... In the long run, I don't think it's been the desire to expose so many virthreadpool.c API's - especially the locks.

John > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index ffe5dfd19b11..aa496ddf8012 100644