From 574424dc5a5e1460f27bf55fd5d605f485e7ac9d Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Sun, 25 Dec 2016 20:53:59 -0800 Subject: [PATCH] fix uvar tests I noticed that universal variable tests were failing on Cygwin and Dragonfly BSD. The failures were because we are attempting to verify the correct behavior of mechanisms that are known to be broken on those platforms. There are still uvar test failures on those platforms with this change but they are due to actual problems rather than bugs in the tests. Fixes #3587 --- src/env_universal_common.cpp | 119 ++++++++++++----------------------- src/env_universal_common.h | 18 ++---- src/fish_tests.cpp | 43 ++++--------- 3 files changed, 57 insertions(+), 123 deletions(-) diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 9d99321a2..eeefa1e3c 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -1,28 +1,30 @@ // The utility library for universal variables. Used both by the client library and by the daemon. -#include "config.h" +#include "config.h" // IWYU pragma: keep #include // IWYU pragma: keep #include #include #include #include -#include +#include // IWYU pragma: keep #include #include #include +#ifdef __CYGWIN__ #include +#endif +#ifdef HAVE_SYS_SELECT_H +#include // IWYU pragma: keep +#endif #include #include -#include +#include // IWYU pragma: keep #include // We need the sys/file.h for the flock() declaration on Linux but not OS X. #include // IWYU pragma: keep // We need the ioctl.h header so we can check if SIOCGIFHWADDR is defined by it so we know if we're // on a Linux system. #include // IWYU pragma: keep -#ifdef HAVE_SYS_SELECT_H -#include -#endif #include #include #include @@ -35,7 +37,7 @@ #include "fallback.h" // IWYU pragma: keep #include "path.h" #include "utf8.h" -#include "util.h" +#include "util.h" // IWYU pragma: keep #include "wutil.h" #if __APPLE__ @@ -950,8 +952,8 @@ wcstring get_machine_identifier() { return result; } -#ifdef HAVE_SHM_OPEN class universal_notifier_shmem_poller_t : public universal_notifier_t { +#ifdef __CYGWIN__ // This is what our shared memory looks like. Everything here is stored in network byte order // (big-endian). struct universal_notifier_shmem_t { @@ -1089,17 +1091,22 @@ class universal_notifier_shmem_poller_t : public universal_notifier_t { } return usec_per_sec / 3; // 3 times a second } -}; +#else // this class isn't valid on this system + public: + universal_notifier_shmem_poller_t() { + DIE("universal_notifier_shmem_poller_t cannot be used on this system"); + } #endif +}; /// A notifyd-based notifier. Very straightforward. class universal_notifier_notifyd_t : public universal_notifier_t { +#if FISH_NOTIFYD_AVAILABLE int notify_fd; int token; std::string name; void setup_notifyd() { -#if FISH_NOTIFYD_AVAILABLE // Per notify(3), the user.uid.%d style is only accessible to processes with that uid. char local_name[256]; snprintf(local_name, sizeof local_name, "user.uid.%d.%ls.uvars", getuid(), @@ -1128,7 +1135,6 @@ class universal_notifier_notifyd_t : public universal_notifier_t { // marked as CLO_EXEC, that's probably a good thing. set_cloexec(this->notify_fd + 1); } -#endif } public: @@ -1138,9 +1144,7 @@ class universal_notifier_notifyd_t : public universal_notifier_t { ~universal_notifier_notifyd_t() { if (token != -1 /* NOTIFY_TOKEN_INVALID */) { -#if FISH_NOTIFYD_AVAILABLE notify_cancel(token); -#endif } } @@ -1161,16 +1165,18 @@ class universal_notifier_notifyd_t : public universal_notifier_t { } void post_notification() { -#if FISH_NOTIFYD_AVAILABLE uint32_t status = notify_post(name.c_str()); if (status != NOTIFY_STATUS_OK) { - fprintf(stderr, - "Warning: notify_post() failed with status %u. Universal variable " - "notifications may not be sent.", - status); + debug(1, "notify_post() failed with status %u. Uvar notifications may not be sent.", + status); } -#endif } +#else // this class isn't valid on this system + public: + universal_notifier_notifyd_t() { + DIE("universal_notifier_notifyd_t cannot be used on this system"); + } +#endif }; #define NAMED_PIPE_FLASH_DURATION_USEC (1000000 / 10) @@ -1186,6 +1192,7 @@ class universal_notifier_notifyd_t : public universal_notifier_t { // when there is data remaining in the pipe, if the pipe is kept readable too long, clients will // attempt to read data out of it (to render it no longer readable). class universal_notifier_named_pipe_t : public universal_notifier_t { +#if !defined(__APPLE__) && !defined(__CYGWIN__) int pipe_fd; long long readback_time_usec; size_t readback_amount; @@ -1206,7 +1213,7 @@ class universal_notifier_named_pipe_t : public universal_notifier_t { } public: - explicit universal_notifier_named_pipe_t(const wchar_t *test_path) + universal_notifier_named_pipe_t(const wchar_t *test_path) : pipe_fd(-1), readback_time_usec(0), readback_amount(0), @@ -1332,54 +1339,15 @@ class universal_notifier_named_pipe_t : public universal_notifier_t { } return true; } +#else // this class isn't valid on this system + public: + universal_notifier_named_pipe_t(const wchar_t *test_path) { + DIE("universal_notifier_named_pipe_t cannot be used on this system"); + } +#endif }; -class universal_notifier_null_t : public universal_notifier_t {}; // does nothing - -static universal_notifier_t::notifier_strategy_t fetch_default_strategy_from_environment() { - universal_notifier_t::notifier_strategy_t result = universal_notifier_t::strategy_default; - - const struct { - const char *name; - universal_notifier_t::notifier_strategy_t strat; - } options[] = {{"default", universal_notifier_t::strategy_default}, -#ifdef HAVE_SHM_OPEN - {"shmem", universal_notifier_t::strategy_shmem_polling}, -#endif - {"pipe", universal_notifier_t::strategy_named_pipe}, - {"notifyd", universal_notifier_t::strategy_notifyd}}; - const size_t opt_count = sizeof options / sizeof *options; - - const char *var = getenv(UNIVERSAL_NOTIFIER_ENV_NAME); - if (var == NULL || var[0] == '\0') { - return result; - } - - size_t i; - for (i = 0; i < opt_count; i++) { - if (!strcmp(var, options[i].name)) { - result = options[i].strat; - break; - } - } - if (i >= opt_count) { - fprintf(stderr, "Warning: unrecognized value for %s: '%s'\n", UNIVERSAL_NOTIFIER_ENV_NAME, - var); - fprintf(stderr, "Warning: valid values are "); - for (size_t j = 0; j < opt_count; j++) { - fprintf(stderr, "%s%s", j > 0 ? ", " : "", options[j].name); - } - fputc('\n', stderr); - } - return result; -} - universal_notifier_t::notifier_strategy_t universal_notifier_t::resolve_default_strategy() { - static universal_notifier_t::notifier_strategy_t s_explicit_strategy = - fetch_default_strategy_from_environment(); - if (s_explicit_strategy != strategy_default) { - return s_explicit_strategy; - } #if FISH_NOTIFYD_AVAILABLE return strategy_notifyd; #elif defined(__CYGWIN__) @@ -1390,32 +1358,25 @@ universal_notifier_t::notifier_strategy_t universal_notifier_t::resolve_default_ } universal_notifier_t &universal_notifier_t::default_notifier() { - static universal_notifier_t *result = new_notifier_for_strategy(strategy_default); + static universal_notifier_t *result = + new_notifier_for_strategy(universal_notifier_t::resolve_default_strategy()); return *result; } universal_notifier_t *universal_notifier_t::new_notifier_for_strategy( universal_notifier_t::notifier_strategy_t strat, const wchar_t *test_path) { - if (strat == strategy_default) { - strat = resolve_default_strategy(); //!OCLINT(parameter reassignment) - } switch (strat) { -#ifdef HAVE_SHM_OPEN - case strategy_shmem_polling: { - return new universal_notifier_shmem_poller_t(); - } -#endif case strategy_notifyd: { return new universal_notifier_notifyd_t(); } + case strategy_shmem_polling: { + return new universal_notifier_shmem_poller_t(); + } case strategy_named_pipe: { return new universal_notifier_named_pipe_t(test_path); } - case strategy_null: { - return new universal_notifier_null_t(); - } default: { - fprintf(stderr, "Unsupported strategy %d\n", strat); + debug(0, "Unsupported universal notifier strategy %d\n", strat); return NULL; } } @@ -1439,6 +1400,7 @@ bool universal_notifier_t::notification_fd_became_readable(int fd) { return false; } +#if !defined(__APPLE__) && !defined(__CYGWIN__) void universal_notifier_named_pipe_t::make_pipe(const wchar_t *test_path) { wcstring vars_path = test_path ? wcstring(test_path) : default_named_pipe_path(); vars_path.append(L".notifier"); @@ -1464,3 +1426,4 @@ void universal_notifier_named_pipe_t::make_pipe(const wchar_t *test_path) { pipe_fd = fd; } +#endif diff --git a/src/env_universal_common.h b/src/env_universal_common.h index d574b6132..c25c7e2e6 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -1,6 +1,6 @@ #ifndef FISH_ENV_UNIVERSAL_COMMON_H #define FISH_ENV_UNIVERSAL_COMMON_H -#include "config.h" +#include "config.h" // IWYU pragma: keep #include #include @@ -113,24 +113,14 @@ class env_universal_t { class universal_notifier_t { public: enum notifier_strategy_t { - // Default meta-strategy to use the 'best' notifier for the system. - strategy_default, - -#ifdef HAVE_SHM_OPEN // Use a value in shared memory. Simple, but requires polling and therefore semi-frequent // wakeups. strategy_shmem_polling, -#endif - + // Strategy that uses notify(3). Simple and efficient, but OS X/macOS only. + strategy_notifyd, // Strategy that uses a named pipe. Somewhat complex, but portable and doesn't require // polling most of the time. strategy_named_pipe, - - // Strategy that uses notify(3). Simple and efficient, but OS X only. - strategy_notifyd, - - // Null notifier, does nothing. - strategy_null }; protected: @@ -140,9 +130,9 @@ class universal_notifier_t { // No copying. universal_notifier_t &operator=(const universal_notifier_t &); universal_notifier_t(const universal_notifier_t &x); - static notifier_strategy_t resolve_default_strategy(); public: + static notifier_strategy_t resolve_default_strategy(); virtual ~universal_notifier_t(); // Factory constructor. Free with delete. diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index ac9616eb7..f19a6783f 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -25,11 +25,8 @@ #include #include #include -#include -#include #include #include -#include #include #include @@ -1648,7 +1645,7 @@ struct pager_layout_testcase_t { wcstring text = sd.line(0).to_string(); if (text != expected) { - fprintf(stderr, "width %lu got <%ls>, expected <%ls>\n", this->width, text.c_str(), + fprintf(stderr, "width %zu got <%ls>, expected <%ls>\n", this->width, text.c_str(), expected.c_str()); } do_test(text == expected); @@ -2583,37 +2580,24 @@ bool poll_notifier(universal_notifier_t *note) { return result; } -static void trigger_or_wait_for_notification(universal_notifier_t *notifier, - universal_notifier_t::notifier_strategy_t strategy) { - // This argument should be removed if it isn't actually needed by these unit tests. I'm going to - // silence the warning. See https://github.com/fish-shell/fish-shell/issues/3439. - UNUSED(notifier); - +static void trigger_or_wait_for_notification(universal_notifier_t::notifier_strategy_t strategy) { switch (strategy) { - case universal_notifier_t::strategy_default: { - DIE("strategy_default should be passed"); - break; - } -#ifdef HAVE_SHM_OPEN case universal_notifier_t::strategy_shmem_polling: { break; // nothing required } -#endif case universal_notifier_t::strategy_notifyd: { // notifyd requires a round trip to the notifyd server, which means we have to wait a - // little bit to receive it. In practice, this seems to be enough. - usleep(1000000 / 25); + // little bit to receive it. In practice 40 ms seems to be enough. + usleep(40000); break; } - case universal_notifier_t::strategy_named_pipe: - case universal_notifier_t::strategy_null: { - break; + case universal_notifier_t::strategy_named_pipe: { + break; // nothing required } } } static void test_notifiers_with_strategy(universal_notifier_t::notifier_strategy_t strategy) { - assert(strategy != universal_notifier_t::strategy_default); say(L"Testing universal notifiers with strategy %d", (int)strategy); universal_notifier_t *notifiers[16]; size_t notifier_count = sizeof notifiers / sizeof *notifiers; @@ -2636,7 +2620,7 @@ static void test_notifiers_with_strategy(universal_notifier_t::notifier_strategy notifiers[post_idx]->post_notification(); // Do special stuff to "trigger" a notification for testing. - trigger_or_wait_for_notification(notifiers[post_idx], strategy); + trigger_or_wait_for_notification(strategy); for (size_t i = 0; i < notifier_count; i++) { // We aren't concerned with the one who posted. Poll from it (to drain it), and then @@ -2680,15 +2664,12 @@ static void test_notifiers_with_strategy(universal_notifier_t::notifier_strategy } static void test_universal_notifiers() { - if (system("mkdir -p /tmp/fish_uvars_test/ && touch /tmp/fish_uvars_test/varsfile.txt")) + if (system("mkdir -p /tmp/fish_uvars_test/ && touch /tmp/fish_uvars_test/varsfile.txt")) { err(L"mkdir failed"); -#ifdef HAVE_SHM_OPEN - test_notifiers_with_strategy(universal_notifier_t::strategy_shmem_polling); -#endif - test_notifiers_with_strategy(universal_notifier_t::strategy_named_pipe); -#if __APPLE__ - test_notifiers_with_strategy(universal_notifier_t::strategy_notifyd); -#endif + } + + auto strategy = universal_notifier_t::resolve_default_strategy(); + test_notifiers_with_strategy(strategy); if (system("rm -Rf /tmp/fish_uvars_test/")) err(L"rm failed"); }