| Message ID | 1467390468-12900-1-git-send-email-pboca@cloudbasesolutions.com |
|---|---|
| State | Superseded |
| Delegated to: | Guru Shetty |
| Headers | show |
> -----Mesaj original----- > De la: dev [mailto:dev-bounces@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 <pboca@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
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-bounces@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 <pboca@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
On Fri, Jul 01, 2016 at 04:27:49PM +0000, Paul Boca wrote: > 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 <pboca@cloudbasesolutions.com> I'm still not sure that real locking is actually needed for Windows. I believe that, on Windows, the functionality of a lock file only requires holding the file open. For a program to detect whether the lock file is locked, it can just try to delete it; on Windows, I believe that deleting an open file fails.
> > 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 <pboca@cloudbasesolutions.com> > > I'm still not sure that real locking is actually needed for Windows. > I believe that, on Windows, the functionality of a lock file only requires > holding the file open. For a program to detect whether the lock file is locked, > it can just try to delete it; on Windows, I believe that deleting an open file > fails. [Alin Gabriel Serdean: ] That is correct Ben. But think about somebody which is using python and coming from a Unix environment, he will try to read/write to the locked file. In my opinion we should try to mimic that behavior. > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
> > I'm still not sure that real locking is actually needed for Windows. > > I believe that, on Windows, the functionality of a lock file only > > requires holding the file open. For a program to detect whether the > > lock file is locked, it can just try to delete it; on Windows, I > > believe that deleting an open file fails. > [Alin Gabriel Serdean: ] That is correct Ben. But think about somebody which > is using python and coming from a Unix environment, he will try to > read/write to the locked file. In my opinion we should try to mimic that > behavior. Another problem would be we can write over the pid file and if we try to do something like $a = type .\ovs-vswitchd.pid taskkill.exe /PID $a /F Would be bogus because someone could have changed it. Alin.
On Mon, Jul 04, 2016 at 06:02:31PM +0000, Alin Serdean wrote: > > > I'm still not sure that real locking is actually needed for Windows. > > > I believe that, on Windows, the functionality of a lock file only > > > requires holding the file open. For a program to detect whether the > > > lock file is locked, it can just try to delete it; on Windows, I > > > believe that deleting an open file fails. > > [Alin Gabriel Serdean: ] That is correct Ben. But think about somebody which > > is using python and coming from a Unix environment, he will try to > > read/write to the locked file. In my opinion we should try to mimic that > > behavior. > Another problem would be we can write over the pid file and if we try to do something like > $a = type .\ovs-vswitchd.pid > taskkill.exe /PID $a /F > Would be bogus because someone could have changed it. It's OK with me if we actually lock the lockfiles, it just seems less necessary on Windows.
diff --git a/lib/daemon-windows.c b/lib/daemon-windows.c index 8cf0fea..4c4724b 100644 --- a/lib/daemon-windows.c +++ b/lib/daemon-windows.c @@ -18,6 +18,7 @@ #include "daemon.h" #include "daemon-private.h" #include <stdio.h> +#include <io.h> #include <stdlib.h> #include "dirs.h" #include "ovs-thread.h" @@ -26,6 +27,13 @@ VLOG_DEFINE_THIS_MODULE(daemon_windows); +/* Constants for flock function */ +#define LOCK_SH 0x0 /* Shared lock. */ +#define LOCK_UN 0x80000000 /* Unlock. Custom value. */ + +/* High-order 32 bits of byte range to lock */ +#define LOCK_MAX_SIZE 0xFFFF0000 + static bool service_create; /* Was --service specified? */ static bool service_started; /* Have we dispatched service to start? */ @@ -414,6 +422,33 @@ unlink_pidfile(void) } } +static int +flock(FILE* fd, int operation) +{ + NTSTATUS status; + HANDLE hFile; + OVERLAPPED ov = {0}; + + hFile = (HANDLE)_get_osfhandle(fileno(filep_pidfile)); + 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) { + 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); } + 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. */ }
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 <pboca@cloudbasesolutions.com> --- V2: No changes V3: No changes V4: No changes V5: Removed this patch from Python series. Removed unused constants and defined 0xFFFF0000 as constant. Updated commit text. --- lib/daemon-windows.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)