On Wed, Nov 14, 2012 at 06:43:50PM -0800, Ethan Jackson wrote:
> make_pidfile() depends on the link() system call to atomically
> create pidfiles when multiple daemons are started concurrently.
> However, this system call isn't available on ESX so an alternative
> strategy is necessary. Fortunately, the approach this patch takes
> is cleaner than the original code.
>
> Signed-off-by: Ethan Jackson <et...@nicira.com>
Here:
> file = fopen(tmpfile, "w+");
> if (!file) {
> VLOG_FATAL("%s: create failed (%s)", tmpfile, strerror(errno));
> }
I think that passing "w+" as the mode to fopen() is now a mistake,
because it will cause us to truncate an existing file to zero length.
That's bad if some other process already opened the file and obtained a
lock on it, because we just erased its pid.
I don't think there's any appropriate standard fopen() mode for what we
want (create if file doesn't exist, don't truncate automatically, do
allow us to truncate later). We might have to switch to open().
Here:
> + if (!overwrite_pidfile) {
> + /* We acquired the lock. Make sure to clean up on exit, and verify
> + * that we're allowed to create the actual pidfile. */
> + fatal_signal_add_file_to_unlink(tmpfile);
> + check_already_running();
> + }
I don't see why we're scheduling tmpfile to get deleted on exit. We're
about to rename it away to pidfile. After we do that, someone else may
come along and newly create a file with that name and lock it on the way
to trying to acquire the pidfile. Deleting it will screw up that
procedure.
> + /* Note that since we locked 'tmpfile', we're the only process allowed to
> + * make this rename, and it's therefore atomic. */
rename is always atomic, so I'm not sure what the statement about
atomicity adds here.
> + if (rename(tmpfile, pidfile) < 0) {
> + VLOG_FATAL("failed to rename \"%s\" to \"%s\" (%s)",
> + tmpfile, pidfile, strerror(errno));
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
