Use flock() instead of UUCP-style locking for serial devices

common/io/Serial.cpp

@@ -141,57 +141,12 @@ bool UIntToSpeedT(uint32_t value, speed_t *output) {
   return false;
 }
 
-bool OpenAndFlock(const std::string &path, int oflag, int *fd) {
-  // First, check if the path exists, there's no point trying to open it if not
-  if (!FileExists(path)) {
-    OLA_INFO << "Device " << path << " doesn't exist.";
-    return false;
-  }
-
-  // Now try to open the serial device.
-  if (!TryOpen(path, oflag, fd)) {
-    OLA_DEBUG << "Failed to open device " << path;
-    return false;
-  }
-
-#ifdef HAVE_FLOCK
-  if (flock(*fd, LOCK_EX | LOCK_NB) == -1) {
-    OLA_INFO << "Failed to flock() device " << path;
-    close(*fd);
-    return false;
-  }
-#else  // HAVE_FLOCK
-  OLA_WARN << "Tried to flock " << path << ", but flock() is unavailable";
-  close(*fd);
-  return false;
-#endif  // HAVE_FLOCK
-
-#if HAVE_SYS_IOCTL_H
-  // As a final safety mechanism, use ioctl(TIOCEXCL) if available to prevent
-  // further opens.
-  if (ioctl(*fd, TIOCEXCL) == -1) {
-    OLA_WARN << "TIOCEXCL " << path << " failed: " << strerror(errno);
-  }
-#endif  // HAVE_SYS_IOCTL_H
-
-  OLA_INFO << "Locked " << path << " using flock()";
-  return true;
-}
 
