Moderize universal variable notifiers

Use some C++11 features.

src/env_universal_common.cpp

@@ -1046,7 +1046,7 @@ static wcstring get_machine_identifier() {
     return result;
 }
 
-class universal_notifier_shmem_poller_t : public universal_notifier_t {
+class universal_notifier_shmem_poller_t final : public universal_notifier_t {
 #ifdef __CYGWIN__
     // This is what our shared memory looks like. Everything here is stored in network byte order
     // (big-endian).
@@ -1060,9 +1060,9 @@ class universal_notifier_shmem_poller_t : public universal_notifier_t {
 #define SHMEM_VERSION_CURRENT 1000
 
    private:
-    long long last_change_time;
+    long long last_change_time{0};
-    uint32_t last_seed;
+    uint32_t last_seed{0};
-    volatile universal_notifier_shmem_t *region;
+    volatile universal_notifier_shmem_t *region{nullptr};
 
     void open_shmem() {
         assert(region == nullptr);
@@ -1072,52 +1072,45 @@ class universal_notifier_shmem_poller_t : public universal_notifier_t {
         snprintf(path, sizeof path, "/%ls_shmem_%d", program_name ? program_name : L"fish",
                  getuid());
 
-        bool errored = false;
         autoclose_fd_t fd{shm_open(path, O_RDWR | O_CREAT, 0600)};
         if (!fd.valid()) {
             const char *error = std::strerror(errno);
             FLOGF(error, _(L"Unable to open shared memory with path '%s': %s"), path, error);
-            errored = true;
+            return;
         }
 
         // Get the size.
         off_t size = 0;
-        if (!errored) {
         struct stat buf = {};
         if (fstat(fd.fd(), &buf) < 0) {
             const char *error = std::strerror(errno);
             FLOGF(error, _(L"Unable to fstat shared memory object with path '%s': %s"), path,
                   error);
-                errored = true;
+            return;
         }
         size = buf.st_size;
-        }
 
         // Set the size, if it's too small.
-        bool set_size = !errored && size < (off_t)sizeof(universal_notifier_shmem_t);
+        if (size < (off_t)sizeof(universal_notifier_shmem_t)) {
-        if (set_size && ftruncate(fd.fd(), sizeof(universal_notifier_shmem_t)) < 0) {
+            if (ftruncate(fd.fd(), sizeof(universal_notifier_shmem_t)) < 0) {
                 const char *error = std::strerror(errno);
                 FLOGF(error, _(L"Unable to truncate shared memory object with path '%s': %s"), path,
                       error);
-            errored = true;
+                return;
+            }
         }
 
         // Memory map the region.
-        if (!errored) {
         void *addr = mmap(nullptr, sizeof(universal_notifier_shmem_t), PROT_READ | PROT_WRITE,
                           MAP_SHARED, fd.fd(), 0);
         if (addr == MAP_FAILED) {
             const char *error = std::strerror(errno);
-                FLOGF(error, _(L"Unable to memory map shared memory object with path '%s': %s"),
+            FLOGF(error, _(L"Unable to memory map shared memory object with path '%s': %s"), path,
-                      path, error);
+                  error);
             this->region = nullptr;
-            } else {
+            return;
+        }
         this->region = static_cast<universal_notifier_shmem_t *>(addr);
-            }
-        }
-
-        // Close the fd, even if the mapping succeeded.
-        fd.close();
 
         // Read the current seed.
         this->poll();
@@ -1129,7 +1122,7 @@ class universal_notifier_shmem_poller_t : public universal_notifier_t {
     // purposes, however, it's useful to keep them in the same process, so we increment the value.
     // This isn't "safe" in the sense that multiple simultaneous increments may result in one being
     // lost, but it should always result in the value being changed, which is sufficient.
-    void post_notification() {
+    void post_notification() override {
         if (region != nullptr) {
             /* Read off the seed */
             uint32_t seed = ntohl(region->universal_variable_seed);  //!OCLINT(constant cond op)
@@ -1147,13 +1140,10 @@ class universal_notifier_shmem_poller_t : public universal_notifier_t {
         }
     }
 
-    universal_notifier_shmem_poller_t() : last_change_time(0), last_seed(0), region(nullptr) {
+    universal_notifier_shmem_poller_t() { open_shmem(); }
-        open_shmem();
-    }
 
     ~universal_notifier_shmem_poller_t() {
         if (region != nullptr) {
-            // Behold: C++ in all its glory!
             void *address = const_cast<void *>(static_cast<volatile void *>(region));
             if (munmap(address, sizeof(universal_notifier_shmem_t)) < 0) {
                 wperror(L"munmap");
@@ -1161,7 +1151,7 @@ class universal_notifier_shmem_poller_t : public universal_notifier_t {
         }
     }
 
-    bool poll() {
+    bool poll() override {
         bool result = false;
         if (region != nullptr) {
             uint32_t seed = ntohl(region->universal_variable_seed);  //!OCLINT(constant cond op)
@@ -1174,7 +1164,7 @@ class universal_notifier_shmem_poller_t : public universal_notifier_t {
         return result;
     }
 
-    unsigned long usec_delay_between_polls() const {
+    unsigned long usec_delay_between_polls() const override {
         // If it's been less than five seconds since the last change, we poll quickly Otherwise we
         // poll more slowly. Note that a poll is a very cheap shmem read. The bad part about making
         // this high is the process scheduling/wakeups it produces.
@@ -1193,11 +1183,13 @@ class universal_notifier_shmem_poller_t : public universal_notifier_t {
 };
 
 /// A notifyd-based notifier. Very straightforward.
-class universal_notifier_notifyd_t : public universal_notifier_t {
+class universal_notifier_notifyd_t final : public universal_notifier_t {
 #ifdef FISH_NOTIFYD_AVAILABLE
-    int notify_fd;
+    // Note that we should not use autoclose_fd_t, as notify_cancel() takes responsibility for
-    int token;
+    // closing it.
-    std::string name;
+    int notify_fd{-1};
+    int token{-1};  // NOTIFY_TOKEN_INVALID
+    std::string name{};
 
     void setup_notifyd() {
         // Per notify(3), the user.uid.%d style is only accessible to processes with that uid.
@@ -1208,40 +1200,40 @@ class universal_notifier_notifyd_t : public universal_notifier_t {
 
         uint32_t status =
             notify_register_file_descriptor(name.c_str(), &this->notify_fd, 0, &this->token);
+
         if (status != NOTIFY_STATUS_OK) {
             FLOGF(warning, "notify_register_file_descriptor() failed with status %u.", status);
             FLOGF(warning, "Universal variable notifications may not be received.");
         }
-        if (this->notify_fd >= 0) {
+        if (notify_fd >= 0) {
             // Mark us for non-blocking reads, and CLO_EXEC.
-            int flags = fcntl(this->notify_fd, F_GETFL, 0);
+            int flags = fcntl(notify_fd, F_GETFL, 0);
             if (flags >= 0 && !(flags & O_NONBLOCK)) {
-                fcntl(this->notify_fd, F_SETFL, flags | O_NONBLOCK);
+                fcntl(notify_fd, F_SETFL, flags | O_NONBLOCK);
             }
 
-            set_cloexec(this->notify_fd);
+            (void)set_cloexec(notify_fd);
             // Serious hack: notify_fd is likely the read end of a pipe. The other end is owned by
             // libnotify, which does not mark it as CLO_EXEC (it should!). The next fd is probably
             // notify_fd + 1. Do it ourselves. If the implementation changes and some other FD gets
             // marked as CLO_EXEC, that's probably a good thing.
-            set_cloexec(this->notify_fd + 1);
+            (void)set_cloexec(notify_fd + 1);
         }
     }
 
    public:
-    universal_notifier_notifyd_t() : notify_fd(-1), token(-1 /* NOTIFY_TOKEN_INVALID */) {
+    universal_notifier_notifyd_t() { setup_notifyd(); }
-        setup_notifyd();
-    }
 
     ~universal_notifier_notifyd_t() {
         if (token != -1 /* NOTIFY_TOKEN_INVALID */) {
+            // Note this closes notify_fd.
             notify_cancel(token);
         }
     }
 
-    int notification_fd() const { return notify_fd; }
+    int notification_fd() const override { return notify_fd; }
 
-    bool notification_fd_became_readable(int fd) {
+    bool notification_fd_became_readable(int fd) override {
         // notifyd notifications come in as 32 bit values. We don't care about the value. We set
         // ourselves as non-blocking, so just read until we can't read any more.
         assert(fd == notify_fd);
@@ -1255,7 +1247,7 @@ class universal_notifier_notifyd_t : public universal_notifier_t {
         return read_something;
     }
 
-    void post_notification() {
+    void post_notification() override {
         uint32_t status = notify_post(name.c_str());
         if (status != NOTIFY_STATUS_OK) {
             FLOGF(warning,
@@ -1271,10 +1263,40 @@ class universal_notifier_notifyd_t : public universal_notifier_t {
 #endif
 };
 
-#if !defined(__APPLE__) && !defined(__CYGWIN__)
+/// Returns a "variables" file in the appropriate runtime directory. This is called infrequently and
-#define NAMED_PIPE_FLASH_DURATION_USEC (1e5)
+/// so does not need to be cached.
-#define SUSTAINED_READABILITY_CLEANUP_DURATION_USEC (5 * 1e6)
+static wcstring default_named_pipe_path() {
-#endif
+    wcstring result = env_get_runtime_path();
+    if (!result.empty()) {
+        result.append(L"/fish_universal_variables");
+    }
+    return result;
+}
+
+/// Create a fifo (named pipe) at \p test_path if non-null, or a default runtime path if null.
+/// Open the fifo for both reading and writing, in non-blocking mode.
+/// \return the fifo, or an invalid fd on failure.
+static autoclose_fd_t make_fifo(const wchar_t *test_path, const wchar_t *suffix) {
+    wcstring vars_path = test_path ? wcstring(test_path) : default_named_pipe_path();
+    vars_path.append(suffix);
+    const std::string narrow_path = wcs2string(vars_path);
+
+    int mkfifo_status = mkfifo(narrow_path.c_str(), 0600);
+    if (mkfifo_status == -1 && errno != EEXIST) {
+        const char *error = std::strerror(errno);
+        const wchar_t *errmsg = _(L"Unable to make a pipe for universal variables using '%ls': %s");
+        FLOGF(error, errmsg, vars_path.c_str(), error);
+        return autoclose_fd_t{};
+    }
+
+    autoclose_fd_t res{wopen_cloexec(vars_path, O_RDWR | O_NONBLOCK, 0600)};
+    if (!res.valid()) {
+        const char *error = std::strerror(errno);
+        const wchar_t *errmsg = _(L"Unable to open a pipe for universal variables using '%ls': %s");
+        FLOGF(error, errmsg, vars_path.c_str(), error);
+    }
+    return res;
+}
 
 // Named-pipe based notifier. All clients open the same named pipe for reading and writing. The
 // pipe's readability status is a trigger to enter polling mode.
@@ -1285,42 +1307,34 @@ class universal_notifier_notifyd_t : public universal_notifier_t {
 // mode until the pipe is no longer readable. To guard against the possibility of a shell exiting
 // 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 {
+class universal_notifier_named_pipe_t final : public universal_notifier_t {
-#if !defined(__APPLE__) && !defined(__CYGWIN__)
+#if !defined(__CYGWIN__)
-    int pipe_fd;
+    autoclose_fd_t pipe_fd;
-    long long readback_time_usec;
+    long long readback_time_usec{0};
-    size_t readback_amount;
+    size_t readback_amount{0};
 
-    bool polling_due_to_readable_fd;
+    // We "flash" the pipe to make it briefly readable, for this many usec.
-    long long drain_if_still_readable_time_usec;
+    static constexpr long long k_flash_duration_usec = 1e5;
 
-    void make_pipe(const wchar_t *test_path);
+    // If the pipe remains readable for this many usec, we drain it.
+    static constexpr long long k_readable_too_long_duration_usec = 5e6;
+
+    bool polling_due_to_readable_fd{false};
+    long long drain_if_still_readable_time_usec{0};
 
     void drain_excessive_data() const {
         // The pipe seems to have data on it, that won't go away. Read a big chunk out of it. We
         // don't read until it's exhausted, because if someone were to pipe say /dev/null, that
         // would cause us to hang!
-        size_t read_amt = 64 * 1024;
+        char buff[512];
-        void *buff = malloc(read_amt);
+        ignore_result(read(pipe_fd.fd(), buff, sizeof buff));
-        ignore_result(read(this->pipe_fd, buff, read_amt));
-        free(buff);
     }
 
    public:
     explicit universal_notifier_named_pipe_t(const wchar_t *test_path)
-        : pipe_fd(-1),
+        : pipe_fd(make_fifo(test_path, L".notifier")) {}
-          readback_time_usec(0),
-          readback_amount(0),
-          polling_due_to_readable_fd(false),
-          drain_if_still_readable_time_usec(0) {
-        make_pipe(test_path);
-    }
 
-    ~universal_notifier_named_pipe_t() override {
+    ~universal_notifier_named_pipe_t() override = default;
-        if (pipe_fd >= 0) {
-            close(pipe_fd);
-        }
-    }
 
     int notification_fd() const override {
         if (polling_due_to_readable_fd) {
@@ -1329,7 +1343,7 @@ class universal_notifier_named_pipe_t : public universal_notifier_t {
             return -1;
         }
         // We are not in polling mode. Return the fd so it can be watched.
-        return pipe_fd;
+        return pipe_fd.fd();
     }
 
     bool notification_fd_became_readable(int fd) override {
@@ -1342,28 +1356,26 @@ class universal_notifier_named_pipe_t : public universal_notifier_t {
         bool should_sync = false;
         if (readback_time_usec == 0) {
             polling_due_to_readable_fd = true;
-            drain_if_still_readable_time_usec =
+            drain_if_still_readable_time_usec = get_time() + k_readable_too_long_duration_usec;
-                get_time() + SUSTAINED_READABILITY_CLEANUP_DURATION_USEC;
             should_sync = true;
         }
         return should_sync;
     }
 
     void post_notification() override {
-        if (pipe_fd >= 0) {
+        if (!pipe_fd.valid()) return;
         // We need to write some data (any data) to the pipe, then wait for a while, then read
         // it back. Nobody is expected to read it except us.
-            int pid_nbo = htonl(getpid());  //!OCLINT(constant cond op)
+        char c[1] = {'\0'};
-            ssize_t amt_written = write(this->pipe_fd, &pid_nbo, sizeof pid_nbo);
+        ssize_t amt_written = write(pipe_fd.fd(), c, sizeof c);
         if (amt_written < 0 && (errno == EWOULDBLOCK || errno == EAGAIN)) {
             // Very unsual: the pipe is full!
             drain_excessive_data();
         }
 
         // Now schedule a read for some time in the future.
-            this->readback_time_usec = get_time() + NAMED_PIPE_FLASH_DURATION_USEC;
+        this->readback_time_usec = get_time() + k_flash_duration_usec;
-            this->readback_amount += sizeof pid_nbo;
+        this->readback_amount += sizeof c;
-        }
     }
 
     unsigned long usec_delay_between_polls() const override {
@@ -1382,7 +1394,7 @@ class universal_notifier_named_pipe_t : public universal_notifier_t {
         unsigned long polling_delay = ULONG_MAX;
         if (polling_due_to_readable_fd) {
             // We're in polling mode. Don't return a value less than our polling interval.
-            polling_delay = NAMED_PIPE_FLASH_DURATION_USEC;
+            polling_delay = k_flash_duration_usec;
         }
 
         // Now return the smaller of the two values. If we get ULONG_MAX, it means there's no more
@@ -1395,13 +1407,15 @@ class universal_notifier_named_pipe_t : public universal_notifier_t {
     }
 
     bool poll() override {
+        if (!pipe_fd.valid()) return false;
+
         // Check if we are past the readback time.
         if (this->readback_time_usec > 0 && get_time() >= this->readback_time_usec) {
             // Read back what we wrote. We do nothing with the value.
             while (this->readback_amount > 0) {
                 char buff[64];
                 size_t amt_to_read = std::min(this->readback_amount, sizeof(buff));
-                ignore_result(read(this->pipe_fd, buff, amt_to_read));
+                ignore_result(read(this->pipe_fd.fd(), buff, amt_to_read));
                 this->readback_amount -= amt_to_read;
             }
             assert(this->readback_amount == 0);
@@ -1409,7 +1423,7 @@ class universal_notifier_named_pipe_t : public universal_notifier_t {
         }
 
         // Check to see if we are doing readability polling.
-        if (!polling_due_to_readable_fd || pipe_fd < 0) {
+        if (!polling_due_to_readable_fd) {
             return false;
         }
 
@@ -1417,10 +1431,10 @@ class universal_notifier_named_pipe_t : public universal_notifier_t {
         // See if this is still readable.
         fd_set fds;
         FD_ZERO(&fds);
-        FD_SET(this->pipe_fd, &fds);
+        FD_SET(pipe_fd.fd(), &fds);
         struct timeval timeout = {};
-        select(this->pipe_fd + 1, &fds, nullptr, nullptr, &timeout);
+        select(pipe_fd.fd() + 1, &fds, nullptr, nullptr, &timeout);
-        if (!FD_ISSET(this->pipe_fd, &fds)) {
+        if (!FD_ISSET(pipe_fd.fd(), &fds)) {
             // No longer readable, no longer polling.
             polling_due_to_readable_fd = false;
             drain_if_still_readable_time_usec = 0;
@@ -1482,51 +1496,13 @@ universal_notifier_t::~universal_notifier_t() = default;
 
 int universal_notifier_t::notification_fd() const { return -1; }
 
-void universal_notifier_t::post_notification() {}
-
 bool universal_notifier_t::poll() { return false; }
 
+void universal_notifier_t::post_notification() {}
+
 unsigned long universal_notifier_t::usec_delay_between_polls() const { return 0; }
 
 bool universal_notifier_t::notification_fd_became_readable(int fd) {
     UNUSED(fd);
     return false;
 }
-
-#if !defined(__APPLE__) && !defined(__CYGWIN__)
-/// Returns a "variables" file in the appropriate runtime directory. This is called infrequently and
-/// so does not need to be cached.
-static wcstring default_named_pipe_path() {
-    wcstring result = env_get_runtime_path();
-    if (!result.empty()) {
-        result.append(L"/fish_universal_variables");
-    }
-    return result;
-}
-
-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");
-    const std::string narrow_path = wcs2string(vars_path);
-
-    int mkfifo_status = mkfifo(narrow_path.c_str(), 0600);
-    if (mkfifo_status == -1 && errno != EEXIST) {
-        const char *error = std::strerror(errno);
-        const wchar_t *errmsg = _(L"Unable to make a pipe for universal variables using '%ls': %s");
-        FLOGF(error, errmsg, vars_path.c_str(), error);
-        pipe_fd = -1;
-        return;
-    }
-
-    int fd = wopen_cloexec(vars_path, O_RDWR | O_NONBLOCK, 0600);
-    if (fd < 0) {
-        const char *error = std::strerror(errno);
-        const wchar_t *errmsg = _(L"Unable to open a pipe for universal variables using '%ls': %s");
-        FLOGF(error, errmsg, vars_path.c_str(), error);
-        pipe_fd = -1;
-        return;
-    }
-
-    pipe_fd = fd;
-}
-#endif

src/env_universal_common.h

@@ -162,8 +162,10 @@ class universal_notifier_t {
         // Use a value in shared memory. Simple, but requires polling and therefore semi-frequent
         // wakeups.
         strategy_shmem_polling,
+
         // 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,