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

.codespellignorelines

@@ -177,3 +177,4 @@ import java.nio.ByteOrder;
         byte[] header = ByteBuffer.allocate(4).order(ByteOrder.nativeOrder()).putInt(headerContent).array();
         int headerValue = ByteBuffer.wrap(header).order(ByteOrder.nativeOrder()).getInt();
 # This is a very bodgy workaround to the fact that the pip install of the archive doesn't seem to work properly now on Travis
+    "uest\032\036.ola.rpc.STREAMING_NO_RESPONSEB\006\200\001"

common/io/Makefile.mk

@@ -47,7 +47,8 @@ test_programs += \
     common/io/MemoryBlockTester \
     common/io/SelectServerTester \
     common/io/StreamTester \
-    common/io/TimeoutManagerTester
+    common/io/TimeoutManagerTester \
+    common/io/SerialLockTester
 
 common_io_IOQueueTester_SOURCES = common/io/IOQueueTest.cpp
 common_io_IOQueueTester_CXXFLAGS = $(COMMON_TESTING_FLAGS)
@@ -78,3 +79,7 @@ common_io_StreamTester_SOURCES = common/io/InputStreamTest.cpp \
                                  common/io/OutputStreamTest.cpp
 common_io_StreamTester_CXXFLAGS = $(COMMON_TESTING_FLAGS)
 common_io_StreamTester_LDADD = $(COMMON_TESTING_LIBS)
+
+common_io_SerialLockTester_SOURCES = common/io/SerialLockTester.cpp
+common_io_SerialLockTester_CXXFLAGS = $(COMMON_TESTING_FLAGS)
+common_io_SerialLockTester_LDADD = $(COMMON_TESTING_LIBS)

common/io/Serial.cpp

@@ -27,6 +27,7 @@
 #include <signal.h>
 #include <string.h>
 #include <sys/stat.h>
+#include <sys/file.h>
 #ifdef _WIN32
 #define VC_EXTRALEAN
 #define WIN32_LEAN_AND_MEAN
