* [PATCH] qemurunner: Use tempfile.mkstemp to create pidfile name
@ 2020-07-21 21:56 Richard Purdie
2020-07-21 23:49 ` [OE-core] " Joshua Watt
0 siblings, 1 reply; 3+ messages in thread
From: Richard Purdie @ 2020-07-21 21:56 UTC (permalink / raw
To: openembedded-core
We continue to see qemu races where qemu fails to write out a pidfile. The
pidfile name being used was based on the parent process starting the qemus
so in theory, a qemu shutting down could have deleted the file into which
the new qemu's pid was about to be written. This shouldn't happen as qemu
processes should be exitting completely before the code returns.
In the absence of better theory for the failure, use a random pid filename
rather than the parent pid so there is no overlap in the pid filename,
thereby removing any theoretical race.
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
meta/lib/oeqa/utils/qemurunner.py | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
index 519aa9aa1e5..73a3efdc964 100644
--- a/meta/lib/oeqa/utils/qemurunner.py
+++ b/meta/lib/oeqa/utils/qemurunner.py
@@ -20,6 +20,7 @@ import string
import threading
import codecs
import logging
+import tempfile
from oeqa.utils.dump import HostDumper
from collections import defaultdict
@@ -65,7 +66,11 @@ class QemuRunner:
self.runqemutime = 120
if not workdir:
workdir = os.getcwd()
- self.qemu_pidfile = workdir + '/pidfile_' + str(os.getpid())
+ # Use tempfile to obtain a unique name. We own the directory
+ # so closing the file isn't an issue as long as we don't delete it
+ self.qemu_pidfile = tempfile.mkstemp(dir=workdir, prefix="qemupid_")
+ os.close(self.qemu_pidfile[0])
+ self.qemu_pidfile = self.qemu_pidfile[1]
self.host_dumper = HostDumper(dump_host_cmds, dump_dir)
self.monitorpipe = None
@@ -182,8 +187,6 @@ class QemuRunner:
# Ask QEMU to store the QEMU process PID in file, this way we don't have to parse running processes
# and analyze descendents in order to determine it.
- if os.path.exists(self.qemu_pidfile):
- os.remove(self.qemu_pidfile)
self.qemuparams = 'bootparams="{0}" qemuparams="-pidfile {1}"'.format(bootparams, self.qemu_pidfile)
if qemuparams:
self.qemuparams = self.qemuparams[:-1] + " " + qemuparams + " " + '\"'
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [OE-core] [PATCH] qemurunner: Use tempfile.mkstemp to create pidfile name
2020-07-21 21:56 [PATCH] qemurunner: Use tempfile.mkstemp to create pidfile name Richard Purdie
@ 2020-07-21 23:49 ` Joshua Watt
2020-07-22 11:44 ` Richard Purdie
0 siblings, 1 reply; 3+ messages in thread
From: Joshua Watt @ 2020-07-21 23:49 UTC (permalink / raw
To: Richard Purdie; +Cc: OE-core
[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]
On Tue, Jul 21, 2020, 4:56 PM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:
> We continue to see qemu races where qemu fails to write out a pidfile. The
> pidfile name being used was based on the parent process starting the qemus
> so in theory, a qemu shutting down could have deleted the file into which
> the new qemu's pid was about to be written. This shouldn't happen as qemu
> processes should be exitting completely before the code returns.
>
> In the absence of better theory for the failure, use a random pid filename
> rather than the parent pid so there is no overlap in the pid filename,
> thereby removing any theoretical race.
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
> meta/lib/oeqa/utils/qemurunner.py | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/meta/lib/oeqa/utils/qemurunner.py
> b/meta/lib/oeqa/utils/qemurunner.py
> index 519aa9aa1e5..73a3efdc964 100644
> --- a/meta/lib/oeqa/utils/qemurunner.py
> +++ b/meta/lib/oeqa/utils/qemurunner.py
> @@ -20,6 +20,7 @@ import string
> import threading
> import codecs
> import logging
> +import tempfile
> from oeqa.utils.dump import HostDumper
> from collections import defaultdict
>
> @@ -65,7 +66,11 @@ class QemuRunner:
> self.runqemutime = 120
> if not workdir:
> workdir = os.getcwd()
> - self.qemu_pidfile = workdir + '/pidfile_' + str(os.getpid())
> + # Use tempfile to obtain a unique name. We own the directory
> + # so closing the file isn't an issue as long as we don't delete it
> + self.qemu_pidfile = tempfile.mkstemp(dir=workdir,
> prefix="qemupid_")
> + os.close(self.qemu_pidfile[0])
> + self.qemu_pidfile = self.qemu_pidfile[1]
>
Minor, but more idiomatic python:
(fd, self.qemu_pidfile) = tempfile.mkstemp(...)
os.close(fd)
self.host_dumper = HostDumper(dump_host_cmds, dump_dir)
> self.monitorpipe = None
>
> @@ -182,8 +187,6 @@ class QemuRunner:
>
> # Ask QEMU to store the QEMU process PID in file, this way we
> don't have to parse running processes
> # and analyze descendents in order to determine it.
> - if os.path.exists(self.qemu_pidfile):
> - os.remove(self.qemu_pidfile)
> self.qemuparams = 'bootparams="{0}" qemuparams="-pidfile
> {1}"'.format(bootparams, self.qemu_pidfile)
> if qemuparams:
> self.qemuparams = self.qemuparams[:-1] + " " + qemuparams + "
> " + '\"'
> --
> 2.25.1
>
>
>
[-- Attachment #2: Type: text/html, Size: 3609 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [OE-core] [PATCH] qemurunner: Use tempfile.mkstemp to create pidfile name
2020-07-21 23:49 ` [OE-core] " Joshua Watt
@ 2020-07-22 11:44 ` Richard Purdie
0 siblings, 0 replies; 3+ messages in thread
From: Richard Purdie @ 2020-07-22 11:44 UTC (permalink / raw
To: Joshua Watt; +Cc: OE-core
On Tue, 2020-07-21 at 18:49 -0500, Joshua Watt wrote:
>
>
> On Tue, Jul 21, 2020, 4:56 PM Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> > We continue to see qemu races where qemu fails to write out a pidfile. The
> > pidfile name being used was based on the parent process starting the qemus
> > so in theory, a qemu shutting down could have deleted the file into which
> > the new qemu's pid was about to be written. This shouldn't happen as qemu
> > processes should be exitting completely before the code returns.
> >
> > In the absence of better theory for the failure, use a random pid filename
> > rather than the parent pid so there is no overlap in the pid filename,
> > thereby removing any theoretical race.
> >
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> > meta/lib/oeqa/utils/qemurunner.py | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/meta/lib/oeqa/utils/qemurunner.py b/meta/lib/oeqa/utils/qemurunner.py
> > index 519aa9aa1e5..73a3efdc964 100644
> > --- a/meta/lib/oeqa/utils/qemurunner.py
> > +++ b/meta/lib/oeqa/utils/qemurunner.py
> > @@ -20,6 +20,7 @@ import string
> > import threading
> > import codecs
> > import logging
> > +import tempfile
> > from oeqa.utils.dump import HostDumper
> > from collections import defaultdict
> >
> > @@ -65,7 +66,11 @@ class QemuRunner:
> > self.runqemutime = 120
> > if not workdir:
> > workdir = os.getcwd()
> > - self.qemu_pidfile = workdir + '/pidfile_' + str(os.getpid())
> > + # Use tempfile to obtain a unique name. We own the directory
> > + # so closing the file isn't an issue as long as we don't delete it
> > + self.qemu_pidfile = tempfile.mkstemp(dir=workdir, prefix="qemupid_")
> > + os.close(self.qemu_pidfile[0])
> > + self.qemu_pidfile = self.qemu_pidfile[1]
>
> Minor, but more idiomatic python:
>
> (fd, self.qemu_pidfile) = tempfile.mkstemp(...)
> os.close(fd)
Yes!
I'd changed the code a few too many times and it ended up a bit silly.
Sadly the autobuilder showed the same fault with this applied so this
patch doesn't work anyway and probably isn't worth applying :(
If anyone can spot the race...
Cheers,
Richard
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-22 11:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-21 21:56 [PATCH] qemurunner: Use tempfile.mkstemp to create pidfile name Richard Purdie
2020-07-21 23:49 ` [OE-core] " Joshua Watt
2020-07-22 11:44 ` Richard Purdie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.