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@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-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? [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) > > + _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
