lib: use fsync() for config writes, plug fd leak

sync() has a HUGE impact on systems that perform actual I/O, i.e. real
servers...

Also, we were leaking a fd on each config write ever since
c5e69a0 "lib/vty: add separate output fd support to VTYs"
(by myself :( ...)

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>

diff --git a/lib/command.c b/lib/command.c
index 510699f91b2eda6fc3a3b7cd75734aa5221e83b0..12add6e4abc9fba735b3630b44be1e864ddf56b5 100644 (file)
--- a/lib/command.c
+++ b/lib/command.c
@@ -1342,9 +1342,9 @@ DEFUN (config_write,
 {
   int idx_type = 1;
   unsigned int i;
-  int fd;
+  int fd, dirfd;
   struct cmd_node *node;
-  char *config_file;
+  char *config_file, *slash;
   char *config_file_tmp = NULL;
   char *config_file_sav = NULL;
   int ret = CMD_WARNING;
@@ -1395,6 +1395,21 @@ DEFUN (config_write,
   /* Get filename. */
   config_file = host.config;
 
+#ifndef O_DIRECTORY
+#define O_DIRECTORY 0
+#endif
+  slash = strrchr (config_file, '/');
+  if (slash)
+    {
+      char *config_dir = XSTRDUP (MTYPE_TMP, config_file);
+      config_dir[slash - config_file] = '\0';
+      dirfd = open(config_dir, O_DIRECTORY | O_RDONLY);
+      XFREE (MTYPE_TMP, config_dir);
+    }
+  else
+    dirfd = open(".", O_DIRECTORY | O_RDONLY);
+  /* if dirfd is invalid, directory sync fails, but we're still OK */
+
   config_file_sav =
     XMALLOC (MTYPE_TMP, strlen (config_file) + strlen (CONF_BACKUP_EXT) + 1);
   strcpy (config_file_sav, config_file);
@@ -1412,6 +1427,12 @@ DEFUN (config_write,
                VTY_NEWLINE);
       goto finished;
     }
+  if (fchmod (fd, CONFIGFILE_MASK) != 0)
+    {
+      vty_out (vty, "Can't chmod configuration file %s: %s (%d).%s",
+        config_file_tmp, safe_strerror(errno), errno, VTY_NEWLINE);
+      goto finished;
+    }
 
   /* Make vty for configuration file. */
   file_vty = vty_new ();
@@ -1446,35 +1467,24 @@ DEFUN (config_write,
                    VTY_NEWLINE);
           goto finished;
         }
-      sync ();
-      if (unlink (config_file) != 0)
-        {
-          vty_out (vty, "Can't unlink configuration file %s.%s", config_file,
-                   VTY_NEWLINE);
-          goto finished;
-        }
+      fsync (dirfd);
     }
-  if (link (config_file_tmp, config_file) != 0)
+  if (rename (config_file_tmp, config_file) != 0)
     {
       vty_out (vty, "Can't save configuration file %s.%s", config_file,
                VTY_NEWLINE);
       goto finished;
     }
-  sync ();
-
-  if (chmod (config_file, CONFIGFILE_MASK) != 0)
-    {
-      vty_out (vty, "Can't chmod configuration file %s: %s (%d).%s",
-        config_file, safe_strerror(errno), errno, VTY_NEWLINE);
-      goto finished;
-    }
+  fsync (dirfd);
 
   vty_out (vty, "Configuration saved to %s%s", config_file,
            VTY_NEWLINE);
   ret = CMD_SUCCESS;
 
 finished:
-  unlink (config_file_tmp);
+  if (ret != CMD_SUCCESS)
+    unlink (config_file_tmp);
+  close (dirfd);
   XFREE (MTYPE_TMP, config_file_tmp);
   XFREE (MTYPE_TMP, config_file_sav);
   return ret;

diff --git a/lib/vty.c b/lib/vty.c
index 2660ca3251f3cad66dfb52f34561cd8387c56e34..0568351989ad2058b32c29b40ac9a02f688eeb4a 100644 (file)
--- a/lib/vty.c
+++ b/lib/vty.c
@@ -2254,9 +2254,16 @@ vty_close (struct vty *vty)
   /* Unset vector. */
   vector_unset (vtyvec, vty->fd);
 
+  if (vty->wfd > 0 && vty->type == VTY_FILE)
+    fsync (vty->wfd);
+
   /* Close socket. */
   if (vty->fd > 0)
-    close (vty->fd);
+    {
+      close (vty->fd);
+      if (vty->wfd > 0 && vty->wfd != vty->fd)
+        close (vty->wfd);
+    }
   else
     was_stdio = true;