On Thu, Jul 19, 2018 at 04:52 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
>> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
