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-boun...@openvswitch.org] În numele Paul Boca > Trimis: Wednesday, July 6, 2016 3:38 PM > Către: dev@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 <pb...@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? > > 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 > + 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 > 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 > > 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? > + 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? > > 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? > + 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 > + > +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 > + 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 > + 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 > + sys.stderr.write("could not fork: %s\n" % os.strerror(e.errno)) > + sys.exit(1) > + _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
