On Wed, Jan 10, 2018 at 12:23:28PM -0500, John Ferlan wrote: ^This hunk would suffice. ^This is a NOP, since all the sockets have been closed already (in the loop which precedes the call) and the IO callback handle removed with watch reset to -1. Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;

Split up virThreadPoolFree to create a Drain function which will be called from virNetServerClose in order to ensure the various worker threads are removed during the close rather than waiting for the dispose function. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/rpc/virnetserver.c | 2 ++ src/util/virthreadpool.c | 19 +++++++++++++------ src/util/virthreadpool.h | 2 ++ 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a705fa846..f1e31ffcb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2864,6 +2864,7 @@ virThreadJobSetWorker; # util/virthreadpool.h +virThreadPoolDrain; virThreadPoolFree; virThreadPoolGetCurrentWorkers; virThreadPoolGetFreeWorkers; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 7bab11efb..1ae98c244 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -835,6 +835,8 @@ void virNetServerClose(virNetServerPtr srv) for (i = 0; i < srv->nservices; i++) virNetServerServiceClose(srv->services[i]); + virThreadPoolDrain(srv->workers); + for (i = 0; i < srv->nclients; i++) virNetServerClientClose(srv->clients[i]); diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index 10f2bd2c3..f4ac88ddc 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -269,7 +269,8 @@ virThreadPoolNewFull(size_t minWorkers, } -void virThreadPoolFree(virThreadPoolPtr pool) +void +virThreadPoolDrain(virThreadPoolPtr pool) { virThreadPoolJobPtr job; bool priority = false; @@ -294,15 +295,21 @@ void virThreadPoolFree(virThreadPoolPtr pool) VIR_FREE(job); } - VIR_FREE(pool->workers); - virMutexUnlock(&pool->mutex); - virMutexDestroy(&pool->mutex); - virCondDestroy(&pool->quit_cond); - virCondDestroy(&pool->cond); if (priority) { VIR_FREE(pool->prioWorkers); virCondDestroy(&pool->prioCond); } + + virMutexUnlock(&pool->mutex); +} + + +void virThreadPoolFree(virThreadPoolPtr pool) +{ + VIR_FREE(pool->workers); + virMutexDestroy(&pool->mutex); + virCondDestroy(&pool->quit_cond); + virCondDestroy(&pool->cond); VIR_FREE(pool); } diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h index e1f362f5b..1b897e1fd 100644 --- a/src/util/virthreadpool.h +++ b/src/util/virthreadpool.h @@ -50,6 +50,8 @@ size_t virThreadPoolGetCurrentWorkers(virThreadPoolPtr pool); size_t virThreadPoolGetFreeWorkers(virThreadPoolPtr pool); size_t virThreadPoolGetJobQueueDepth(virThreadPoolPtr pool); +void virThreadPoolDrain(virThreadPoolPtr pool); + void virThreadPoolFree(virThreadPoolPtr pool); int virThreadPoolSendJob(virThreadPoolPtr pool, -- 2.13.6

