[ovs-dev] [daemon 09/10] daemon: Integrate checking for an existing pidfile into daemonize_start().

Ethan Jackson ethan at nicira.com
Fri Apr 1 23:42:32 UTC 2011


Looks Good.

On Fri, Apr 1, 2011 at 8:38 AM, Justin Petbot <jpetbot at gmail.com> wrote:
> On Thu, 31 Mar 2011, at 4:31:32 PM, Ben Pfaff wrote:
>> Until now, it has been the responsibility of an individual daemon to call
>> die_if_already_running() at an appropriate time.  A long time ago, this
>> had to happen *before* daemonizing, because once the process daemonized
>> itself there was no way to report failure to the process that originally
>> started the daemon.  With the introduction of daemonize_start(), this is
>> now possible, but we haven't been taking advantage of it.
>>
>> Therefore, this commit integrates the die_if_already_running() call into
>> daemonize_start() and deletes the calls to it from individual daemons.
>> ---
>>  debian/ovs-monitor-ipsec                           |    4 +---
>>  lib/daemon.c                                       |    9 +++++----
>>  lib/daemon.h                                       |    1 -
>>  ovsdb/ovsdb-server.c                               |    1 -
>>  python/ovs/daemon.py                               |    9 +++++----
>>  tests/test-daemon.py                               |    3 +--
>>  tests/test-jsonrpc.c                               |    2 --
>>  tests/test-jsonrpc.py                              |    4 +---
>>  utilities/ovs-controller.c                         |    1 -
>>  utilities/ovs-openflowd.c                          |    1 -
>>  vswitchd/ovs-brcompatd.c                           |    1 -
>>  vswitchd/ovs-vswitchd.c                            |    1 -
>>  .../usr_share_openvswitch_scripts_ovs-xapi-sync    |    2 --
>>  13 files changed, 13 insertions(+), 26 deletions(-)
>>
>> diff --git a/debian/ovs-monitor-ipsec b/debian/ovs-monitor-ipsec
>> index 17f3997..febd569 100755
>> --- a/debian/ovs-monitor-ipsec
>> +++ b/debian/ovs-monitor-ipsec
>> @@ -1,5 +1,5 @@
>>  #!/usr/bin/python
>> -# Copyright (c) 2009, 2010 Nicira Networks
>> +# Copyright (c) 2009, 2010, 2011 Nicira Networks
>>  #
>>  # Licensed under the Apache License, Version 2.0 (the "License");
>>  # you may not use this file except in compliance with the License.
>> @@ -450,8 +450,6 @@ def main(argv):
>>                           "(use --help for help)\n" % ovs.util.PROGRAM_NAME)
>>          sys.exit(1)
>>
>> -    ovs.daemon.die_if_already_running()
>> -
>>      remote = args[0]
>
> Do you think it's worth it to correct the man page for ovs-dpctl?
>
>>      idl = ovs.db.idl.Idl(remote, "Open_vSwitch", monitor_uuid_schema_cb)
>>
>> diff --git a/lib/daemon.c b/lib/daemon.c
>> index 173dabe..a9cada8 100644
>> --- a/lib/daemon.c
>> +++ b/lib/daemon.c
>> @@ -107,9 +107,9 @@ is_chdir_enabled(void)
>>      return chdir_;
>>  }
>>
>> -/* Normally, die_if_already_running() will terminate the program with a message
>> - * if a locked pidfile already exists.  If this function is called,
>> - * die_if_already_running() will merely log a warning. */
>> +/* Normally, daemonize() or damonize_start() will terminate the program with a
>> + * message if a locked pidfile already exists.  If this function is called, an
>> + * existing pidfile will be replaced, with a warning. */
>
> I'm not sure how much of a problem this is, but  you should change this
> variable to !pidfile.
>
>>  void
>>  ignore_existing_pidfile(void)
>>  {
>> @@ -141,7 +141,7 @@ daemon_set_monitor(void)
>>
>>  /* If a locked pidfile exists, issue a warning message and, unless
>>   * ignore_existing_pidfile() has been called, terminate the program. */
>> -void
>> +static void
>>  die_if_already_running(void)
>>  {
>>      pid_t pid;
>> @@ -449,6 +449,7 @@ daemonize_start(void)
>>          /* Running in daemon process. */
>>      }
>>
>> +    die_if_already_running();
>>      make_pidfile();
>>
>>      /* Make sure that the unixctl commands for vlog get registered in a
>> diff --git a/lib/daemon.h b/lib/daemon.h
>> index 8f4fe39..4af98f6 100644
>> --- a/lib/daemon.h
>> +++ b/lib/daemon.h
>> @@ -67,7 +67,6 @@ void daemon_set_monitor(void);
>>  void daemonize(void);
>>  void daemonize_start(void);
>>  void daemonize_complete(void);
>> -void die_if_already_running(void);
>>  void ignore_existing_pidfile(void);
>>  void daemon_usage(void);
>>  pid_t read_pidfile(const char *name);
>> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
>> index 8d44d84..17ccb6e 100644
>> --- a/ovsdb/ovsdb-server.c
>> +++ b/ovsdb/ovsdb-server.c
>> @@ -101,7 +101,6 @@ main(int argc, char *argv[])
>>      parse_options(argc, argv, &file_name, &remotes, &unixctl_path,
>>                    &run_command);
>>
>> -    die_if_already_running();
>>      daemonize_start();
>>
>>      error = ovsdb_file_open(file_name, false, &db, &file);
>> diff --git a/python/ovs/daemon.py b/python/ovs/daemon.py
>> index 4df2371..cfb4178 100644
>> --- a/python/ovs/daemon.py
>> +++ b/python/ovs/daemon.py
>> @@ -89,9 +89,9 @@ def is_chdir_enabled():
>>      return _chdir
>>
>>  def ignore_existing_pidfile():
>> -    """Normally, die_if_already_running() will terminate the program with a
>
> Did you mean to fix this statement?
>
>> -    message if a locked pidfile already exists.  If this function is called,
>> -    die_if_already_running() will merely log a warning."""
>> +    """Normally, daemonize() or daemonize_start() will terminate the program
>> +    with a message if a locked pidfile already exists.  If this function is
>> +    called, an existing pidfile will be replaced, with a warning."""
>>      global _overwrite_pidfile
>>      _overwrite_pidfile = True
>>
>> @@ -111,7 +111,7 @@ def set_monitor():
>>      global _monitor
>>      _monitor = True
>>
>> -def die_if_already_running():
>> +def _die_if_already_running():
>>      """If a locked pidfile exists, issue a warning message and, unless
>>      ignore_existing_pidfile() has been called, terminate the program."""
>>      if _pidfile is None:
>> @@ -343,6 +343,7 @@ def daemonize_start():
>>              _monitor_daemon(daemon_pid)
>>          # Running in daemon process
>>
>> +    _die_if_already_running()
>>      _make_pidfile()
>>
>>  def daemonize_complete():
>> diff --git a/tests/test-daemon.py b/tests/test-daemon.py
>> index 386445d..586e0ec 100644
>> --- a/tests/test-daemon.py
>> +++ b/tests/test-daemon.py
>> @@ -1,4 +1,4 @@
>> -# Copyright (c) 2010 Nicira Networks.
>> +# Copyright (c) 2010, 2011 Nicira Networks.
>>  #
>>  # Licensed under the Apache License, Version 2.0 (the "License");
>>  # you may not use this file except in compliance with the License.
>> @@ -45,7 +45,6 @@ def main(argv):
>>                               % (ovs.util.PROGRAM_NAME, key))
>>              sys.exit(1)
>>
>> -    ovs.daemon.die_if_already_running()
>>      ovs.daemon.daemonize_start()
>>      if bail:
>>          sys.stderr.write("%s: exiting after daemonize_start() as requested\n"
>> diff --git a/tests/test-jsonrpc.c b/tests/test-jsonrpc.c
>> index 44edda8..12bbc97 100644
>> --- a/tests/test-jsonrpc.c
>> +++ b/tests/test-jsonrpc.c
>> @@ -182,8 +182,6 @@ do_listen(int argc OVS_UNUSED, char *argv[])
>>      bool done;
>>      int error;
>>
>> -    die_if_already_running();
>> -
>>      error = jsonrpc_pstream_open(argv[1], &pstream);
>>      if (error) {
>>          ovs_fatal(error, "could not listen on \"%s\"", argv[1]);
>> diff --git a/tests/test-jsonrpc.py b/tests/test-jsonrpc.py
>> index a8bf553..064457e 100644
>> --- a/tests/test-jsonrpc.py
>> +++ b/tests/test-jsonrpc.py
>> @@ -1,4 +1,4 @@
>> -# Copyright (c) 2009, 2010 Nicira Networks
>> +# Copyright (c) 2009, 2010, 2011 Nicira Networks
>>  #
>>  # Licensed under the Apache License, Version 2.0 (the "License");
>>  # you may not use this file except in compliance with the License.
>> @@ -49,8 +49,6 @@ def handle_rpc(rpc, msg):
>>      return done
>>
>>  def do_listen(name):
>> -    ovs.daemon.die_if_already_running()
>> -
>>      error, pstream = ovs.stream.PassiveStream.open(name)
>>      if error:
>>          sys.stderr.write("could not listen on \"%s\": %s\n"
>> diff --git a/utilities/ovs-controller.c b/utilities/ovs-controller.c
>> index af9e11f..ae0ea3d 100644
>> --- a/utilities/ovs-controller.c
>> +++ b/utilities/ovs-controller.c
>> @@ -139,7 +139,6 @@ main(int argc, char *argv[])
>>          ovs_fatal(0, "no active or passive switch connections");
>>      }
>>
>> -    die_if_already_running();
>>      daemonize_start();
>>
>>      retval = unixctl_server_create(unixctl_path, &unixctl);
>> diff --git a/utilities/ovs-openflowd.c b/utilities/ovs-openflowd.c
>> index 1494209..f9d1ee2 100644
>> --- a/utilities/ovs-openflowd.c
>> +++ b/utilities/ovs-openflowd.c
>> @@ -103,7 +103,6 @@ main(int argc, char *argv[])
>>      parse_options(argc, argv, &s);
>>      signal(SIGPIPE, SIG_IGN);
>>
>> -    die_if_already_running();
>>      daemonize_start();
>>
>>      /* Start listening for ovs-appctl requests. */
>> diff --git a/vswitchd/ovs-brcompatd.c b/vswitchd/ovs-brcompatd.c
>> index 7b341aa..75e5009 100644
>> --- a/vswitchd/ovs-brcompatd.c
>> +++ b/vswitchd/ovs-brcompatd.c
>> @@ -1295,7 +1295,6 @@ main(int argc, char *argv[])
>>      process_init();
>>      ovsrec_init();
>>
>> -    die_if_already_running();
>>      daemonize_start();
>>
>>      retval = unixctl_server_create(NULL, &unixctl);
>> diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
>> index bae03dd..b9a2461 100644
>> --- a/vswitchd/ovs-vswitchd.c
>> +++ b/vswitchd/ovs-vswitchd.c
>> @@ -74,7 +74,6 @@ main(int argc, char *argv[])
>>      process_init();
>>      ovsrec_init();
>>
>> -    die_if_already_running();
>>      daemonize_start();
>>
>>      retval = unixctl_server_create(NULL, &unixctl);
>> diff --git a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
>> index 4d82b99..41a0ba8 100755
>> --- a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
>> +++ b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
>> @@ -252,8 +252,6 @@ def main(argv):
>>                           "(use --help for help)\n" % ovs.util.PROGRAM_NAME)
>>          sys.exit(1)
>>
>> -    ovs.daemon.die_if_already_running()
>
> I see you decided to go back in time and address the issue I mentioned
> in the previous commit to look less foolish.  Extremely clever.
>
>> -
>>      remote = args[0]
>>      idl = ovs.db.idl.Idl(remote, "Open_vSwitch", monitor_uuid_schema_cb)
>>
>> --
>> 1.7.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>



More information about the dev mailing list