On Thu, Jul 19, 2018 at 04:52 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- 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.
Is the code still safe if the worker count changes after getting the worker count? If yes, then I can split the patch if you want to.
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);
The reason is/was that my original code assumed it’s allowed to switch between zero and non-zero worker counts. My intention was to hold the lock for srv->workers (and therefore forbid any changes on the worker count) as long as needed.
/* 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.
Yes, I don’t like it either.
John
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ffe5dfd19b11..aa496ddf8012 100644
[…snip] -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294