From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; This function is called by daemon before shutting down netdaemon threads that serves client requests to make sure all these threads will be able to shutdown. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- daemon/libvirtd.c | 3 +++ src/driver-state.h | 4 ++++ src/libvirt.c | 18 ++++++++++++++++++ src/libvirt_internal.h | 1 + src/libvirt_private.syms | 1 + 5 files changed, 27 insertions(+) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index b47f875d9..37d66b3e9 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1529,6 +1529,9 @@ int main(int argc, char **argv) { cleanup: /* Keep cleanup order in inverse order of startup */ + if (driversInitialized) + virStateShutdown(); + virNetDaemonClose(dmn); virNetlinkEventServiceStopAll(); diff --git a/src/driver-state.h b/src/driver-state.h index 1cb3e4faf..ea549a7db 100644 --- a/src/driver-state.h +++ b/src/driver-state.h @@ -42,6 +42,9 @@ typedef int typedef int (*virDrvStateStop)(void); +typedef void +(*virDrvStateShutdown)(void); + typedef struct _virStateDriver virStateDriver; typedef virStateDriver *virStateDriverPtr; @@ -52,6 +55,7 @@ struct _virStateDriver { virDrvStateCleanup stateCleanup; virDrvStateReload stateReload; virDrvStateStop stateStop; + virDrvStateShutdown stateShutdown; }; diff --git a/src/libvirt.c b/src/libvirt.c index 536d56f0a..330c5ce8c 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -812,6 +812,24 @@ virStateCleanup(void) /** + * virStateShutdown: + * + * Run each virtualization driver's shutdown method. + * + */ +void +virStateShutdown(void) +{ + int r; + + for (r = virStateDriverTabCount - 1; r >= 0; r--) { + if (virStateDriverTab[r]->stateShutdown) + virStateDriverTab[r]->stateShutdown(); + } +} + + +/** * virStateReload: * * Run each virtualization driver's reload method. diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index 62f490a7d..9863b0781 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -36,6 +36,7 @@ int virStateInitialize(bool privileged, int virStateCleanup(void); int virStateReload(void); int virStateStop(void); +void virStateShutdown(void); /* Feature detection. This is a libvirt-private interface for determining * what features are supported by the driver. diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f1e31ffcb..48223f8ed 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1211,6 +1211,7 @@ virSetSharedStorageDriver; virStateCleanup; virStateInitialize; virStateReload; +virStateShutdown; virStateStop; virStreamInData; -- 2.13.6

On Wed, Jan 10, 2018 at 12:23:31PM -0500, John Ferlan wrote: I'm curious why we need this separate .stateShutdown hook. It feels like this code could just be placed at the start of qemuStateStop and achieve the same result. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; Normally if first agent sync is failed we retry. First sync can also be failed due to agent was closed. In this case we should fail sync otherwise second attempt will hang. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_agent.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 5d125c413..e1440ec27 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -987,17 +987,17 @@ qemuAgentGuestSync(qemuAgentPtr mon) goto cleanup; if (!sync_msg.rxObject) { - if (sync_msg.first) { + if (!mon->running) { + virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", + _("Guest agent disappeared while executing command")); + goto cleanup; + } else if (sync_msg.first) { VIR_FREE(sync_msg.txBuffer); memset(&sync_msg, 0, sizeof(sync_msg)); goto retry; } else { - if (mon->running) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing monitor reply object")); - else - virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", - _("Guest agent disappeared while executing command")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing monitor reply object")); goto cleanup; } } -- 2.13.6

Modify GetAllStats to generate a long enough pause in order to send a SIGTERM to libvirtd while a client connection is processing. In order to "set things up": 1. In one terminal window from a local branch built using these patches using a root account run libvirtd debug, e.g.: # ./run gdb daemon/libvirtd once running, type a 'c' (e.g. continue) and <return> 2. Start a domain (or have one running with the current libvirtd) virsh start $domain 3. Prepare a domstats command for that domain (but don't yet hit <return> in order run it): virsh domstats $domain 4. Prepare a kill command for the running libvirtd, e.g.: jferlan 4143 4107 0 09:51 pts/1 00:00:00 vim +1396 daemon/libvirtd.c root 30054 21195 6 11:17 pts/8 00:00:01 gdb /home/jferlan/git/libvirt.work/daemon/.libs/lt-libvirtd root 30087 30054 7 11:17 pts/8 00:00:01 /home/jferlan/git/libvirt.work/daemon/.libs/lt-libvirtd root 30385 19861 0 11:17 pts/17 00:00:00 grep --color=auto libvirtd but again don't hit <return> yet. 5. Align your stars perfectly now... a. Hit <return> on your domstats command b. Swap to the kill command window and hit <return> This should cause the libvirtd debug window to stop, but since you already typed 'c' it'll continue at least briefly, for example: ... [Thread 0x7fffc3231700 (LWP 30374) exited] Detaching after fork from child process 30376. Detaching after fork from child process 30377. Detaching after fork from child process 30378. [Thread 0x7fffc4445700 (LWP 30106) exited] c 2018-01-10 16:18:12.962+0000: 30094: info : libvirt version: 4.0.0 2018-01-10 16:18:12.962+0000: 30094: info : hostname: unknown4ceb42c824f4.attlocal.net 2018-01-10 16:18:12.962+0000: 30094: warning : qemuConnectGetAllDomainStats:20265 : k = -5340232226128654848 Thread 1 "lt-libvirtd" received signal SIGTERM, Terminated. 0x00007ffff3ae6d2d in poll () from /lib64/libc.so.6 ... (gdb) c Continuing. [Thread 0x7fffc5c48700 (LWP 30103) exited] [Thread 0x7fffc5447700 (LWP 30104) exited] [Thread 0x7fffc4c46700 (LWP 30105) exited] [Thread 0x7fffc6449700 (LWP 30102) exited] [Thread 0x7fffc6c4a700 (LWP 30101) exited] [Thread 0x7fffe3b57700 (LWP 30097) exited] [Thread 0x7fffe4358700 (LWP 30096) exited] [Thread 0x7fffe2354700 (LWP 30100) exited] [Thread 0x7fffe3356700 (LWP 30098) exited] [Thread 0x7fffe2b55700 (LWP 30099) exited] [Thread 0x7fffe535a700 (LWP 30094) exited] [Thread 0x7fffe5b5b700 (LWP 30093) exited] [Thread 0x7fffe635c700 (LWP 30092) exited] [Thread 0x7fffe6b5d700 (LWP 30091) exited] 2018-01-10 16:18:25.451+0000: 30095: warning : qemuConnectGetAllDomainStats:20265 : k = -5340232226128654848 [Thread 0x7fffe4b59700 (LWP 30095) exited] [Thread 0x7fffc3a32700 (LWP 30187) exited] [Inferior 1 (process 30087) exited normally] (gdb) c The program is not being run. (gdb) quit The virsh domstats window will "close" as follows: error: Disconnected from qemu:///system due to end of file error: End of file while reading data: Input/output error If something's wrong, then the libvirtd window may not exit those final two threads in which case you could interrupt it (^c) and check the threads (thread apply all bt) which will probably show some sort of hang... My testing shows that the hang no longer occurs with all the previous patches applied. The subsequent patch calling virHashRemoveAll from virNetDaemonClose does not seem to be necessary, although I suppose it cannot hurt as the same essential functionality occurs during the Dispose function Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1de236cb5..0d0b03d86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20222,6 +20222,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, bool enforce = !!(flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS); int nstats = 0; size_t i; + size_t j, k = 0; int ret = -1; unsigned int privflags = 0; unsigned int domflags = 0; @@ -20259,6 +20260,10 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, if (qemuDomainGetStatsNeedMonitor(stats)) privflags |= QEMU_DOMAIN_STATS_HAVE_JOB; + for (j = 0; j < 10000000000; j++) // Add one more zero for longer... + k = j + k; + VIR_WARN("k = %zd", k); + for (i = 0; i < nvms; i++) { virDomainStatsRecordPtr tmp = NULL; domflags = 0; -- 2.13.6