@@ -57,7 +58,7 @@ using std::string;
 
 namespace {
 
-string GetLockFile(const string &path) {
+string GetUUCPLockFile(const string &path) {
   const string base_name = ola::file::FilenameFromPath(path);
   return ola::file::JoinPaths(UUCP_LOCK_DIR, "LCK.." + base_name);
 }
@@ -106,7 +107,7 @@ bool ProcessExists(pid_t pid) {
 #endif  // _WIN32
 }
 
-bool RemoveLockFile(const string &lock_file) {
+bool RemoveUUCPLockFile(const string &lock_file) {
   if (unlink(lock_file.c_str())) {
     OLA_WARN << "Failed to remove UUCP lock file: " << lock_file;
     return false;
@@ -140,6 +141,43 @@ 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) {
   // This is rather tricky since there is no real convention for LCK files.
@@ -154,7 +192,7 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) {
   }
 
   // Second, clean up a stale lockfile.
-  const string lock_file = GetLockFile(path);
+  const string lock_file = GetUUCPLockFile(path);
   OLA_DEBUG << "Checking for " << lock_file;
   pid_t locked_pid;
   if (!GetPidFromFile(lock_file, &locked_pid)) {
@@ -173,7 +211,7 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) {
     }
     // There is a race between the read & the unlink here. I'm not convinced it
     // can be solved.
-    if (!RemoveLockFile(lock_file)) {
+    if (!RemoveUUCPLockFile(lock_file)) {
       OLA_INFO << "Device " << path << " was locked by PID " << locked_pid
                << " which is no longer active, however failed to remove stale "
                << "lock file";
@@ -207,15 +245,15 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) {
   close(lock_fd);
   if (r != pid_file_contents.size()) {
     OLA_WARN << "Failed to write complete LCK file: " << lock_file;
-    RemoveLockFile(lock_file);
+    RemoveUUCPLockFile(lock_file);
     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";
-    RemoveLockFile(lock_file);
+    RemoveUUCPLockFile(lock_file);
     return false;
   }
 
@@ -224,17 +262,14 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) {
   // further opens.
   if (ioctl(*fd, TIOCEXCL) == -1) {
     OLA_WARN << "TIOCEXCL " << path << " failed: " << strerror(errno);
-    close(*fd);
-    RemoveLockFile(lock_file);
-    return false;
   }
 #endif  // HAVE_SYS_IOCTL_H
   return true;
 }
 
 
 void ReleaseUUCPLock(const std::string &path) {
-  const string lock_file = GetLockFile(path);
+  const string lock_file = GetUUCPLockFile(path);
 
   pid_t locked_pid;
   if (!GetPidFromFile(lock_file, &locked_pid)) {
@@ -243,10 +278,28 @@ void ReleaseUUCPLock(const std::string &path) {
 
   pid_t our_pid = getpid();
   if (our_pid == locked_pid) {
-    if (RemoveLockFile(lock_file)) {
+    if (RemoveUUCPLockFile(lock_file)) {
       OLA_INFO << "Released " << lock_file;
     }
   }
 }
+
+bool AcquireLockAndOpenSerialPort(const std::string &path, int oflag, int *fd) {
+#ifdef UUCP_LOCKING
+  return AcquireUUCPLockAndOpen(path, oflag, fd);
+#else
+  return OpenAndFlock(path, oflag, fd);
+#endif
+}
+
+void ReleaseSerialPortLock(const std::string &path) {
+#ifdef UUCP_LOCKING
+  ReleaseUUCPLock(path);
+#else   // UUCP_LOCKING
+  OLA_INFO << "No unlock necessary for " << path;
+#endif  // UUCP_LOCKING
+}
+
+
 }  // namespace io
 }  // namespace ola

common/io/SerialLockTester.cpp

@@ -0,0 +1,73 @@
+/*
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ * SerialLockTester.cpp
+ * Test for serial port locking
+ * Copyright (C) 2022 Thomas White
+ */
+
+#if HAVE_CONFIG_H
+#include <config.h>
+#endif  // HAVE_CONFIG_H
+
+#include <unistd.h>
+#include <fcntl.h>
+#include <cppunit/extensions/HelperMacros.h>
+#include <string>
+
+#include "ola/io/Serial.h"
+#include "ola/io/IOUtils.h"
+#include "ola/testing/TestUtils.h"
+
+class SerialLockTest: public CppUnit::TestFixture {
+ public:
+  CPPUNIT_TEST_SUITE(SerialLockTest);
+  CPPUNIT_TEST(testLock);
+  CPPUNIT_TEST_SUITE_END();
+
+ public:
+  void testLock();
+};
+
+
+CPPUNIT_TEST_SUITE_REGISTRATION(SerialLockTest);
+
+void SerialLockTest::testLock() {
+  bool r1, r2;
+  int fd1, fd2, fd3;
+  const std::string path = "serialLockTestFile";
+
+  OLA_ASSERT_FALSE(ola::io::FileExists(path));
+
+  fd3 = 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);
+
+  r1 = ola::io::AcquireLockAndOpenSerialPort(path, O_RDWR, &fd1);
+  OLA_ASSERT_TRUE(r1);
+
+  r2 = ola::io::AcquireLockAndOpenSerialPort(path, O_RDWR, &fd2);
+  OLA_ASSERT_FALSE(r2);
+
+  ola::io::ReleaseSerialPortLock(path);
+  close(fd1);
+
+  OLA_ASSERT_FALSE(unlink(path.c_str()));
+}

configure.ac

@@ -787,6 +787,22 @@ AC_ARG_ENABLE(
   [AS_HELP_STRING([--disable-doxygen-version],
   [Substitute the Doxygen version with latest, for the website])])
 
+# Use UUCP locking?
+AC_ARG_ENABLE(
+  [uucp-locking],
+  [AS_HELP_STRING([--enable-uucp-locking],
+  [Use UUCP locking instead of flock()])])
+
+AC_CHECK_FUNC([flock], [AC_DEFINE([HAVE_FLOCK], [1],
+                                  [Define if flock() is available])
+                        have_flock=yes], [])
+
+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])
+
 # UUCP Lock directory
 AC_ARG_WITH([uucp-lock],
             [AS_HELP_STRING([--with-uucp-lock],
@@ -1022,6 +1038,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 Lock Directory: $UUCPLOCK
 
 Now type 'make @<:@<target>@:>@'

include/ola/io/Serial.h

@@ -67,6 +67,9 @@ bool UIntToSpeedT(uint32_t value, speed_t *output);
  * @returns true if the open succeeded, false otherwise.
  *
  * This fails-fast, it we can't get the lock immediately, we'll return false.
+ *
+ * @deprecated Use AcquireLockAndOpenSerialPort() instead (19 Feb 2022).
+ * @see ReleaseUUCPLock()
  */
 bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd);
 
@@ -75,8 +78,41 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd);
  * @param path The path to unlock.
  *
  * The lock is only removed if the PID matches.
+ *
+ * @deprecated Use ReleaseSerialPortLock() instead (19 Feb 2022).
+ * @see AcquireUUCPLockAndOpen()
  */
 void ReleaseUUCPLock(const std::string &path);
+
+/**
+ * @brief Try to open the path and obtain a lock to control access
+ * @param path the path to open
+ * @param oflag flags passed to open
+ * @param[out] fd a pointer to the fd which is returned.
+ * @returns true if the open succeeded, false otherwise.
+ *
+ * Depending on the compile-time configuration, this will use either flock()
+ * (the default) or UUCP locking.  See: ./configure --enable-uucp-locking.
+ *
+ * This fails-fast, it we can't get the lock immediately, we'll return false.
+ *
+ * @see ReleaseSerialPortLock()
+ */
+bool AcquireLockAndOpenSerialPort(const std::string &path, int oflag, int *fd);
+
+/**
+ * @brief Release a lock for the serial port
+ * @param path The path to unlock.
+ *
+ * If UUCP locking was used (see AcquireLockAndOpenSerialPort()), the lockfile
+ * will be removed (but only if the PID matches).
+ *
+ * Does nothing if flock() was used (see ./configure --enable-uucp-locking).
+ *
+ * @see AcquireLockAndOpenSerialPort()
+ */
+void ReleaseSerialPortLock(const std::string &path);
+
 }  // namespace io
 }  // namespace ola
 #endif  // INCLUDE_OLA_IO_SERIAL_H_

plugins/stageprofi/StageProfiDetector.cpp

@@ -126,7 +126,7 @@ void StageProfiDetector::ReleaseWidget(const std::string &widget_path) {
   // the map.
   DescriptorMap::iterator iter = m_usb_widgets.find(widget_path);
   if (iter != m_usb_widgets.end()) {
-    ola::io::ReleaseUUCPLock(widget_path);
+    ola::io::ReleaseSerialPortLock(widget_path);
     iter->second = NULL;
     return;
   }
@@ -164,9 +164,9 @@ ConnectedDescriptor* StageProfiDetector::ConnectToUSB(
   struct termios newtio;
 
   int fd;
-  if (!ola::io::AcquireUUCPLockAndOpen(widget_path,
-                                       O_RDWR | O_NONBLOCK | O_NOCTTY,
-                                       &fd)) {
+  if (!ola::io::AcquireLockAndOpenSerialPort(widget_path,
+                                             O_RDWR | O_NONBLOCK | O_NOCTTY,
+                                             &fd)) {
     return NULL;
   }
 

plugins/usbpro/BaseUsbProWidget.cpp

@@ -129,8 +129,9 @@ ola::io::ConnectedDescriptor *BaseUsbProWidget::OpenDevice(
     const string &path) {
   struct termios newtio;
   int fd;
-  if (!ola::io::AcquireUUCPLockAndOpen(path, O_RDWR | O_NONBLOCK | O_NOCTTY,
-                                       &fd)) {
+  if (!ola::io::AcquireLockAndOpenSerialPort(path,
+                                             O_RDWR | O_NONBLOCK | O_NOCTTY,
+                                             &fd)) {
     return NULL;
   }
 

plugins/usbpro/WidgetDetectorThread.cpp

@@ -420,7 +420,7 @@ void WidgetDetectorThread::FreeDescriptor(ConnectedDescriptor *descriptor) {
   DescriptorInfo &descriptor_info = m_active_descriptors[descriptor];
 
   m_active_paths.erase(descriptor_info.first);
-  io::ReleaseUUCPLock(descriptor_info.first);
+  io::ReleaseSerialPortLock(descriptor_info.first);
   m_active_descriptors.erase(descriptor);
   delete descriptor;
 }