-
-bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) {
+bool AcquireUUCPLock(const std::string &path) {
   // This is rather tricky since there is no real convention for LCK files.
   // If it was only a single process doing the locking we could use fnctl as
   // described in 55.6 of the Linux Programming Interface book.
 
-  // First, check if the path exists, there's no point trying to open it if not
-  if (!FileExists(path)) {
-    OLA_INFO << "Device " << path << " doesn't exist, so there's no point "
-                "trying to acquire a lock";
-    return false;
-  }
-
-  // Second, clean up a stale lockfile.
   const string lock_file = GetUUCPLockFile(path);
   OLA_DEBUG << "Checking for " << lock_file;
   pid_t locked_pid;
@@ -200,6 +155,7 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) {
     return false;
   }
 
+  // Clean up a stale lockfile.
   if (locked_pid) {
     // This will return false even if we have the lock, this is what we want
     // since different plugins may try to open the same serial port - see issue
@@ -249,21 +205,51 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) {
     return false;
   }
 
+  return true;
+}
+
+bool LockTIOCEXCL(int fd, const std::string &path) {
+#if HAVE_SYS_IOCTL_H
+  // This is a final safety mechanism, on top of UUCP locking or flock()
+  if (ioctl(fd, TIOCEXCL) == -1) {
+    OLA_WARN << "TIOCEXCL " << path << " failed: " << strerror(errno);
+    return false;
+  } else {
+    OLA_INFO << "Set TIOCEXCL on " << path;
+  }
+#else
+    OLA_INFO << "Not setting " << path << " - ioctl.h unavailable";
+#endif  // HAVE_SYS_IOCTL_H
+  return true;
+}
+
+bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) {
+  // First, check if the path exists, there's no point trying to open it if not
+  if (!FileExists(path)) {
+    OLA_INFO << "Device " << path << " doesn't exist, so there's no point "
+                "trying to acquire a lock";
+    return false;
+  }
+
+  if (!AcquireUUCPLock(path)) {
+    return false;
+  }
+
   // Now try to open the serial device.
   if (!TryOpen(path, oflag, fd)) {
     OLA_DEBUG << "Failed to open device " << path << " despite having the "
               << "lock file";
-    RemoveUUCPLockFile(lock_file);
+    ReleaseUUCPLock(path);
+    close(*fd);
     return false;
   }
 
-#if HAVE_SYS_IOCTL_H
-  // As a final safety mechanism, use ioctl(TIOCEXCL) if available to prevent
-  // further opens.
-  if (ioctl(*fd, TIOCEXCL) == -1) {
-    OLA_WARN << "TIOCEXCL " << path << " failed: " << strerror(errno);
+  if (!LockTIOCEXCL(*fd, path)) {
+    ReleaseUUCPLock(path);
+    close(*fd);
+    return false;
   }
-#endif  // HAVE_SYS_IOCTL_H
+
   return true;
 }
 
@@ -285,19 +271,55 @@ void ReleaseUUCPLock(const std::string &path) {
 }
 
 bool AcquireLockAndOpenSerialPort(const std::string &path, int oflag, int *fd) {
+  // First, check if the path exists, there's no point trying to open it if not
+  if (!FileExists(path)) {
+    OLA_INFO << "Device " << path << " doesn't exist, so there's no point "
+                "trying to acquire a lock";
+    return false;
+  }
+
+#ifdef UUCP_LOCKING
+  if (!AcquireUUCPLock(path)) {
+    return false;
+  }
+#endif
+
+  // Now try to open the serial device.
+  if (!TryOpen(path, oflag, fd)) {
 #ifdef UUCP_LOCKING
-  return AcquireUUCPLockAndOpen(path, oflag, fd);
+    OLA_DEBUG << "Failed to open device " << path << " despite having the "
+              << "lock file";
+    ReleaseUUCPLock(path);
 #else
-  return OpenAndFlock(path, oflag, fd);
+    OLA_DEBUG << "Failed to open device " << path;
 #endif
+    return false;
+  }
+
+#ifdef HAVE_FLOCK
+  if (flock(*fd, LOCK_EX | LOCK_NB) == -1) {
+    OLA_INFO << "Failed to flock() device " << path;
+    close(*fd);
+    return false;
+  } else {
+    OLA_INFO << "Locked " << path << " using flock()";
+  }
+#endif
+
+  if (!LockTIOCEXCL(*fd, path)) {
+    close(*fd);
+    return false;
+  }
+
+  return true;
 }
 
 void ReleaseSerialPortLock(const std::string &path) {
 #ifdef UUCP_LOCKING
   ReleaseUUCPLock(path);
-#else   // UUCP_LOCKING
-  OLA_INFO << "No unlock necessary for " << path;
-#endif  // UUCP_LOCKING
+#else
+  OLA_INFO << "No unlock necessary for " << path << " (UUCP locking not used)";
+#endif
 }
 
 

common/io/SerialLockTester.cpp

@@ -22,6 +22,7 @@
 #include <config.h>
 #endif  // HAVE_CONFIG_H
 
+#include <sys/file.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <cppunit/extensions/HelperMacros.h>
@@ -45,29 +46,38 @@ class SerialLockTest: public CppUnit::TestFixture {
 CPPUNIT_TEST_SUITE_REGISTRATION(SerialLockTest);
 
 void SerialLockTest::testLock() {
-  bool r1, r2;
-  int fd1, fd2, fd3;
-  const std::string path = "serialLockTestFile";
+  int fd;
+  pid_t our_pid = getpid();
 
-  OLA_ASSERT_FALSE(ola::io::FileExists(path));
+  std::stringstream str;
+  str << "serialLockTestFile." << our_pid;
+  const std::string path = str.str();
 
-  fd3 = open(path.c_str(), O_CREAT | O_RDWR,
+  OLA_ASSERT_FALSE_MSG(ola::io::FileExists(path),
+                       "Test file already exists");
+
+  fd = open(path.c_str(), O_CREAT | O_RDWR,
              S_IRUSR | S_IWUSR
 #ifndef _WIN32
              | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH
 #endif  // !_WIN32
              );  // NOLINT(whitespace/parens)
-  OLA_ASSERT_FALSE(fd3 < 0);
-  close(fd3);
+  OLA_ASSERT_FALSE_MSG(fd < 0, "Couldn't create test file");
+
+#ifdef HAVE_FLOCK
+  OLA_ASSERT_TRUE(flock(fd, LOCK_EX | LOCK_NB) != -1);
+#else
 
-  r1 = ola::io::AcquireLockAndOpenSerialPort(path, O_RDWR, &fd1);
-  OLA_ASSERT_TRUE(r1);
+#ifdef UUCP_LOCKING
+  // flock() is not available, but UUCP locking was selected.  OK.
+#else
+  OLA_FAIL("Not using UUCP locking, and flock() is not available");
+#endif
 
-  r2 = ola::io::AcquireLockAndOpenSerialPort(path, O_RDWR, &fd2);
-  OLA_ASSERT_FALSE(r2);
+#endif
 
-  ola::io::ReleaseSerialPortLock(path);
-  close(fd1);
+  close(fd);
 
-  OLA_ASSERT_FALSE(unlink(path.c_str()));
+  OLA_ASSERT_FALSE_MSG(unlink(path.c_str()),
+                       "Couldn't delete test file");
 }

configure.ac

@@ -797,11 +797,12 @@ AC_CHECK_FUNC([flock], [AC_DEFINE([HAVE_FLOCK], [1],
                                   [Define if flock() is available])
                         have_flock=yes], [])
 
+UUCP_LOCKING="no"
 AS_IF([test "x$enable_uucp_locking" = xyes || test "x$have_flock" != xyes],
       [AC_DEFINE([UUCP_LOCKING], [1],
-                 [Define if UUCP locking should be used instead of flock()])
-       SERIAL_LOCK_METHOD=UUCP],
-      [SERIAL_LOCK_METHOD=flock])
+                 [Define if UUCP locking should be used])
+       UUCP_LOCKING=yes],
+      [])
 
 # UUCP Lock directory
 AC_ARG_WITH([uucp-lock],
@@ -1038,7 +1039,7 @@ Enable HTTP Server: ${have_microhttpd}
 RDM Responder Tests: ${enable_rdm_tests}
 Ja Rule: ${BUILDING_JA_RULE}
 Enabled Plugins:${PLUGINS}
-Serial port locking: $SERIAL_LOCK_METHOD
+UUCP Locking: $UUCP_LOCKING
 UUCP Lock Directory: $UUCPLOCK
 
 Now type 'make @<:@<target>@:>@'

include/ola/io/Serial.h

@@ -68,7 +68,7 @@ bool UIntToSpeedT(uint32_t value, speed_t *output);
  *
  * This fails-fast, it we can't get the lock immediately, we'll return false.
  *
- * @deprecated Use AcquireLockAndOpenSerialPort() instead (19 Feb 2022).
+ * @deprecated Use the more general AcquireLockAndOpenSerialPort() instead (19 Feb 2022).
  * @see ReleaseUUCPLock()
  */
 bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd);
@@ -79,7 +79,7 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd);
  *
  * The lock is only removed if the PID matches.
  *
- * @deprecated Use ReleaseSerialPortLock() instead (19 Feb 2022).
+ * @deprecated Use the more general ReleaseSerialPortLock() instead (19 Feb 2022).
  * @see AcquireUUCPLockAndOpen()
  */
 void ReleaseUUCPLock(const std::string &path);