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 <et...@nicira.com>
On Wed, Jun 19, 2013 at 1:17 PM, Ben Pfaff <b...@nicira.com> wrote:
> Signed-off-by: Ben Pfaff <b...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
X-CudaMail-Whitelist-To: dev@openvswitch.org
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
