| Message ID | 1468430683-17972-1-git-send-email-pboca@cloudbasesolutions.com |
|---|---|
| State | Superseded |
| Delegated to: | Guru Shetty |
| Headers | show |
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > -----Mesaj original----- > De la: dev [mailto:dev-bounces@openvswitch.org] În numele Paul Boca > Trimis: Wednesday, July 13, 2016 8:35 PM > Către: dev@openvswitch.org > Subiect: [ovs-dev] [PATCH V7] 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> > --- > 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. > V6: flock returns now the error of GetLastError. > Also it uses fd parameter instead of the global filep_pidfile. > Removed unused local variable and unnecessary error message. > V7: Keep a shared lock on PID file after the fopen. > This ensures us that at most one process can write the same PID file. > fopen with 'w' on Windows creates a file with FILE_SHARED_WRITE > flag, so anyone with write access on file can write it. > --- > lib/daemon-windows.c | 41 > +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/lib/daemon-windows.c b/lib/daemon-windows.c index > 8cf0fea..e6e1452 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,14 @@ > > VLOG_DEFINE_THIS_MODULE(daemon_windows); > > +/* Constants for flock function */ > +#define LOCK_SH 0x0 /* Shared lock. */ > +#define LOCK_EX LOCKFILE_EXCLUSIVE_LOCK /* Exclusive > 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? */ > > @@ -404,9 +413,35 @@ detach_process(int argc, char *argv[]) } > > static void > +flock(FILE* fd, int operation) > +{ > + HANDLE hFile; > + OVERLAPPED ov = {0}; > + > + hFile = (HANDLE)_get_osfhandle(fileno(fd)); > + if (hFile == INVALID_HANDLE_VALUE) { > + VLOG_FATAL("Invalid handle value"); > + } > + > + if (operation & LOCK_UN) { > + if (UnlockFileEx(hFile, 0, 0, LOCK_MAX_SIZE, &ov) == 0) { > + VLOG_FATAL("Failed to unlock PID file (%s).", > + ovs_lasterror_to_string()); > + } > + } else { > + if (LockFileEx(hFile, operation, 0, 0, LOCK_MAX_SIZE, &ov) == FALSE) { > + VLOG_FATAL("Failed to lock PID file (%s).", > + ovs_lasterror_to_string()); > + } > + } > +} > + > +static void > unlink_pidfile(void) > { > if (filep_pidfile) { > + /* Remove the shared lock on file */ > + flock(filep_pidfile, LOCK_UN); > fclose(filep_pidfile); > } > if (pidfile) { > @@ -437,6 +472,8 @@ make_pidfile(void) > VLOG_FATAL("failed to open %s (%s)", pidfile, ovs_strerror(errno)); > } > > + flock(filep_pidfile, LOCK_EX); > + > fatal_signal_add_hook(unlink_pidfile, NULL, NULL, true); > > fprintf(filep_pidfile, "%d\n", _getpid()); @@ -444,6 +481,10 @@ > make_pidfile(void) > VLOG_FATAL("Failed to write into the pidfile %s", pidfile); > } > > + flock(filep_pidfile, LOCK_SH); > + /* This will remove the exclusive lock. The shared lock will remain */ > + flock(filep_pidfile, LOCK_UN); > + > /* 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 13 July 2016 at 10:35, Paul Boca <pboca@cloudbasesolutions.com> 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> > --- > 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. > V6: flock returns now the error of GetLastError. > Also it uses fd parameter instead of the global filep_pidfile. > Removed unused local variable and unnecessary error message. > V7: Keep a shared lock on PID file after the fopen. > This ensures us that at most one process can write the same PID file. > fopen with 'w' on Windows creates a file with FILE_SHARED_WRITE > flag, so anyone with write access on file can write it. > --- > lib/daemon-windows.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/lib/daemon-windows.c b/lib/daemon-windows.c > index 8cf0fea..e6e1452 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,14 @@ > > VLOG_DEFINE_THIS_MODULE(daemon_windows); > > +/* Constants for flock function */ > +#define LOCK_SH 0x0 /* Shared lock. */ > +#define LOCK_EX LOCKFILE_EXCLUSIVE_LOCK /* Exclusive lock. */ > When reading code, "LOCK_SH" does not tell me much. Can you please expand it to LOCK_SHARED. You can get rid of LOCK_EX and instead use the real value. > +#define LOCK_UN 0x80000000 /* Unlock. Custom > value. */ > + > +/* High-order 32 bits of byte range to lock */ > +#define LOCK_MAX_SIZE 0xFFFF0000 > Any reason for this specific value? Anyone reading the code will start wondering why this is important. If we don't care about it, why not just do the same as the other invocation of LockFileEx does in the code base (lib/lockfile.c). i.e. just set 1 in nNumberOfBytesToLockLow. > + > static bool service_create; /* Was --service specified? */ > static bool service_started; /* Have we dispatched service to > start? */ > > @@ -404,9 +413,35 @@ detach_process(int argc, char *argv[]) > } > > static void > +flock(FILE* fd, int operation) > +{ > + HANDLE hFile; > + OVERLAPPED ov = {0}; > + > + hFile = (HANDLE)_get_osfhandle(fileno(fd)); > + if (hFile == INVALID_HANDLE_VALUE) { > + VLOG_FATAL("Invalid handle value"); > The above message is not very helpful when someone sees it on the console. Please have a look at other VLOG_FATAL messages in this file to see what kind of messages are more helpful. > + } > + > + if (operation & LOCK_UN) { > + if (UnlockFileEx(hFile, 0, 0, LOCK_MAX_SIZE, &ov) == 0) { > + VLOG_FATAL("Failed to unlock PID file (%s).", > + ovs_lasterror_to_string()); > + } > + } else { > + if (LockFileEx(hFile, operation, 0, 0, LOCK_MAX_SIZE, &ov) == > FALSE) { > + VLOG_FATAL("Failed to lock PID file (%s).", > + ovs_lasterror_to_string()); > + } > + } > +} > + > +static void > unlink_pidfile(void) > { > if (filep_pidfile) { > + /* Remove the shared lock on file */ > + flock(filep_pidfile, LOCK_UN); > fclose(filep_pidfile); > } > if (pidfile) { > @@ -437,6 +472,8 @@ make_pidfile(void) > VLOG_FATAL("failed to open %s (%s)", pidfile, > ovs_strerror(errno)); > } > > + flock(filep_pidfile, LOCK_EX); > Won't the above call block indefinitely if the lock has already been taken by someone else? We don't want the behavior when someone starts a daemon and it simply hang there. > + > fatal_signal_add_hook(unlink_pidfile, NULL, NULL, true); > > fprintf(filep_pidfile, "%d\n", _getpid()); > @@ -444,6 +481,10 @@ make_pidfile(void) > VLOG_FATAL("Failed to write into the pidfile %s", pidfile); > } > > + flock(filep_pidfile, LOCK_SH); > + /* This will remove the exclusive lock. The shared lock will remain */ > + flock(filep_pidfile, LOCK_UN); > I unsuccessfully tried to read MSDN documentation for the above behavior. Can you please point me to the documentation which states that unlocking once will move the lock from exclusive to shared? > + > /* 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 >
> > + flock(filep_pidfile, LOCK_EX); > > > Won't the above call block indefinitely if the lock has already been taken by > someone else? We don't want the behavior when someone starts a daemon > and it simply hang there. [Alin Gabriel Serdean: ] Hard to say from the MSDN documentation we could add "| LOCKFILE_FAIL_IMMEDIATELY" when calling it just to be sure. > > > > > + > > fatal_signal_add_hook(unlink_pidfile, NULL, NULL, true); > > > > fprintf(filep_pidfile, "%d\n", _getpid()); @@ -444,6 +481,10 @@ > > make_pidfile(void) > > VLOG_FATAL("Failed to write into the pidfile %s", pidfile); > > } > > > > + flock(filep_pidfile, LOCK_SH); > > + /* This will remove the exclusive lock. The shared lock will remain */ > > + flock(filep_pidfile, LOCK_UN); > > > I unsuccessfully tried to read MSDN documentation for the above behavior. > Can you please point me to the documentation which states that unlocking > once will move the lock from exclusive to shared? [Alin Gabriel Serdean: ] https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx The part: "If the same range is locked with an exclusive and a shared lock, two unlock operations are necessary to unlock the region; the first unlock operation unlocks the exclusive lock, the second unlock operation unlocks the shared lock."
On 14 July 2016 at 15:52, Alin Serdean <aserdean@cloudbasesolutions.com> wrote: > > > + flock(filep_pidfile, LOCK_EX); > > > > > Won't the above call block indefinitely if the lock has already been > taken by > > someone else? We don't want the behavior when someone starts a daemon > > and it simply hang there. > [Alin Gabriel Serdean: ] Hard to say from the MSDN documentation we could > add "| LOCKFILE_FAIL_IMMEDIATELY" when calling it just to be sure. > > > > > > > > > + > > > fatal_signal_add_hook(unlink_pidfile, NULL, NULL, true); > > > > > > fprintf(filep_pidfile, "%d\n", _getpid()); @@ -444,6 +481,10 @@ > > > make_pidfile(void) > > > VLOG_FATAL("Failed to write into the pidfile %s", pidfile); > > > } > > > > > > + flock(filep_pidfile, LOCK_SH); > > > + /* This will remove the exclusive lock. The shared lock will > remain */ > > > + flock(filep_pidfile, LOCK_UN); > > > > > I unsuccessfully tried to read MSDN documentation for the above behavior. > > Can you please point me to the documentation which states that unlocking > > once will move the lock from exclusive to shared? > [Alin Gabriel Serdean: ] > https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx > The part: > "If the same range is locked with an exclusive and a shared lock, two > unlock operations are necessary to unlock the region; the first unlock > operation unlocks the exclusive lock, the second unlock operation unlocks > the shared lock." > I see. Thank you. (I am blind!). I wonder whether we actually need the exclusive lock then? If what we need is just shared lock, why not just take it in the beginning? Would it have been possible to open the file in write mode if someone had taken the exclusive lock in the first place?
[Alin Gabriel Serdean: ] https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx The part: "If the same range is locked with an exclusive and a shared lock, two unlock operations are necessary to unlock the region; the first unlock operation unlocks the exclusive lock, the second unlock operation unlocks the shared lock." I see. Thank you. (I am blind!). I wonder whether we actually need the exclusive lock then? If what we need is just shared lock, why not just take it in the beginning? Would it have been possible to open the file in write mode if someone had taken the exclusive lock in the first place? [Alin Gabriel Serdean: ] No worries the MS documentation is not that easy to follow or precise in certain scenarios ☺. The problem is “Locking a portion of a file for shared access denies all processes write access to the specified region of the file, including the process that first locks the region. All processes can read the locked region.” “Locking a portion of a file for exclusive access denies all other processes both read and write access to the specified region of the file. Locking a region that goes beyond the current end-of-file position is not an error.” I agree it doesn’t make any sense why a shared lock would block the owner but probably they had reasons to implement it that way.
On 14 July 2016 at 19:25, Alin Serdean <aserdean@cloudbasesolutions.com> wrote: > > [Alin Gabriel Serdean: ] > https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx > The part: > "If the same range is locked with an exclusive and a shared lock, two > unlock operations are necessary to unlock the region; the first unlock > operation unlocks the exclusive lock, the second unlock operation unlocks > the shared lock." > > > > I see. Thank you. (I am blind!). I wonder whether we actually need the > exclusive lock then? If what we need is just shared lock, why not just take > it in the beginning? Would it have been possible to open the file in write > mode if someone had taken the exclusive lock in the first place? > > > > [Alin Gabriel Serdean: ] No worries the MS documentation is not that easy > to follow or precise in certain scenarios J. > > The problem is “Locking a portion of a file for *shared access denies* *all > processes* *write access* to the specified region of the file, including > the process that first locks the region. All processes can read the locked > region.” > > “Locking a portion of a file for *exclusive access* * denies all other > processes both read and write* access to the specified region of the > file. Locking a region that goes beyond the current end-of-file position is > not an error.” > > I agree it doesn’t make any sense why a shared lock would block the owner > but probably they had reasons to implement it that way. > What I was trying to say was, we open the file in write mode. It wouldn't have been successful if someone else had taken a lock (exclusive or shared). Am I correct? If so, why take exclusive lock again to write? Just write the pid and then take shared lock. Would that cause problems? (I must be missing some subtlety here.)
De la: Guru Shetty [mailto:guru@ovn.org] Trimis: Friday, July 15, 2016 5:49 AM Către: Alin Serdean <aserdean@cloudbasesolutions.com> Cc: Paul Boca <pboca@cloudbasesolutions.com>; dev@openvswitch.org Subiect: Re: [ovs-dev] [PATCH V7] windows: Added lockf function and lock PID file On 14 July 2016 at 19:25, Alin Serdean <aserdean@cloudbasesolutions.com<mailto:aserdean@cloudbasesolutions.com>> wrote: [Alin Gabriel Serdean: ] https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx The part: "If the same range is locked with an exclusive and a shared lock, two unlock operations are necessary to unlock the region; the first unlock operation unlocks the exclusive lock, the second unlock operation unlocks the shared lock." I see. Thank you. (I am blind!). I wonder whether we actually need the exclusive lock then? If what we need is just shared lock, why not just take it in the beginning? Would it have been possible to open the file in write mode if someone had taken the exclusive lock in the first place? [Alin Gabriel Serdean: ] No worries the MS documentation is not that easy to follow or precise in certain scenarios ☺. The problem is “Locking a portion of a file for shared access denies all processes write access to the specified region of the file, including the process that first locks the region. All processes can read the locked region.” “Locking a portion of a file for exclusive access denies all other processes both read and write access to the specified region of the file. Locking a region that goes beyond the current end-of-file position is not an error.” I agree it doesn’t make any sense why a shared lock would block the owner but probably they had reasons to implement it that way. What I was trying to say was, we open the file in write mode. It wouldn't have been successful if someone else had taken a lock (exclusive or shared). Am I correct? If so, why take exclusive lock again to write? Just write the pid and then take shared lock. Would that cause problems? (I must be missing some subtlety here.) [Alin Gabriel Serdean: ] If we just open the file in write mode and write the pid and then take the shared lock, someone else could write in the file between the moment we opened it and gained the shared lock.
Another option would be to ditch fopen(which opens a file with FILE_SHARE_WRITE | FILE_SHARE_WRITE) and use CreateFile(https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx ) without FILE_SHARE_WRITE and from python check if we can write to it (since only the owner can write to it). De la: Guru Shetty [mailto:guru@ovn.org] Trimis: Friday, July 15, 2016 5:49 AM Către: Alin Serdean <aserdean@cloudbasesolutions.com<mailto:aserdean@cloudbasesolutions.com>> Cc: Paul Boca <pboca@cloudbasesolutions.com<mailto:pboca@cloudbasesolutions.com>>; dev@openvswitch.org<mailto:dev@openvswitch.org> Subiect: Re: [ovs-dev] [PATCH V7] windows: Added lockf function and lock PID file What I was trying to say was, we open the file in write mode. It wouldn't have been successful if someone else had taken a lock (exclusive or shared). Am I correct? If so, why take exclusive lock again to write? Just write the pid and then take shared lock. Would that cause problems? (I must be missing some subtlety here.) [Alin Gabriel Serdean: ] If we just open the file in write mode and write the pid and then take the shared lock, someone else could write in the file between the moment we opened it and gained the shared lock.
On 14 July 2016 at 20:03, Alin Serdean <aserdean@cloudbasesolutions.com> wrote: > > > De la: Guru Shetty [mailto:guru@ovn.org] > Trimis: Friday, July 15, 2016 5:49 AM > Către: Alin Serdean <aserdean@cloudbasesolutions.com> > Cc: Paul Boca <pboca@cloudbasesolutions.com>; dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH V7] windows: Added lockf function and lock > PID file > > > > On 14 July 2016 at 19:25, Alin Serdean <aserdean@cloudbasesolutions.com > <mailto:aserdean@cloudbasesolutions.com>> wrote: > > [Alin Gabriel Serdean: ] > https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx > The part: > "If the same range is locked with an exclusive and a shared lock, two > unlock operations are necessary to unlock the region; the first unlock > operation unlocks the exclusive lock, the second unlock operation unlocks > the shared lock." > > I see. Thank you. (I am blind!). I wonder whether we actually need the > exclusive lock then? If what we need is just shared lock, why not just take > it in the beginning? Would it have been possible to open the file in write > mode if someone had taken the exclusive lock in the first place? > > [Alin Gabriel Serdean: ] No worries the MS documentation is not that easy > to follow or precise in certain scenarios ☺. > The problem is “Locking a portion of a file for shared access denies all > processes write access to the specified region of the file, including the > process that first locks the region. All processes can read the locked > region.” > “Locking a portion of a file for exclusive access denies all other > processes both read and write access to the specified region of the file. > Locking a region that goes beyond the current end-of-file position is not > an error.” > I agree it doesn’t make any sense why a shared lock would block the owner > but probably they had reasons to implement it that way. > > What I was trying to say was, we open the file in write mode. It wouldn't > have been successful if someone else had taken a lock (exclusive or > shared). Am I correct? If so, why take exclusive lock again to write? Just > write the pid and then take shared lock. Would that cause problems? (I must > be missing some subtlety here.) > [Alin Gabriel Serdean: ] If we just open the file in write mode and write > the pid and then take the shared lock, someone else could write in the file > between the moment we opened it and gained the shared lock. > > Thanks, I understand it now. (I had to write a small program to get it.) I did see though that a daemon will hang if it tries to take a exclusive lock without LOCKFILE_FAIL_IMMEDIATELY.
Hi Guru! I will re-spin the patch with LOCKFILE_FAIL_IMMEDIATELY flag set to avoid the hang of other daemons. Thanks, Paul > -----Original Message----- > From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Guru Shetty > Sent: Friday, July 15, 2016 6:58 AM > To: Alin Serdean > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH V7] windows: Added lockf function and lock > PID file > > On 14 July 2016 at 20:03, Alin Serdean <aserdean@cloudbasesolutions.com> > wrote: > > > > > > > De la: Guru Shetty [mailto:guru@ovn.org] > > Trimis: Friday, July 15, 2016 5:49 AM > > Către: Alin Serdean <aserdean@cloudbasesolutions.com> > > Cc: Paul Boca <pboca@cloudbasesolutions.com>; dev@openvswitch.org > > Subiect: Re: [ovs-dev] [PATCH V7] windows: Added lockf function and lock > > PID file > > > > > > > > On 14 July 2016 at 19:25, Alin Serdean <aserdean@cloudbasesolutions.com > > <mailto:aserdean@cloudbasesolutions.com>> wrote: > > > > [Alin Gabriel Serdean: ] > > https://msdn.microsoft.com/en- > us/library/windows/desktop/aa365203(v=vs.85).aspx > > The part: > > "If the same range is locked with an exclusive and a shared lock, two > > unlock operations are necessary to unlock the region; the first unlock > > operation unlocks the exclusive lock, the second unlock operation unlocks > > the shared lock." > > > > I see. Thank you. (I am blind!). I wonder whether we actually need the > > exclusive lock then? If what we need is just shared lock, why not just take > > it in the beginning? Would it have been possible to open the file in write > > mode if someone had taken the exclusive lock in the first place? > > > > [Alin Gabriel Serdean: ] No worries the MS documentation is not that easy > > to follow or precise in certain scenarios ☺. > > The problem is “Locking a portion of a file for shared access denies all > > processes write access to the specified region of the file, including the > > process that first locks the region. All processes can read the locked > > region.” > > “Locking a portion of a file for exclusive access denies all other > > processes both read and write access to the specified region of the file. > > Locking a region that goes beyond the current end-of-file position is not > > an error.” > > I agree it doesn’t make any sense why a shared lock would block the owner > > but probably they had reasons to implement it that way. > > > > What I was trying to say was, we open the file in write mode. It wouldn't > > have been successful if someone else had taken a lock (exclusive or > > shared). Am I correct? If so, why take exclusive lock again to write? Just > > write the pid and then take shared lock. Would that cause problems? (I > must > > be missing some subtlety here.) > > [Alin Gabriel Serdean: ] If we just open the file in write mode and write > > the pid and then take the shared lock, someone else could write in the file > > between the moment we opened it and gained the shared lock. > > > > Thanks, I understand it now. (I had to write a small program to get it.) I > did see though that a daemon will hang if it tries to take a exclusive lock > without LOCKFILE_FAIL_IMMEDIATELY. > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
> On Jul 14, 2016, at 11:19 PM, Paul Boca <pboca@cloudbasesolutions.com> wrote: > > Hi Guru! > > I will re-spin the patch with LOCKFILE_FAIL_IMMEDIATELY flag set to avoid > the hang of other daemons. There were a few other comments too. Please have a look at them too. > > Thanks, > Paul > >> -----Original Message----- >> From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Guru Shetty >> Sent: Friday, July 15, 2016 6:58 AM >> To: Alin Serdean >> Cc: dev@openvswitch.org >> Subject: Re: [ovs-dev] [PATCH V7] windows: Added lockf function and lock >> PID file >> >> On 14 July 2016 at 20:03, Alin Serdean <aserdean@cloudbasesolutions.com> >> wrote: >> >>> >>> >>> De la: Guru Shetty [mailto:guru@ovn.org] >>> Trimis: Friday, July 15, 2016 5:49 AM >>> Către: Alin Serdean <aserdean@cloudbasesolutions.com> >>> Cc: Paul Boca <pboca@cloudbasesolutions.com>; dev@openvswitch.org >>> Subiect: Re: [ovs-dev] [PATCH V7] windows: Added lockf function and lock >>> PID file >>> >>> >>> >>> On 14 July 2016 at 19:25, Alin Serdean <aserdean@cloudbasesolutions.com >>> <mailto:aserdean@cloudbasesolutions.com>> wrote: >>> >>> [Alin Gabriel Serdean: ] >>> https://msdn.microsoft.com/en- >> us/library/windows/desktop/aa365203(v=vs.85).aspx >>> The part: >>> "If the same range is locked with an exclusive and a shared lock, two >>> unlock operations are necessary to unlock the region; the first unlock >>> operation unlocks the exclusive lock, the second unlock operation unlocks >>> the shared lock." >>> >>> I see. Thank you. (I am blind!). I wonder whether we actually need the >>> exclusive lock then? If what we need is just shared lock, why not just take >>> it in the beginning? Would it have been possible to open the file in write >>> mode if someone had taken the exclusive lock in the first place? >>> >>> [Alin Gabriel Serdean: ] No worries the MS documentation is not that easy >>> to follow or precise in certain scenarios ☺. >>> The problem is “Locking a portion of a file for shared access denies all >>> processes write access to the specified region of the file, including the >>> process that first locks the region. All processes can read the locked >>> region.” >>> “Locking a portion of a file for exclusive access denies all other >>> processes both read and write access to the specified region of the file. >>> Locking a region that goes beyond the current end-of-file position is not >>> an error.” >>> I agree it doesn’t make any sense why a shared lock would block the owner >>> but probably they had reasons to implement it that way. >>> >>> What I was trying to say was, we open the file in write mode. It wouldn't >>> have been successful if someone else had taken a lock (exclusive or >>> shared). Am I correct? If so, why take exclusive lock again to write? Just >>> write the pid and then take shared lock. Would that cause problems? (I >> must >>> be missing some subtlety here.) >>> [Alin Gabriel Serdean: ] If we just open the file in write mode and write >>> the pid and then take the shared lock, someone else could write in the file >>> between the moment we opened it and gained the shared lock. >>> >>> Thanks, I understand it now. (I had to write a small program to get it.) I >> did see though that a daemon will hang if it tries to take a exclusive lock >> without LOCKFILE_FAIL_IMMEDIATELY. >> >> >> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> http://openvswitch.org/mailman/listinfo/dev >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev
diff --git a/lib/daemon-windows.c b/lib/daemon-windows.c index 8cf0fea..e6e1452 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,14 @@ VLOG_DEFINE_THIS_MODULE(daemon_windows); +/* Constants for flock function */ +#define LOCK_SH 0x0 /* Shared lock. */ +#define LOCK_EX LOCKFILE_EXCLUSIVE_LOCK /* Exclusive 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? */ @@ -404,9 +413,35 @@ detach_process(int argc, char *argv[]) } static void +flock(FILE* fd, int operation) +{ + HANDLE hFile; + OVERLAPPED ov = {0}; + + hFile = (HANDLE)_get_osfhandle(fileno(fd)); + if (hFile == INVALID_HANDLE_VALUE) { + VLOG_FATAL("Invalid handle value"); + } + + if (operation & LOCK_UN) { + if (UnlockFileEx(hFile, 0, 0, LOCK_MAX_SIZE, &ov) == 0) { + VLOG_FATAL("Failed to unlock PID file (%s).", + ovs_lasterror_to_string()); + } + } else { + if (LockFileEx(hFile, operation, 0, 0, LOCK_MAX_SIZE, &ov) == FALSE) { + VLOG_FATAL("Failed to lock PID file (%s).", + ovs_lasterror_to_string()); + } + } +} + +static void unlink_pidfile(void) { if (filep_pidfile) { + /* Remove the shared lock on file */ + flock(filep_pidfile, LOCK_UN); fclose(filep_pidfile); } if (pidfile) { @@ -437,6 +472,8 @@ make_pidfile(void) VLOG_FATAL("failed to open %s (%s)", pidfile, ovs_strerror(errno)); } + flock(filep_pidfile, LOCK_EX); + fatal_signal_add_hook(unlink_pidfile, NULL, NULL, true); fprintf(filep_pidfile, "%d\n", _getpid()); @@ -444,6 +481,10 @@ make_pidfile(void) VLOG_FATAL("Failed to write into the pidfile %s", pidfile); } + flock(filep_pidfile, LOCK_SH); + /* This will remove the exclusive lock. The shared lock will remain */ + flock(filep_pidfile, LOCK_UN); + /* 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. V6: flock returns now the error of GetLastError. Also it uses fd parameter instead of the global filep_pidfile. Removed unused local variable and unnecessary error message. V7: Keep a shared lock on PID file after the fopen. This ensures us that at most one process can write the same PID file. fopen with 'w' on Windows creates a file with FILE_SHARED_WRITE flag, so anyone with write access on file can write it. --- lib/daemon-windows.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)