From 8c7691a9a5548c58f1e362f3e045d90199dd1667 Mon Sep 17 00:00:00 2001
From: Nir Soffer <nirsof@gmail.com>
Date: Fri, 6 Dec 2024 02:23:51 +0200
Subject: [PATCH] Improve pidfile management

We used to create an empty pidfile, and write the pid only after
starting the interface. Having pidfile without a pid does not make
sense, as other processes cannot use the pid to terminate the process.
For example if starting the interface is blocking for long time due to a
bug, we are left with unhelpful pid file.

To keep the main flow more clear, add a helper to create and remove the
pid file. The helpers check all errors and log better log messages using
ERRORF instead of ERRORN, including the pidfile path.

Previously if we got a short write to the pidfile, we log bogus error
since errno is set only when write return negative return value.

Rename pid_fd to pidfile_fd to avoid confusion with Linux's pidfd
https://man7.org/linux/man-pages/man2/pidfd_open.2.html.

Signed-off-by: Nir Soffer <nirsof@gmail.com>
---
 main.c | 60 ++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 18 deletions(-)

diff --git a/main.c b/main.c
index 95cddce..0edbed7 100644
--- a/main.c
+++ b/main.c
@@ -364,6 +364,40 @@ static int socket_bindlisten(const char *socket_path,
   return -1;
 }
 
+static void remove_pidfile(const char *pidfile)
+{
+  if (unlink(pidfile) != 0) {
+    ERRORF("Failed to remove pidfile: \"%s\": %s", pidfile, strerror(errno));
+  }
+}
+
+static int create_pidfile(const char *pidfile)
+{
+  int flags = O_WRONLY | O_CREAT | O_EXLOCK | O_TRUNC | O_NONBLOCK;
+  int fd = open(pidfile, flags, 0644);
+  if (fd == -1) {
+    ERRORF("Failed to open pidfile: \"%s\": %s", pidfile, strerror(errno));
+    return -1;
+  }
+
+  char pid[20];
+  snprintf(pid, sizeof(pid), "%u", getpid());
+  ssize_t n = write(fd, pid, strlen(pid));
+  if (n != (ssize_t)strlen(pid)) {
+    if (n < 0) {
+      ERRORF("Failed to write pidfile: \"%s\": %s", pidfile, strerror(errno));
+    } else {
+      // Should never happen, but if it does errno is not set.
+      ERRORF("Short write to pidfile: \"%s\"", pidfile);
+    }
+    remove_pidfile(pidfile);
+    close(fd);
+    return -1;
+  }
+
+  return fd;
+}
+
 static void on_accept(struct state *state, int accept_fd, interface_ref iface);
 
 int main(int argc, char *argv[]) {
@@ -395,15 +429,14 @@ int main(int argc, char *argv[]) {
   // We will receive EPIPE on the socket.
   signal(SIGPIPE, SIG_IGN);
 
-  int pid_fd = -1;
+  int pidfile_fd = -1;
   if (cliopt->pidfile != NULL) {
-    pid_fd = open(cliopt->pidfile,
-                  O_WRONLY | O_CREAT | O_EXLOCK | O_TRUNC | O_NONBLOCK, 0644);
-    if (pid_fd == -1) {
-      ERRORN("pidfile_open");
-      goto done;
+    pidfile_fd = create_pidfile(cliopt->pidfile);
+    if (pidfile_fd == -1) {
+      goto done; // error already logged.
     }
   }
+
   DEBUGF("Opening socket \"%s\" (for UNIX group \"%s\")", cliopt->socket_path,
          cliopt->socket_group);
   listen_fd = socket_bindlisten(cliopt->socket_path, cliopt->socket_group);
@@ -428,15 +461,6 @@ int main(int argc, char *argv[]) {
     goto done;
   }
 
-  if (pid_fd != -1) {
-    char pid[20];
-    snprintf(pid, sizeof(pid), "%u", getpid());
-    if (write(pid_fd, pid, strlen(pid)) != (ssize_t)strlen(pid)) {
-      ERRORN("write");
-      goto done;
-    }
-  }
-
   while (1) {
     int accept_fd = accept(listen_fd, NULL, NULL);
     if (accept_fd < 0) {
@@ -457,9 +481,9 @@ int main(int argc, char *argv[]) {
   if (listen_fd != -1) {
     close(listen_fd);
   }
-  if (pid_fd != -1) {
-    unlink(cliopt->pidfile);
-    close(pid_fd);
+  if (pidfile_fd != -1) {
+    remove_pidfile(cliopt->pidfile);
+    close(pidfile_fd);
   }
   if (state.vms_queue != NULL)
     dispatch_release(state.vms_queue);