From 4958e3ee121f31909696e0d98930805f45ba9188 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Wed, 7 Nov 2012 10:16:27 -0800 Subject: [PATCH] Makefile.am: add check that is not used from unexpected files. In general, with a few specific exceptions, ovs_assert is now preferred over assert, so this commit adds a check for that in the top-level Makefile. Signed-off-by: Ben Pfaff Acked-by: Ethan Jackson --- Makefile.am | 13 ++++++ configure.ac | 1 + lib/vlog.c | 112 +++++++++++++++++++++++++++++++++++++++++++++------ lib/worker.c | 5 +++ 4 files changed, 118 insertions(+), 13 deletions(-) diff --git a/Makefile.am b/Makefile.am index 878a1a961..328b248e1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -195,6 +195,19 @@ rate-limit-check: fi .PHONY: rate-limit-check +# Check that assert.h is not used outside a whitelist of files. +ALL_LOCAL += check-assert-h-usage +check-assert-h-usage: + @if test -e $(srcdir)/.git && (git --version) >/dev/null 2>&1 && \ + (cd $(srcdir) && git --no-pager grep -l -E '[<]assert.h[>]') | \ + $(EGREP) -v '^lib/(sflow_receiver|vlog|worker).c$$|^tests/'; \ + then \ + echo "Files listed above unexpectedly #include <""assert.h"">."; \ + echo "Please use ovs_assert (from util.h) instead of assert."; \ + exit 1; \ + fi +.PHONY: check-assert-h-usage + if HAVE_GROFF ALL_LOCAL += manpage-check manpage-check: $(man_MANS) $(dist_man_MANS) $(noinst_man_MANS) diff --git a/configure.ac b/configure.ac index 539d5e3aa..bcfb2276d 100644 --- a/configure.ac +++ b/configure.ac @@ -27,6 +27,7 @@ AC_PROG_CPP AC_PROG_RANLIB AC_PROG_MKDIR_P AC_PROG_FGREP +AC_PROG_EGREP AC_ARG_VAR([PERL], [path to Perl interpreter]) AC_PATH_PROG([PERL], perl, no) diff --git a/lib/vlog.c b/lib/vlog.c index 0bd9bd135..7867b070a 100644 --- a/lib/vlog.c +++ b/lib/vlog.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. @@ -28,6 +28,7 @@ #include #include #include +#include "coverage.h" #include "dirs.h" #include "dynamic-string.h" #include "ofpbuf.h" @@ -40,6 +41,13 @@ VLOG_DEFINE_THIS_MODULE(vlog); +COVERAGE_DEFINE(vlog_recursive); + +/* ovs_assert() logs the assertion message, so using ovs_assert() in this + * source file could cause recursion. */ +#undef ovs_assert +#define ovs_assert use_assert_instead_of_ovs_assert_in_this_module + /* Name for each logging level. */ static const char *level_names[VLL_N_LEVELS] = { #define VLOG_LEVEL(NAME, SYSLOG_LEVEL) #NAME, @@ -480,6 +488,55 @@ vlog_unixctl_reopen(struct unixctl_conn *conn, int argc OVS_UNUSED, } } +static void +set_all_rate_limits(bool enable) +{ + struct vlog_module **mp; + + for (mp = vlog_modules; mp < &vlog_modules[n_vlog_modules]; mp++) { + (*mp)->honor_rate_limits = enable; + } +} + +static void +set_rate_limits(struct unixctl_conn *conn, int argc, + const char *argv[], bool enable) +{ + if (argc > 1) { + int i; + + for (i = 1; i < argc; i++) { + if (!strcasecmp(argv[i], "ANY")) { + set_all_rate_limits(enable); + } else { + struct vlog_module *module = vlog_module_from_name(argv[i]); + if (!module) { + unixctl_command_reply_error(conn, "unknown module"); + return; + } + module->honor_rate_limits = enable; + } + } + } else { + set_all_rate_limits(enable); + } + unixctl_command_reply(conn, NULL); +} + +static void +vlog_enable_rate_limit(struct unixctl_conn *conn, int argc, + const char *argv[], void *aux OVS_UNUSED) +{ + set_rate_limits(conn, argc, argv, true); +} + +static void +vlog_disable_rate_limit(struct unixctl_conn *conn, int argc, + const char *argv[], void *aux OVS_UNUSED) +{ + set_rate_limits(conn, argc, argv, false); +} + /* Initializes the logging subsystem and registers its unixctl server * commands. */ void @@ -515,6 +572,10 @@ vlog_init(void) "vlog/set", "{spec | PATTERN:facility:pattern}", 1, INT_MAX, vlog_unixctl_set, NULL); unixctl_command_register("vlog/list", "", 0, 0, vlog_unixctl_list, NULL); + unixctl_command_register("vlog/enable-rate-limit", "[module]...", + 0, INT_MAX, vlog_enable_rate_limit, NULL); + unixctl_command_register("vlog/disable-rate-limit", "[module]...", + 0, INT_MAX, vlog_disable_rate_limit, NULL); unixctl_command_register("vlog/reopen", "", 0, 0, vlog_unixctl_reopen, NULL); } @@ -543,12 +604,20 @@ vlog_get_levels(void) ds_put_format(&s, " ------- ------ ------\n"); for (mp = vlog_modules; mp < &vlog_modules[n_vlog_modules]; mp++) { - line = xasprintf("%-16s %4s %4s %4s\n", - vlog_get_module_name(*mp), - vlog_get_level_name(vlog_get_level(*mp, VLF_CONSOLE)), - vlog_get_level_name(vlog_get_level(*mp, VLF_SYSLOG)), - vlog_get_level_name(vlog_get_level(*mp, VLF_FILE))); - svec_add_nocopy(&lines, line); + struct ds line; + + ds_init(&line); + ds_put_format(&line, "%-16s %4s %4s %4s", + vlog_get_module_name(*mp), + vlog_get_level_name(vlog_get_level(*mp, VLF_CONSOLE)), + vlog_get_level_name(vlog_get_level(*mp, VLF_SYSLOG)), + vlog_get_level_name(vlog_get_level(*mp, VLF_FILE))); + if (!(*mp)->honor_rate_limits) { + ds_put_cstr(&line, " (rate limiting disabled)"); + } + ds_put_char(&line, '\n'); + + svec_add_nocopy(&lines, ds_steal_cstr(&line)); } svec_sort(&lines); @@ -826,6 +895,10 @@ bool vlog_should_drop(const struct vlog_module *module, enum vlog_level level, struct vlog_rate_limit *rl) { + if (!module->honor_rate_limits) { + return false; + } + if (!vlog_is_enabled(module, level)) { return true; } @@ -887,13 +960,26 @@ static void vlog_write_file(struct ds *s) { if (worker_is_running()) { - worker_request(s->string, s->length, - &log_fd, vlog_async_inited ? 0 : 1, - vlog_async_write_request_cb, NULL, NULL); - vlog_async_inited = true; - } else { - ignore(write(log_fd, s->string, s->length)); + static bool in_worker_request = false; + if (!in_worker_request) { + in_worker_request = true; + + worker_request(s->string, s->length, + &log_fd, vlog_async_inited ? 0 : 1, + vlog_async_write_request_cb, NULL, NULL); + vlog_async_inited = true; + + in_worker_request = false; + return; + } else { + /* We've been entered recursively. This can happen if + * worker_request(), or a function that it calls, tries to log + * something. We can't call worker_request() recursively, so fall + * back to writing the log file directly. */ + COVERAGE_INC(vlog_recursive); + } } + ignore(write(log_fd, s->string, s->length)); } static void diff --git a/lib/worker.c b/lib/worker.c index b6d268deb..6ca05cdbe 100644 --- a/lib/worker.c +++ b/lib/worker.c @@ -37,6 +37,11 @@ VLOG_DEFINE_THIS_MODULE(worker); +/* ovs_assert() logs the assertion message and logging sometimes goes through a + * worker, so using ovs_assert() in this source file could cause recursion. */ +#undef ovs_assert +#define ovs_assert use_assert_instead_of_ovs_assert_in_this_module + /* Header for an RPC request. */ struct worker_request { size_t request_len; /* Length of the payload in bytes. */ -- 2.43.0