[ovs-dev] [threads 05/11] ovs-thread: Add support for various thread-related assertions.

Ethan Jackson ethan at nicira.com
Fri Jun 28 22:34:26 UTC 2013


Should the multithreaded bool be atomic?  I doubt it matters, but
perhaps if pthread_create()'s implementation is odd, we could have a
problem.

Acked-by: Ethan Jackson <ethan at nicira.com>


On Wed, Jun 19, 2013 at 1:17 PM, Ben Pfaff <blp at nicira.com> wrote:
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/command-line.c    |    4 ++-
>  lib/daemon.c          |   12 +++-----
>  lib/ofp-version-opt.c |    3 ++
>  lib/ovs-thread.c      |   68 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/ovs-thread.h      |   11 ++++++++
>  lib/process.c         |    4 +++
>  lib/timeval.c         |    2 +
>  lib/util.c            |    4 +++
>  8 files changed, 100 insertions(+), 8 deletions(-)
>
> diff --git a/lib/command-line.c b/lib/command-line.c
> index 70b1f4d..7800c0b 100644
> --- a/lib/command-line.c
> +++ b/lib/command-line.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2008, 2009, 2010, 2011 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -19,6 +19,7 @@
>  #include <getopt.h>
>  #include <limits.h>
>  #include <stdlib.h>
> +#include "ovs-thread.h"
>  #include "util.h"
>  #include "vlog.h"
>
> @@ -110,6 +111,7 @@ proctitle_init(int argc, char **argv)
>  {
>      int i;
>
> +    assert_single_threaded();
>      if (!argc || !argv[0]) {
>          /* This situation should never occur, but... */
>          return;
> diff --git a/lib/daemon.c b/lib/daemon.c
> index e12bc14..56b32b8 100644
> --- a/lib/daemon.c
> +++ b/lib/daemon.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -29,6 +29,7 @@
>  #include "fatal-signal.h"
>  #include "dirs.h"
>  #include "lockfile.h"
> +#include "ovs-thread.h"
>  #include "process.h"
>  #include "socket-util.h"
>  #include "timeval.h"
> @@ -88,6 +89,7 @@ make_pidfile_name(const char *name)
>  void
>  set_pidfile(const char *name)
>  {
> +    assert_single_threaded();
>      free(pidfile);
>      pidfile = make_pidfile_name(name);
>  }
> @@ -280,9 +282,7 @@ daemonize(void)
>  pid_t
>  fork_and_clean_up(void)
>  {
> -    pid_t pid;
> -
> -    pid = fork();
> +    pid_t pid = xfork();
>      if (pid > 0) {
>          /* Running in parent process. */
>          fatal_signal_fork();
> @@ -290,10 +290,7 @@ fork_and_clean_up(void)
>          /* Running in child process. */
>          time_postfork();
>          lockfile_postfork();
> -    } else {
> -        VLOG_FATAL("fork failed (%s)", strerror(errno));
>      }
> -
>      return pid;
>  }
>
> @@ -504,6 +501,7 @@ close_standard_fds(void)
>  void
>  daemonize_start(void)
>  {
> +    assert_single_threaded();
>      daemonize_fd = -1;
>
>      if (detach) {
> diff --git a/lib/ofp-version-opt.c b/lib/ofp-version-opt.c
> index 35d79e6..84e83d8 100644
> --- a/lib/ofp-version-opt.c
> +++ b/lib/ofp-version-opt.c
> @@ -1,6 +1,7 @@
>  #include <config.h>
>  #include "ofp-util.h"
>  #include "ofp-version-opt.h"
> +#include "ovs-thread.h"
>  #include "vlog.h"
>  #include "dynamic-string.h"
>
> @@ -17,12 +18,14 @@ get_allowed_ofp_versions(void)
>  void
>  set_allowed_ofp_versions(const char *string)
>  {
> +    assert_single_threaded();
>      allowed_versions = ofputil_versions_from_string(string);
>  }
>
>  void
>  mask_allowed_ofp_versions(uint32_t bitmap)
>  {
> +    assert_single_threaded();
>      allowed_versions &= bitmap;
>  }
>
> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> index d69ec5e..56303f7 100644
> --- a/lib/ovs-thread.c
> +++ b/lib/ovs-thread.c
> @@ -17,7 +17,12 @@
>  #include <config.h>
>  #include "ovs-thread.h"
>  #include <errno.h>
> +#include <poll.h>
> +#include <stdlib.h>
> +#include <unistd.h>
>  #include "compiler.h"
> +#include "poll-loop.h"
> +#include "socket-util.h"
>  #include "util.h"
>
>  #ifdef __CHECKER__
> @@ -26,6 +31,18 @@
>   * cut-and-paste.  Since "sparse" is just a checker, not a compiler, it
>   * doesn't matter that we don't define them. */
>  #else
> +#include "vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(ovs_thread);
> +
> +/* If there is a reason that we cannot fork anymore (unless the fork will be
> + * immediately followed by an exec), then this points to a string that
> + * explains why. */
> +static const char *must_not_fork;
> +
> +/* True if we created any threads beyond the main initial thread. */
> +static bool multithreaded;
> +
>  #define XPTHREAD_FUNC1(FUNCTION, PARAM1)                \
>      void                                                \
>      x##FUNCTION(PARAM1 arg1)                            \
> @@ -100,6 +117,9 @@ xpthread_create(pthread_t *threadp, pthread_attr_t *attr,
>      pthread_t thread;
>      int error;
>
> +    forbid_forking("multiple threads exist");
> +    multithreaded = true;
> +
>      error = pthread_create(threadp ? threadp : &thread, attr, start, arg);
>      if (error) {
>          ovs_abort(error, "pthread_create failed");
> @@ -123,4 +143,52 @@ ovsthread_once_done(struct ovsthread_once *once)
>      atomic_store(&once->done, true);
>      xpthread_mutex_unlock(&once->mutex);
>  }
> +
> +/* Asserts that the process has not yet created any threads (beyond the initial
> + * thread).  */
> +void
> +(assert_single_threaded)(const char *where)
> +{
> +    if (multithreaded) {
> +        VLOG_FATAL("%s: attempted operation not allowed when multithreaded",
> +                   where);
> +    }
> +}
> +
> +/* Forks the current process (checking that this is allowed).  Aborts with
> + * VLOG_FATAL if fork() returns an error, and otherwise returns the value
> + * returned by fork().  */
> +pid_t
> +(xfork)(const char *where)
> +{
> +    pid_t pid;
> +
> +    if (must_not_fork) {
> +        VLOG_FATAL("%s: attempted to fork but forking not allowed (%s)",
> +                   where, must_not_fork);
> +    }
> +
> +    pid = fork();
> +    if (pid < 0) {
> +        VLOG_FATAL("fork failed (%s)", strerror(errno));
> +    }
> +    return pid;
> +}
> +
> +/* Notes that the process must not call fork() from now on, for the specified
> + * 'reason'.  (The process may still fork() if it execs itself immediately
> + * afterward.) */
> +void
> +forbid_forking(const char *reason)
> +{
> +    ovs_assert(reason != NULL);
> +    must_not_fork = reason;
> +}
> +
> +/* Returns true if the process is allowed to fork, false otherwise. */
> +bool
> +may_fork(void)
> +{
> +    return !must_not_fork;
> +}
>  #endif
> diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
> index 4779030..10f8b6b 100644
> --- a/lib/ovs-thread.h
> +++ b/lib/ovs-thread.h
> @@ -18,6 +18,8 @@
>  #define OVS_THREAD_H 1
>
>  #include <pthread.h>
> +#include <stddef.h>
> +#include <sys/types.h>
>  #include "ovs-atomic.h"
>  #include "util.h"
>
> @@ -355,5 +357,14 @@ ovsthread_once_start(struct ovsthread_once *once)
>  #define ovsthread_once_start(ONCE) \
>      ((ONCE)->done ? false : ({ OVS_ACQUIRE(ONCE); true; }))
>  #endif
> +
> +void assert_single_threaded(const char *where);
> +#define assert_single_threaded() assert_single_threaded(SOURCE_LOCATOR)
> +
> +pid_t xfork(const char *where);
> +#define xfork() xfork(SOURCE_LOCATOR)
> +
> +void forbid_forking(const char *reason);
> +bool may_fork(void);
>
>  #endif /* ovs-thread.h */
> diff --git a/lib/process.c b/lib/process.c
> index 03e00ce..266c90b 100644
> --- a/lib/process.c
> +++ b/lib/process.c
> @@ -28,6 +28,7 @@
>  #include "dynamic-string.h"
>  #include "fatal-signal.h"
>  #include "list.h"
> +#include "ovs-thread.h"
>  #include "poll-loop.h"
>  #include "signals.h"
>  #include "socket-util.h"
> @@ -71,6 +72,7 @@ process_init(void)
>      static bool inited;
>      struct sigaction sa;
>
> +    assert_single_threaded();
>      if (inited) {
>          return;
>      }
> @@ -180,6 +182,8 @@ process_start(char **argv, struct process **pp)
>      pid_t pid;
>      int error;
>
> +    assert_single_threaded();
> +
>      *pp = NULL;
>      COVERAGE_INC(process_start);
>      error = process_prestart(argv);
> diff --git a/lib/timeval.c b/lib/timeval.c
> index 5ff59bc..4723c56 100644
> --- a/lib/timeval.c
> +++ b/lib/timeval.c
> @@ -30,6 +30,7 @@
>  #include "fatal-signal.h"
>  #include "hash.h"
>  #include "hmap.h"
> +#include "ovs-thread.h"
>  #include "signals.h"
>  #include "unixctl.h"
>  #include "util.h"
> @@ -297,6 +298,7 @@ time_alarm(unsigned int secs)
>      long long int now;
>      long long int msecs;
>
> +    assert_single_threaded();
>      time_init();
>      time_refresh();
>
> diff --git a/lib/util.c b/lib/util.c
> index cd4019e..2a06461 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -28,6 +28,7 @@
>  #include "byte-order.h"
>  #include "coverage.h"
>  #include "openvswitch/types.h"
> +#include "ovs-thread.h"
>  #include "vlog.h"
>
>  VLOG_DEFINE_THIS_MODULE(util);
> @@ -340,6 +341,9 @@ set_program_name__(const char *argv0, const char *version, const char *date,
>                     const char *time)
>  {
>      const char *slash = strrchr(argv0, '/');
> +
> +    assert_single_threaded();
> +
>      program_name = slash ? slash + 1 : argv0;
>
>      free(program_version);
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
X-CudaMail-Whitelist-To: dev at openvswitch.org



More information about the dev mailing list