[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v5 3/3] libvirtd: fix crash on termination

On Fri, Nov 03, 2017 at 11:07:36AM +0300, Nikolay Shirokovskiy wrote:
On 02.11.2017 19:32, Martin Kletzander wrote:
This just makes the window of opportunity (between daemonServerClose()
and the actual removal of the virNetServerPtr from the hash) smaller.
That's why I don't see it as a fix, rather as a workaround.

What I think should be done is the order of what's done with services,
servers, drivers, daemon, workers, etc.

I read the other threds and I think we're on the same note here, it's
just that there is lot of confusion and we're all approaching it

So first let's agree on what should be done when virNetDaemonClose() is
called and the loop in virNetDaemonRun() exits.  My opinion it is:

1) Stop accepting new calls and services:

This not really necessary as this has effect only if loop is running.

Sure, good point.

2) Wait for all the workers to finish:
   First part of virThreadPoolFree()

I was thinking of splitting this function to finish/free too.
After finish it is not really usable because there are no
more threads in pool anymore so we have to introduce state
finished and check it in every thread pool api function to
be safe. So this will introduce a bit of complexity only
for the sake of some scheme IMO.

Yeah, it's similar to the daemon when you are between close and dispose, but for
the daemon we need it.

What function do you think we would need to check this for?  The finish would
end all the threads and since the event loop is not running no new jobs could be

3) Kill off clients:

4) Clean up drivers:

I think it is better for net daemon/servers not to know about
hypervisor drivers directly.

Sure, I was only talking about the libvirtd for the sake of simplicity.

5) Clean up services, servers, daemon, etc.
   including the second part of virThreadPoolFree()

The last thing is what should be done in virNetDaemonDispose(), but
unfortunately we (mis)use it for more than that.  Numbers 1-4 should be,
IMO in virNet{Daemon,Server}Close().

Would this solve your problem?  I mean I'm not against more reference
counting, it should be done properly, but I think the above would do the
trick and also make sense from the naming point of view.

Since both the daemon and server will probably not ever be consistent
after calling the Close function, it might make sense to just include
virNetDaemonClose() into virNetDaemonDispose() and I'm not against that.
But the order of the steps should still be as described above IMO.

This won't work IMO. If we are already in virNetDaemonDispose then
@dmn object is invalid but we don't yet call virStateCleanup yet
(if it is moved to virNetDaemonClose) and this is the problem 85c3a182 dealt with.

It would solve it if order above is followed.  Sure, virNetDaemon should not
know about any drivers, but we can simply register a cleanup callback for the
daemon which the Dispose function will call after the thread pools are cleaned
up (just thought of that now).

Let me know what you think.  And don't forget to have a nice day!


P.S.: The order of clean ups in libvirtd's main() is absolutely terrible and
     unreadable.  Also, even though I guess it goes without saying, if proper
     reference counting is in place, the order of virObjectUnref()s and
     VIR_FREE()s should not matter.  I think it chould be changed like this:

It makes sense that in cleanup section we can first make all cleanup and
then all dispose. It is only not clear to me can we call virStateCleanup
before virNetlinkShutdown. Also I would keep comment for @dmn unref
until proper refcount for @dmn is implemented.

I don't think virStateCleanup() and virNetlinkShutdown() are dependent.  The
comment can stay, I was just trying to show that it's also a mess.  What I would
like, actually, is to have the daemon running with all the references and
pointers (that the main() function doesn't need anymore) freed, but both this
and what I suggested below is unrelated to what we're trying to fix, I don't
know why I started talking about it.

Attachment: signature.asc
Description: Digital signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]