Re: [PATCH 2/3] qemu: Set "qemu-event" thread identity
On 10/26/21 4:31 PM, Daniel P. Berrangé wrote: > On Tue, Oct 26, 2021 at 12:17:07PM +0200, Michal Privoznik wrote: >> It may happen that qemuProcessStop() is called from "qemu-event" >> thread. But this thread doesn't have any virIdentity set >> (virIdentity being thread local) and therefore it may be unable >> to open connection to secondary drivers. It is unable to do so >> in split daemon scenario, because in there opening a connection >> is coupled with copying current thread identity onto the >> connection. Code-wise, virIdentityGetCurrent() returns NULL which >> in turn makes virGetConnectGeneric() fail. This problem does not >> occur in monolithic daemon scenario, because no identity copying >> is done there. >> >> Long story short, inability to open secondary driver connection >> can lead to unwanted results. Therefore, do what >> qemuProcessReconnectHelper() does - set the new thread identity >> to be the one of the caller. >> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2013573 >> Signed-off-by: Michal Privoznik >> --- >> src/qemu/qemu_driver.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index a8bf0ecc6f..70b5f37e6b 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -571,6 +571,7 @@ qemuStateInitialize(bool privileged, >> size_t i; >> const char *defsecmodel = NULL; >> g_autofree virSecurityManager **sec_managers = NULL; >> +g_autoptr(virIdentity) identity = virIdentityGetCurrent(); > > Note, qemuStateInitialize will run with the system identity, so this > is functionally the same as the next patch. Yes, but for some reason that patch looks like a hack to me. But I can polish it and we can go with that one instead. Michal
Re: [PATCH 2/3] qemu: Set "qemu-event" thread identity
On Tue, Oct 26, 2021 at 12:17:07PM +0200, Michal Privoznik wrote: > It may happen that qemuProcessStop() is called from "qemu-event" > thread. But this thread doesn't have any virIdentity set > (virIdentity being thread local) and therefore it may be unable > to open connection to secondary drivers. It is unable to do so > in split daemon scenario, because in there opening a connection > is coupled with copying current thread identity onto the > connection. Code-wise, virIdentityGetCurrent() returns NULL which > in turn makes virGetConnectGeneric() fail. This problem does not > occur in monolithic daemon scenario, because no identity copying > is done there. > > Long story short, inability to open secondary driver connection > can lead to unwanted results. Therefore, do what > qemuProcessReconnectHelper() does - set the new thread identity > to be the one of the caller. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2013573 > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_driver.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index a8bf0ecc6f..70b5f37e6b 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -571,6 +571,7 @@ qemuStateInitialize(bool privileged, > size_t i; > const char *defsecmodel = NULL; > g_autofree virSecurityManager **sec_managers = NULL; > +g_autoptr(virIdentity) identity = virIdentityGetCurrent(); Note, qemuStateInitialize will run with the system identity, so this is functionally the same as the next patch. > > qemu_driver = g_new0(virQEMUDriver, 1); > > @@ -915,7 +916,7 @@ qemuStateInitialize(bool privileged, > * events that will be dispatched to the worker pool */ > qemu_driver->workerPool = virThreadPoolNewFull(0, 1, 0, > qemuProcessEventHandler, > "qemu-event", > - NULL, > + identity, > qemu_driver); > if (!qemu_driver->workerPool) > goto error; Reviewed-by: Daniel P. Berrangé 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 :|
[PATCH 2/3] qemu: Set "qemu-event" thread identity
It may happen that qemuProcessStop() is called from "qemu-event" thread. But this thread doesn't have any virIdentity set (virIdentity being thread local) and therefore it may be unable to open connection to secondary drivers. It is unable to do so in split daemon scenario, because in there opening a connection is coupled with copying current thread identity onto the connection. Code-wise, virIdentityGetCurrent() returns NULL which in turn makes virGetConnectGeneric() fail. This problem does not occur in monolithic daemon scenario, because no identity copying is done there. Long story short, inability to open secondary driver connection can lead to unwanted results. Therefore, do what qemuProcessReconnectHelper() does - set the new thread identity to be the one of the caller. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2013573 Signed-off-by: Michal Privoznik --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a8bf0ecc6f..70b5f37e6b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -571,6 +571,7 @@ qemuStateInitialize(bool privileged, size_t i; const char *defsecmodel = NULL; g_autofree virSecurityManager **sec_managers = NULL; +g_autoptr(virIdentity) identity = virIdentityGetCurrent(); qemu_driver = g_new0(virQEMUDriver, 1); @@ -915,7 +916,7 @@ qemuStateInitialize(bool privileged, * events that will be dispatched to the worker pool */ qemu_driver->workerPool = virThreadPoolNewFull(0, 1, 0, qemuProcessEventHandler, "qemu-event", - NULL, + identity, qemu_driver); if (!qemu_driver->workerPool) goto error; -- 2.32.0
