qemurunner: Use tempfile.mkstemp to create pidfile name

Submitted by Richard Purdie on July 21, 2020, 9:56 p.m. | Patch ID: 174596

Details

Message ID 20200721215640.136052-1-richard.purdie@linuxfoundation.org
State Master Next
Commit 951c70472b8cd9259dd354f848cf98d9f598888d
Headers show

Commit Message

Richard Purdie July 21, 2020, 9:56 p.m.
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(-)

Patch hide | download patch | download mbox

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 + " " + '\"'

Comments

Joshua Watt July 21, 2020, 11:49 p.m.
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
>
> 
>
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.

View/Reply Online (#140848): https://lists.openembedded.org/g/openembedded-core/message/140848
Mute This Topic: https://lists.openembedded.org/mt/75713689/3617530
Group Owner: openembedded-core+owner@lists.openembedded.org
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub  [oe-patchwork@oe-patch.openembedded.org]
-=-=-=-=-=-=-=-=-=-=-=-
Richard Purdie July 22, 2020, 11:44 a.m.
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
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.

View/Reply Online (#140852): https://lists.openembedded.org/g/openembedded-core/message/140852
Mute This Topic: https://lists.openembedded.org/mt/75713689/3617530
Group Owner: openembedded-core+owner@lists.openembedded.org
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub  [oe-patchwork@oe-patch.openembedded.org]
-=-=-=-=-=-=-=-=-=-=-=-