On 13 July 2016 at 10:35, Paul Boca <pb...@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 <pb...@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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
