[ovs-dev] [PATCH V6 12/17] python tests: Ported Python daemon to Windows
Paul Boca
pboca at cloudbasesolutions.com
Fri Jul 15 12:04:44 UTC 2016
Hi Alin,
Comments inlined.
Thanks for review,
Paul
> -----Original Message-----
> From: Alin Serdean
> Sent: Friday, July 15, 2016 2:28 AM
> To: Paul Boca; dev at openvswitch.org
> Subject: RE: [ovs-dev] [PATCH V6 12/17] python tests: Ported Python daemon
> to Windows
>
> Thanks a lot for the patch.
>
> In my opinion I think this part should go in a different file.
>
> The patch looks good overall some comments questions/inlined.
>
> > -----Mesaj original-----
> > De la: dev [mailto:dev-bounces at openvswitch.org] În numele Paul Boca
> > Trimis: Wednesday, July 6, 2016 3:38 PM
> > Către: dev at openvswitch.org
> > Subiect: [ovs-dev] [PATCH V6 12/17] python tests: Ported Python daemon
> to
> > Windows
> >
> > Used subprocess.Popen instead os.fork (not implemented on windows)
> and
> > repaced of os.pipe with Windows pipes.
> >
> > To be able to identify the child process I added an extra parameter to
> > daemon process '--pipe-handle', this parameter also contains the parent
> > Windows pipe handle, used by the child to signal the start.
> >
> > The PID file is created directly on Windows, without using a temporary file
> > because the symbolic link doesn't inheriths the file lock set on temporary
> file.
> >
> > Signed-off-by: Paul-Daniel Boca <pboca at cloudbasesolutions.com>
> > import errno
> > -import fcntl
> > import os
> > -import resource
> > import signal
> > import sys
> > import time
> > +import threading
> [Alin Gabriel Serdean: ] is threading needed for both or just on Windows?
[Paul Boca] This is needed only on Windows, will fix
> >
> > import ovs.dirs
> > import ovs.fatal_signal
> > @@ -28,6 +27,15 @@ import ovs.timeval
> > import ovs.util
> > import ovs.vlog
> >
> > +if sys.platform == "win32":
> > + import ovs.fcntl_win as fcntl
> > + import win32file, win32pipe, win32api, win32security, win32con,
> > pywintypes
> [Alin Gabriel Serdean: ] this is an external library we need to add it to the
> docs as a prerequisite
[Paul Boca] You are right, will update the docs for Windows
> > + import msvcrt
> > + import subprocess
> > +else:
> > + import fcntl
> > + import resource
> > +
> > vlog = ovs.vlog.Vlog("daemon")
> >
> > # --detach: Should we run in the background?
> > @@ -53,6 +61,9 @@ _monitor = False
> > # File descriptor used by daemonize_start() and daemonize_complete().
> > _daemonize_fd = None
> >
> > +# Running as the child process - Windows only.
> > +_detached = False
> > +
> [Alin Gabriel Serdean: ] Maybe add it just on windows with the sys check
[Paul Boca] Will do
> > RESTART_EXIT_CODE = 5
> >
> >
> > @@ -109,13 +120,20 @@ def set_monitor():
> > global _monitor
> > _monitor = True
> >
> > +def set_detached(wp):
> > + """Sets up a following call to daemonize() to fork a supervisory process
> to
> > + monitor the daemon and restart it if it dies due to an error signal."""
> > + global _detached
> > + global _daemonize_fd
> > + _detached = True
> > + _daemonize_fd = int(wp)
> > +
> [Alin Gabriel Serdean: ] [Alin Gabriel Serdean: ] Maybe add it just on
> windows with the sys check or add a comment that it should be used on
> windows only
[Paul Boca] Will add a check 'win32'
> >
> > def _fatal(msg):
> > vlog.err(msg)
> > sys.stderr.write("%s\n" % msg)
> > sys.exit(1)
> >
> > -
> > def _make_pidfile():
> > """If a pidfile has been configured, creates it and stores the running
> > process's pid in it. Ensures that the pidfile will be deleted when the @@
> -
> > 123,8 +141,12 @@ def _make_pidfile():
> > pid = os.getpid()
> >
> > # Create a temporary pidfile.
> > - tmpfile = "%s.tmp%d" % (_pidfile, pid)
> > - ovs.fatal_signal.add_file_to_unlink(tmpfile)
> > + if sys.platform == "win32":
> > + tmpfile = _pidfile
> [Alin Gabriel Serdean: ] shouldn't we add
> "ovs.fatal_signal.add_file_to_unlink" here as well or we cannot use it?
[Paul Boca] It is added later in this function with the second parameter set
but I will move it here to be easier to follow
> > + else:
> > + tmpfile = "%s.tmp%d" % (_pidfile, pid)
> > + ovs.fatal_signal.add_file_to_unlink(tmpfile)
> > +
> > try:
> > # This is global to keep Python from garbage-collecting and
> > # therefore closing our file after this function exits. That would @@ -
> > 147,40 +169,47 @@ def _make_pidfile():
> > _fatal("%s: write failed: %s" % (tmpfile, e.strerror))
> >
> > try:
> > - # Rename or link it to the correct name.
> > - if _overwrite_pidfile:
> > - try:
> > - os.rename(tmpfile, _pidfile)
> > - except OSError as e:
> > - _fatal("failed to rename \"%s\" to \"%s\" (%s)"
> > - % (tmpfile, _pidfile, e.strerror))
> > - else:
> > - while True:
> > + if sys.platform != "win32":
> > + # Rename or link it to the correct name.
> > + if _overwrite_pidfile:
> > try:
> > - os.link(tmpfile, _pidfile)
> > - error = 0
> > + os.rename(tmpfile, _pidfile)
> > except OSError as e:
> > - error = e.errno
> > - if error == errno.EEXIST:
> > - _check_already_running()
> > - elif error != errno.EINTR:
> > - break
> > - if error:
> > - _fatal("failed to link \"%s\" as \"%s\" (%s)"
> > - % (tmpfile, _pidfile, os.strerror(error)))
> > -
> > - # Ensure that the pidfile will get deleted on exit.
> > - ovs.fatal_signal.add_file_to_unlink(_pidfile)
> > -
> > - # Delete the temporary pidfile if it still exists.
> > - if not _overwrite_pidfile:
> > - error = ovs.fatal_signal.unlink_file_now(tmpfile)
> > - if error:
> > - _fatal("%s: unlink failed (%s)" % (tmpfile, os.strerror(error)))
> > + _fatal("failed to rename \"%s\" to \"%s\" (%s)"
> > + % (tmpfile, _pidfile, e.strerror))
> > + else:
> > + while True:
> > + try:
> > + os.link(tmpfile, _pidfile)
> > + error = 0
> > + except OSError as e:
> > + error = e.errno
> > + if error == errno.EEXIST:
> > + _check_already_running()
> > + elif error != errno.EINTR:
> > + break
> > + if error:
> > + _fatal("failed to link \"%s\" as \"%s\" (%s)"
> > + % (tmpfile, _pidfile, os.strerror(error)))
> > +
> > + # Ensure that the pidfile will get deleted on exit.
> > + ovs.fatal_signal.add_file_to_unlink(_pidfile)
> > +
> > + # Delete the temporary pidfile if it still exists.
> > + if not _overwrite_pidfile:
> > + error = ovs.fatal_signal.unlink_file_now(tmpfile)
> > + if error:
> > + _fatal("%s: unlink failed (%s)" % (tmpfile, os.strerror(error)))
> > + else:
> > + # Ensure that the pidfile will gets closed and deleted on exit.
> > + ovs.fatal_signal.add_file_to_close_and_unlink(_pidfile,
> > + file_handle)
> [Alin Gabriel Serdean: ] can't we use ovs.fatal_signal.add_file_to_unlink?
> Shouldn't we retry to rename the file?
[Paul Boca] The code checks if the lock over the file can be acquired,
no need to try to rename it in my opinion.
add_file_to_close_and_unlink is a function I added, that closes the file descriptor
before trying to delete it (on Windows the file cannot be deleted while it is open)
> >
> > global _pidfile_dev
> > global _pidfile_ino
> > @@ -204,6 +233,97 @@ def _waitpid(pid, options):
> > pass
> > return -e.errno, 0
> >
> > +def _windows_create_pipe():
> > + sAttrs = win32security.SECURITY_ATTRIBUTES()
> > + sAttrs.bInheritHandle = 1
> > +
> > + (read_pipe, write_pipe) = win32pipe.CreatePipe(sAttrs, 0)
> > + pid = win32api.GetCurrentProcess()
> > + write_pipe2 = win32api.DuplicateHandle(pid, write_pipe, pid, 0, 1,
> > win32con.DUPLICATE_SAME_ACCESS)
> [Alin Gabriel Serdean: ] maybe I am missing something but why do we need
> the duplicate?
[Paul Boca] On Python3 a file handle doesn't have INHERIT flag by default for child
process, so a duplicate handle is needed
> > + win32file.CloseHandle(write_pipe)
> > +
> > + return (read_pipe, write_pipe2)
> > +
> > +def __windows_fork_notify_startup(fd):
> > + if fd is not None:
> > + try:
> > + win32file.WriteFile(fd, "0", None)
> > + except:
> > + win32file.WriteFile(fd, bytes("0", 'UTF-8'), None)
> [Alin Gabriel Serdean: ] I don't really understand the utf-8 part and also that
> may throw an exception as well
[Paul Boca] On python3 the call win32file.WriteFile(fd, "0", None) throws an error,
and the second call will run ok, also win32file.WriteFile(fd, bytes("0", 'UTF-8'), None)
throws an error on Python2
> > +
> > +def _windows_read_pipe(fd):
> > + if fd is not None:
> > + sAttrs = win32security.SECURITY_ATTRIBUTES()
> > + sAttrs.bInheritHandle = 1
> > + overlapped = pywintypes.OVERLAPPED()
> [Alin Gabriel Serdean: ] sAttrs and overlapped not used
[Paul Boca] flake8 warned me about this, will be fixed in next version
> > + try:
> > + (ret, data) = win32file.ReadFile(fd, 1, None)
> > + return data
> > + except pywintypes.error as e:
> > + raise OSError(errno.EIO, "", "")
> > +
> > +def _windows_detach(proc, wfd):
> > + """ If the child process closes and it was detached
> > + then close the communication pipe so the parent process
> > + can terminate """
> > + proc.wait()
> > + win32file.CloseHandle(wfd)
> > +
> > +def _windows_fork_and_wait_for_startup():
> > + if _detached:
> > + return 0
> > +
> > + ''' close the log file, on Windows cannot be moved while the parent has
> > + a reference on it.'''
> > + vlog.close_log_file()
> > +
> > + try:
> > + (rfd, wfd) = _windows_create_pipe()
> > + except OSError as e:
> [Alin Gabriel Serdean: ] I think it's a pywintypes.error not oserror
[Paul Boca] Right, thanks
> > + sys.stderr.write("pipe failed: %s\n" % os.strerror(e.errno))
> > + sys.exit(1)
> > +
> > + try:
> > + proc = subprocess.Popen("%s %s --pipe-handle=%ld" %
> > (sys.executable, " ".join(sys.argv), int(wfd)), close_fds=False, shell=False)
> > + pid = proc.pid
> > + #start a thread and wait the subprocess exit code
> > + thread = threading.Thread(target=_windows_detach, args=(proc,
> wfd))
> > + thread.daemon = True
> > + thread.start()
> > + except OSError as e:
> [Alin Gabriel Serdean: ] pywintypes.error needs to be treated as well because
> closehandle can throw an exception
[Paul Boca] Right, thanks
> > + sys.stderr.write("could not fork: %s\n" % os.strerror(e.errno))
> > + sys.exit(1)
> > +
More information about the dev
mailing list