5:49 a.m.
*** BLURB HERE ***
Michal PrÃvoznÃk (4):
qemu_shim: Don't hang if failed to start domain
docs: Lift restriction on running API from the event loop thread
virConnectOpen: Require root dir to be absolute path
qemu_shim: Always pre-create root dir
docs/drvqemu.html.in | 8 ++++++--
src/libvirt.c | 6 ++++++
src/qemu/qemu_shim.c | 20 ++++++++++++++------
3 files changed, 26 insertions(+), 8 deletions(-)
--
2.26.2
5:49 a.m.
New subject: [PATCH 1/4] qemu_shim: Don't hang if failed to start domain
The qemu shim spawns a separate thread in which the event loop is
ran. The virEventRunDefaultImpl() call is wrapped in a while()
loop, just like it should. There are few lines of code around
which try to ensure that domain is destroyed (when quitting) and
that the last round of event loop is ran after the
virDomainDestroy() call. Only after that the loop is quit from
and the thread quits. Well, if creating the domain failed then
the thread is stuck inside event loop forever.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1920337
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_shim.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_shim.c b/src/qemu/qemu_shim.c
index c10598df4b..21a24abade 100644
--- a/src/qemu/qemu_shim.c
+++ b/src/qemu/qemu_shim.c
@@ -45,9 +45,12 @@ qemuShimEventLoop(void *opaque G_GNUC_UNUSED)
while (!quit) {
g_mutex_lock(&eventLock);
if (eventQuitFlag && !eventPreventQuitFlag) {
+ quit = true;
if (dom) {
virDomainDestroy(dom);
- quit = true;
+ } else {
+ g_mutex_unlock(&eventLock);
+ break;
}
}
g_mutex_unlock(&eventLock);
--
2.26.2
4:42 a.m.
New subject: [PATCH 1/4] qemu_shim: Don't hang if failed to start domain
On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
+++ b/src/qemu/qemu_shim.c
@@ -45,9 +45,12 @@ qemuShimEventLoop(void *opaque G_GNUC_UNUSED)
while (!quit) {
g_mutex_lock(&eventLock);
if (eventQuitFlag && !eventPreventQuitFlag) {
+ quit = true;
if (dom) {
virDomainDestroy(dom);
- quit = true;
+ } else {
+ g_mutex_unlock(&eventLock);
+ break;
}
}
g_mutex_unlock(&eventLock);
I'm probably missing something obvious, but I thought this could be
simply
while (!quit) {
g_mutex_lock(&eventLock);
if (eventQuitFlag && !eventPreventQuitFlag) {
quit = true;
if (dom) {
virDomainDestroy(dom);
}
}
g_mutex_unlock(&eventLock);
virEventRunDefaultImpl();
}
Do we specifically want to avoid call virEventRunDefaultImpl() one
last time if the domain has failed to start?
--
Andrea Bolognani / Red Hat / Virtualization
8:24 a.m.
New subject: [PATCH 1/4] qemu_shim: Don't hang if failed to start domain
On 3/12/21 11:42 AM, Andrea Bolognani wrote:
On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
> +++ b/src/qemu/qemu_shim.c
> @@ -45,9 +45,12 @@ qemuShimEventLoop(void *opaque G_GNUC_UNUSED)
> while (!quit) {
> g_mutex_lock(&eventLock);
> if (eventQuitFlag && !eventPreventQuitFlag) {
> + quit = true;
> if (dom) {
> virDomainDestroy(dom);
> - quit = true;
> + } else {
> + g_mutex_unlock(&eventLock);
> + break;
> }
> }
> g_mutex_unlock(&eventLock);
I'm probably missing something obvious, but I thought this could be
simply
while (!quit) {
g_mutex_lock(&eventLock);
if (eventQuitFlag && !eventPreventQuitFlag) {
quit = true;
if (dom) {
virDomainDestroy(dom);
}
}
g_mutex_unlock(&eventLock);
virEventRunDefaultImpl();
}
Do we specifically want to avoid call virEventRunDefaultImpl() one
last time if the domain has failed to start?
Huh, riddle me this - how come now this is sufficient, but back then
when I was writing this it wasn't?
diff --git i/src/qemu/qemu_shim.c w/src/qemu/qemu_shim.c
index c10598df4b..5a9ffdd416 100644
--- i/src/qemu/qemu_shim.c
+++ w/src/qemu/qemu_shim.c
@@ -45,9 +45,9 @@ qemuShimEventLoop(void *opaque G_GNUC_UNUSED)
while (!quit) {
g_mutex_lock(&eventLock);
if (eventQuitFlag && !eventPreventQuitFlag) {
+ quit = true;
if (dom) {
virDomainDestroy(dom);
- quit = true;
}
}
g_mutex_unlock(&eventLock);
Will post it as v2, soon.
Michal
9:59 a.m.
New subject: [PATCH 1/4] qemu_shim: Don't hang if failed to start domain
On Fri, 2021-03-12 at 15:24 +0100, Michal Privoznik wrote:
On 3/12/21 11:42 AM, Andrea Bolognani wrote:
> On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
> > +++ b/src/qemu/qemu_shim.c
> > @@ -45,9 +45,12 @@ qemuShimEventLoop(void *opaque G_GNUC_UNUSED)
> > while (!quit) {
> > g_mutex_lock(&eventLock);
> > if (eventQuitFlag && !eventPreventQuitFlag) {
> > + quit = true;
> > if (dom) {
> > virDomainDestroy(dom);
> > - quit = true;
> > + } else {
> > + g_mutex_unlock(&eventLock);
> > + break;
> > }
> > }
> > g_mutex_unlock(&eventLock);
>
> I'm probably missing something obvious, but I thought this could be
> simply
>
> while (!quit) {
> g_mutex_lock(&eventLock);
> if (eventQuitFlag && !eventPreventQuitFlag) {
> quit = true;
> if (dom) {
> virDomainDestroy(dom);
> }
> }
> g_mutex_unlock(&eventLock);
> virEventRunDefaultImpl();
> }
>
> Do we specifically want to avoid call virEventRunDefaultImpl() one
> last time if the domain has failed to start?
Huh, riddle me this - how come now this is sufficient, but back then
when I was writing this it wasn't?
diff --git i/src/qemu/qemu_shim.c w/src/qemu/qemu_shim.c
index c10598df4b..5a9ffdd416 100644
--- i/src/qemu/qemu_shim.c
+++ w/src/qemu/qemu_shim.c
@@ -45,9 +45,9 @@ qemuShimEventLoop(void *opaque G_GNUC_UNUSED)
while (!quit) {
g_mutex_lock(&eventLock);
if (eventQuitFlag && !eventPreventQuitFlag) {
+ quit = true;
if (dom) {
virDomainDestroy(dom);
- quit = true;
}
}
g_mutex_unlock(&eventLock);
Will post it as v2, soon.
I see v2 it in the archives[1], but it doesn't seem to have any
intention of reaching my inbox (is libvir-list broken again?) so I'll
just comment here.
Assuming you have verified that the rewritten code still addresses
the bug,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
to v2.
[1] https://listman.redhat.com/archives/libvir-list/2021-March/msg00607.html
--
Andrea Bolognani / Red Hat / Virtualization
5:49 a.m.
New subject: [PATCH 2/4] docs: Lift restriction on running API from the event loop thread
Since v6.2.0-rc1~238 (and friends) QMP processing was moved to a
per-domain thread. Therefore, it is now safe to call APIs from
the event loop thread (e.g. just like qemu shim is doing in
qemuShimEventLoop(). However, it is still important to let the
event loop run after each API call (obviously).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/drvqemu.html.in | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in
index 31d3fee213..3cdd04aa1e 100644
--- a/docs/drvqemu.html.in
+++ b/docs/drvqemu.html.in
@@ -158,8 +158,10 @@ qemu+ssh://root@example.com/system (remote access, SSH tunnelled)
in mind, applications must <strong>NEVER</strong> invoke API
calls from the event loop thread itself, only other threads.
Not following this rule will lead to deadlocks in the API.
- This restriction is intended to be lifted in a future release
- of libvirt, once QMP processing moves to a dedicated thread.
+ This restriction was lifted starting from 6.2.0 release, when
+ QMP processing moved to a dedicated thread. However, it is
+ important to let the event loop run after each API call, even
+ the ones made from the vent loop thread itself.
</p>
<h2><a id="security">Driver security
architecture</a></h2>
--
2.26.2
7:51 a.m.
New subject: [PATCH 2/4] docs: Lift restriction on running API from the event loop thread
On 3/1/21 5:49 AM, Michal Privoznik wrote:
Since v6.2.0-rc1~238 (and friends) QMP processing was moved to a
per-domain thread. Therefore, it is now safe to call APIs from
the event loop thread (e.g. just like qemu shim is doing in
qemuShimEventLoop(). However, it is still important to let the
event loop run after each API call (obviously).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/drvqemu.html.in | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in
index 31d3fee213..3cdd04aa1e 100644
--- a/docs/drvqemu.html.in
+++ b/docs/drvqemu.html.in
@@ -158,8 +158,10 @@ qemu+ssh://root@example.com/system (remote access, SSH tunnelled)
in mind, applications must <strong>NEVER</strong> invoke API
calls from the event loop thread itself, only other threads.
Not following this rule will lead to deadlocks in the API.
- This restriction is intended to be lifted in a future release
- of libvirt, once QMP processing moves to a dedicated thread.
+ This restriction was lifted starting from 6.2.0 release, when
+ QMP processing moved to a dedicated thread. However, it is
+ important to let the event loop run after each API call, even
+ the ones made from the vent loop thread itself.
event loop
</p>
<h2><a id="security">Driver security
architecture</a></h2>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
4:43 a.m.
New subject: [PATCH 2/4] docs: Lift restriction on running API from the event loop thread
On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
+++ b/docs/drvqemu.html.in
@@ -158,8 +158,10 @@ qemu+ssh://root@example.com/system (remote access, SSH tunnelled)
in mind, applications must <strong>NEVER</strong> invoke API
calls from the event loop thread itself, only other threads.
Not following this rule will lead to deadlocks in the API.
- This restriction is intended to be lifted in a future release
- of libvirt, once QMP processing moves to a dedicated thread.
+ This restriction was lifted starting from 6.2.0 release, when
+ QMP processing moved to a dedicated thread. However, it is
+ important to let the event loop run after each API call, even
+ the ones made from the vent loop thread itself.
s/ vent/ event/g
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
5:49 a.m.
New subject: [PATCH 3/4] virConnectOpen: Require root dir to be absolute path
In theory, users might want to use a relative path as a root
directory for embed drivers. But in practice, nothing in driver
initialization (specifically QEMU driver since it's the only one
that supports embedding now), is prepared for that. Document and
enforce absolute paths.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1883725
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/drvqemu.html.in | 2 ++
src/libvirt.c | 6 ++++++
2 files changed, 8 insertions(+)
diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in
index 3cdd04aa1e..494dda56ef 100644
--- a/docs/drvqemu.html.in
+++ b/docs/drvqemu.html.in
@@ -73,6 +73,8 @@ qemu+ssh://root@example.com/system (remote access, SSH tunnelled)
registered & be running an instance of the event loop. To open
the driver in embedded mode the app use the new URI path and specify
a virtual root directory under which the driver will create content.
+ The path to the root directory must be absolute. Passing a relative
+ path results in an error.
</p>
<pre>
diff --git a/src/libvirt.c b/src/libvirt.c
index 5778b5daee..f2f0efa0cb 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -996,6 +996,12 @@ virConnectOpenInternal(const char *name,
if (!root)
goto failed;
+ if (!g_path_is_absolute(root)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("root path must be absolute"));
+ goto failed;
+ }
+
if (virEventRequireImpl() < 0)
goto failed;
--
2.26.2
4:44 a.m.
New subject: [PATCH 3/4] virConnectOpen: Require root dir to be absolute path
On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
In theory, users might want to use a relative path as a root
directory for embed drivers. But in practice, nothing in driver
initialization (specifically QEMU driver since it's the only one
that supports embedding now), is prepared for that. Document and
enforce absolute paths.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1883725
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/drvqemu.html.in | 2 ++
src/libvirt.c | 6 ++++++
2 files changed, 8 insertions(+)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
5:49 a.m.
New subject: [PATCH 4/4] qemu_shim: Always pre-create root dir
This problem is reproducible only with secret driver. When
starting a domain via virt-qemu-run and both secret and
(nonexistent) root directory specified this is what happens:
1) virt-qemu-run opens "secret:///embed?root=$rootdir"
connection, which results in the secret driver initialization
(done in secretStateInitialize()). During this process, the
driver creates it's own configDir (derived from $rootdir)
including those parents which don't exists yet. This is all
done with the mode S_IRWXU and thus results in the $rootdir
being created with very restrictive mode (specifically, +x is
missing for group and others).
2) now, virt-qemu-run-opens "qemu:///embed?root=$rootdir" and
calls virDomainCreateXML(). This results in the master-key.aes
being written somewhere under the $rootdir and telling qemu
where to find it.
But because the secret driver created $rootdir with too
restrictive mode, qemu can't access the file (even though it
knows the full path) and fails to start.
It looks like the best solution is to pre-create the root
directory before opening any connection (letting any driver
initialize itself) and set its mode to something less
restrictive.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1859873
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_shim.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_shim.c b/src/qemu/qemu_shim.c
index 21a24abade..a08bdcac6a 100644
--- a/src/qemu/qemu_shim.c
+++ b/src/qemu/qemu_shim.c
@@ -213,11 +213,16 @@ int main(int argc, char **argv)
}
tmproot = true;
- if (chmod(root, 0755) < 0) {
- g_printerr("%s: cannot chown temporary dir: %s\n",
- argv[0], g_strerror(errno));
- goto cleanup;
- }
+ } else if (g_mkdir_with_parents(root, 0755) < 0) {
+ g_printerr("%s: cannot create dir: %s\n",
+ argv[0], g_strerror(errno));
+ goto cleanup;
+ }
+
+ if (chmod(root, 0755) < 0) {
+ g_printerr("%s: cannot chmod temporary dir: %s\n",
+ argv[0], g_strerror(errno));
+ goto cleanup;
}
escaped = g_uri_escape_string(root, NULL, true);
--
2.26.2
4:51 a.m.
New subject: [PATCH 4/4] qemu_shim: Always pre-create root dir
On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
This problem is reproducible only with secret driver. When
starting a domain via virt-qemu-run and both secret and
(nonexistent) root directory specified this is what happens:
1) virt-qemu-run opens "secret:///embed?root=$rootdir"
connection, which results in the secret driver initialization
(done in secretStateInitialize()). During this process, the
driver creates it's own configDir (derived from $rootdir)
s/it's own/its own/
including those parents which don't exists yet. This is all
done with the mode S_IRWXU and thus results in the $rootdir
being created with very restrictive mode (specifically, +x is
missing for group and others).
2) now, virt-qemu-run-opens "qemu:///embed?root=$rootdir" and
s/run-opens/run opens/
+++ b/src/qemu/qemu_shim.c
@@ -213,11 +213,16 @@ int main(int argc, char **argv)
}
tmproot = true;
- if (chmod(root, 0755) < 0) {
- g_printerr("%s: cannot chown temporary dir: %s\n",
- argv[0], g_strerror(errno));
- goto cleanup;
- }
+ } else if (g_mkdir_with_parents(root, 0755) < 0) {
+ g_printerr("%s: cannot create dir: %s\n",
+ argv[0], g_strerror(errno));
+ goto cleanup;
+ }
+
+ if (chmod(root, 0755) < 0) {
+ g_printerr("%s: cannot chmod temporary dir: %s\n",
+ argv[0], g_strerror(errno));
+ goto cleanup;
}
Wouldn't it make sense to leave the chmod() bit where it was?
g_mkdir_with_parents() already accepts the mode as a parameter, so
calling chmod() again seems unnecessary.
With that changed and the commit message fixed,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:38 a.m.
New subject: [PATCH 4/4] qemu_shim: Always pre-create root dir
On 3/12/21 11:51 AM, Andrea Bolognani wrote:
On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
> This problem is reproducible only with secret driver. When
> starting a domain via virt-qemu-run and both secret and
> (nonexistent) root directory specified this is what happens:
>
> 1) virt-qemu-run opens "secret:///embed?root=$rootdir"
> connection, which results in the secret driver initialization
> (done in secretStateInitialize()). During this process, the
> driver creates it's own configDir (derived from $rootdir)
s/it's own/its own/
> including those parents which don't exists yet. This is all
> done with the mode S_IRWXU and thus results in the $rootdir
> being created with very restrictive mode (specifically, +x is
> missing for group and others).
>
> 2) now, virt-qemu-run-opens "qemu:///embed?root=$rootdir" and
s/run-opens/run opens/
> +++ b/src/qemu/qemu_shim.c
> @@ -213,11 +213,16 @@ int main(int argc, char **argv)
> }
> tmproot = true;
>
> - if (chmod(root, 0755) < 0) {
> - g_printerr("%s: cannot chown temporary dir: %s\n",
> - argv[0], g_strerror(errno));
> - goto cleanup;
> - }
> + } else if (g_mkdir_with_parents(root, 0755) < 0) {
> + g_printerr("%s: cannot create dir: %s\n",
> + argv[0], g_strerror(errno));
> + goto cleanup;
> + }
> +
> + if (chmod(root, 0755) < 0) {
> + g_printerr("%s: cannot chmod temporary dir: %s\n",
> + argv[0], g_strerror(errno));
> + goto cleanup;
> }
Wouldn't it make sense to leave the chmod() bit where it was?
g_mkdir_with_parents() already accepts the mode as a parameter, so
calling chmod() again seems unnecessary.
Well, if the dir exists but doesn't have right perms then
g_mkdir_with_parents() does nothing and we need that explicit chmod().
Michal
9:29 a.m.
New subject: [PATCH 4/4] qemu_shim: Always pre-create root dir
On Fri, 2021-03-12 at 15:38 +0100, Michal Privoznik wrote:
On 3/12/21 11:51 AM, Andrea Bolognani wrote:
> On Mon, 2021-03-01 at 12:49 +0100, Michal Privoznik wrote:
> > @@ -213,11 +213,16 @@ int main(int argc, char **argv)
> > }
> > tmproot = true;
> >
> > - if (chmod(root, 0755) < 0) {
> > - g_printerr("%s: cannot chown temporary dir: %s\n",
> > - argv[0], g_strerror(errno));
> > - goto cleanup;
> > - }
> > + } else if (g_mkdir_with_parents(root, 0755) < 0) {
> > + g_printerr("%s: cannot create dir: %s\n",
> > + argv[0], g_strerror(errno));
> > + goto cleanup;
> > + }
> > +
> > + if (chmod(root, 0755) < 0) {
> > + g_printerr("%s: cannot chmod temporary dir: %s\n",
> > + argv[0], g_strerror(errno));
> > + goto cleanup;
> > }
>
> Wouldn't it make sense to leave the chmod() bit where it was?
> g_mkdir_with_parents() already accepts the mode as a parameter, so
> calling chmod() again seems unnecessary.
Well, if the dir exists but doesn't have right perms then
g_mkdir_with_parents() does nothing and we need that explicit chmod().
Fair point, I hadn't considered that :)
--
Andrea Bolognani / Red Hat / Virtualization
1461
days inactive
1472
days old
13 comments
3 participants
participants (3)
-
Andrea Bolognani -
Eric Blake -
Michal Privoznik