Hi Alin! Thanks for review. You are right with all the observations. I will send another patch that will fix them.
Thanks, Paul > -----Original Message----- > From: Alin Serdean > Sent: Monday, July 4, 2016 5:41 PM > To: Paul Boca; dev@openvswitch.org > Subject: RE: [PATCH V5] windows: Added lockf function and lock PID file > > > -----Mesaj original----- > > De la: dev [mailto:dev-boun...@openvswitch.org] În numele Paul Boca > > Trimis: Friday, July 1, 2016 7:28 PM > > Către: dev@openvswitch.org > > Subiect: [ovs-dev] [PATCH V5] windows: Added lockf function and lock PID > > file > > > > If the PID file isn't locked then appctl.py detects it as stale and bails > > out > > without doing anything. Because of this lots of Python tests fail. > > Also this protects the PID file from being overwritten. > > > > I used only shared lock, in order to be compatible with Python tests, which > > try to acquire the lock exclusively. On Windows if the exclusive lock is > > used, > > than the read access is denied too for other instances of this file. > > > > Signed-off-by: Paul-Daniel Boca <pb...@cloudbasesolutions.com> > > +static int > [Alin Gabriel Serdean: ] I don't really understand why 'int' when you are > returning -1/0. The problem is: you need to deal with GetLastError and errno > if a specific function failed. > > +flock(FILE* fd, int operation) > > +{ > > + NTSTATUS status; > > + HANDLE hFile; > > + OVERLAPPED ov = {0}; > > + > > + hFile = (HANDLE)_get_osfhandle(fileno(filep_pidfile)); > [Alin Gabriel Serdean: ] either use fd or drop the parameter fd. > > + if (hFile == INVALID_HANDLE_VALUE) { > > + VLOG_FATAL("Invalid handle value"); > > + return -1; > > + } > > + > > + if (operation & LOCK_UN) { > > + if (UnlockFileEx(hFile, 0, 0, LOCK_MAX_SIZE, &ov) == FALSE) { > [Alin Gabriel Serdean: ] According to the docs UnlockFileEx can be 0 or NULL > https://msdn.microsoft.com/en- > us/library/windows/desktop/aa365716(v=vs.85).aspx > > > + return -1; > > + } > > + } else { > > + if (LockFileEx(hFile, operation, 0, 0, LOCK_MAX_SIZE, &ov) == > > FALSE) { > > + VLOG_FATAL("LockFileEx failed, status = 0x%08x\n", status); > > + return -1; > > + } > > + } > > + > > + return 0; > > +} > > + > > /* 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 > > * process exits. */ > > @@ -444,6 +479,14 @@ make_pidfile(void) > > VLOG_FATAL("Failed to write into the pidfile %s", pidfile); > > } > > > [Alin Gabriel Serdean: ] Unless I am missing something: there is another call > to fflush just above, do we need to flush the buffers again? > > + fflush(filep_pidfile); > > + error = flock(filep_pidfile, LOCK_SH); > > + if (error) { > > + /* Looks like we failed to acquire the lock. */ > > + VLOG_FATAL("%s: fcntl(F_SETLK) failed (%s)", pidfile, > > + ovs_strerror(error)); > > + } > > + > > /* Don't close the pidfile till the process exits. */ } > > > > -- > > 2.7.2.windows.1 > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
