Merge pull request #310 from ZeroCM/core-lockfile

Lockfile simplification

docs/changelog.md

@@ -5,3 +5,4 @@ Untyped functional subscriptions in c++
 Improved documentation
 Added can transport based on socketcan
 New zcm-logplayer-gui based on gtk3
+Fixed lockfiles to unlock on all program exits

zcm/transport/transport_file.cpp

@@ -4,7 +4,6 @@
 
 #include "zcm/zcm-cpp.hpp"
 #include "zcm/util/debug.h"
-//#include "zcm/util/lockfile.h"
 
 #include "util/Types.hpp"
 #include "util/TimeUtil.hpp"

zcm/transport/transport_serial.cpp

@@ -65,6 +65,7 @@ struct Serial
   private:
     string port;
     int fd = -1;
+    lockfile_t* lf;
 };
 
 bool Serial::open(const string& port_, int baud, bool hwFlowControl)
@@ -77,7 +78,8 @@ bool Serial::open(const string& port_, int baud, bool hwFlowControl)
         return false;
     }
 
-    if (!lockfile_trylock(port_.c_str())) {
+    lf = lockfile_trylock(port_.c_str());
+    if (!lf) {
         ZCM_DEBUG("failed to create lock file, refusing to open serial device (%s)",
                   port_.c_str());
         return false;
@@ -141,7 +143,10 @@ bool Serial::open(const string& port_, int baud, bool hwFlowControl)
     this->fd = -1;
 
     // Unlock the lock file
-    if (port != "") lockfile_unlock(port.c_str());
+    if (lf) {
+        lockfile_unlock(lf);
+        lf = nullptr;
+    }
     this->port = "";
 
     return false;
@@ -154,9 +159,10 @@ void Serial::close()
         ::close(fd);
         fd = 0;
     }
-    if (port != "") {
-        lockfile_unlock(port.c_str());
-        port = "";
+    if (port != "") port = "";
+    if (lf) {
+        lockfile_unlock(lf);
+        lf = nullptr;
     }
 }
 

zcm/transport/transport_zmq_local.cpp

@@ -19,6 +19,7 @@
 #include <string>
 #include <vector>
 #include <unordered_map>
+#include <utility>
 #include <mutex>
 #include <thread>
 #include <sys/stat.h>
@@ -42,7 +43,7 @@ struct ZCM_TRANS_CLASSNAME : public zcm_trans_t
 
     string subnet;
 
-    unordered_map<string, void*> pubsocks;
+    unordered_map<string, pair<void*,lockfile_t*>> pubsocks;
     // socket pair contains the socket + whether it was subscribed to explicitly or not
     unordered_map<string, pair<void*, bool>> subsocks;
     bool recvAllChannels = false;
@@ -83,17 +84,17 @@ struct ZCM_TRANS_CLASSNAME : public zcm_trans_t
         for (auto it = pubsocks.begin(); it != pubsocks.end(); ++it) {
             address = getAddress(it->first);
 
-            rc = zmq_unbind(it->second, address.c_str());
+            rc = zmq_unbind(it->second.first, address.c_str());
             if (rc == -1) {
                 ZCM_DEBUG("failed to unbind pubsock: %s", zmq_strerror(errno));
             }
 
-            rc = zmq_close(it->second);
+            rc = zmq_close(it->second.first);
             if (rc == -1) {
                 ZCM_DEBUG("failed to close pubsock: %s", zmq_strerror(errno));
             }
 
-            lockfile_unlock(address.c_str());
+            lockfile_unlock(it->second.second);
         }
 
         // Clean up all subscribe sockets
@@ -136,9 +137,10 @@ struct ZCM_TRANS_CLASSNAME : public zcm_trans_t
     {
         auto it = pubsocks.find(channel);
         if (it != pubsocks.end())
-            return it->second;
+            return it->second.first;
         // Before we create a pubsock, we need to acquire the lock file for this
-        if (!lockfile_trylock(getAddress(channel).c_str())) {
+        lockfile_t *lf = lockfile_trylock(getAddress(channel).c_str());
+        if (!lf) {
             fprintf(stderr, "Failed to acquire publish lock on %s! "
                             "Are you attempting multiple publishers?\n",
                             channel.c_str());
@@ -147,15 +149,18 @@ struct ZCM_TRANS_CLASSNAME : public zcm_trans_t
         void *sock = zmq_socket(ctx, ZMQ_PUB);
         if (sock == nullptr) {
             ZCM_DEBUG("failed to create pubsock: %s", zmq_strerror(errno));
+            lockfile_unlock(lf);
             return nullptr;
         }
         string address = getAddress(channel);
         int rc = zmq_bind(sock, address.c_str());
         if (rc == -1) {
             ZCM_DEBUG("failed to bind pubsock: %s", zmq_strerror(errno));
+            lockfile_unlock(lf);
             return nullptr;
         }
-        pubsocks.emplace(channel, sock);
+        pair<void*, lockfile_t*> p {sock, lf};
+        pubsocks.emplace(channel, p);
         return sock;
     }
 

zcm/util/lockfile.cpp

@@ -1,16 +1,17 @@
 #include "zcm/util/lockfile.h"
 #include "zcm/util/debug.h"
 
+#include <cstring>
+#include <fcntl.h>
+#include <pwd.h>
+#include <signal.h>
 #include <stdlib.h>
-#include <sys/types.h>
+#include <string>
 #include <sys/stat.h>
-#include <pwd.h>
-#include <fcntl.h>
+#include <sys/types.h>
 #include <unistd.h>
-#include <signal.h>
+#include <unordered_map>
 
-#include <cstring>
-#include <string>
 using namespace std;
 
 #define DEFAULT_LOCK_DIR "/var/lock/zcm"
@@ -58,67 +59,40 @@ static string makeLockfilePath(const string& name)
     return ret;
 }
 
-static bool isLocked(const string& path)
-{
-    int fd = open(path.c_str(), O_RDONLY);
-    if (fd < 0)
-        return false;
-
-    char buf[128];
-    int n = read(fd, buf, sizeof(buf)-1);
-    close(fd);
-    if (n == 0)
-        return false;
-    int pid = -1;
-    buf[n] = 0;
-    sscanf(buf, "%d", &pid);
-    if (pid <= 0)
-        return false;
-
-    // Check to see if the process that created the file is still alive
-    if (kill((pid_t)pid, 0) < 0 && errno == ESRCH) {
-        unlink(path.c_str());
-        return false;
-    }
-
-    return true;
-}
-
-
-bool lockfile_trylock(const char *name)
+lockfile_t* lockfile_trylock(const char *name)
 {
     auto path = makeLockfilePath(name);
-    if (path == "") return false;
-    if (isLocked(path)) {
-        ZCM_DEBUG("Cannot lock '%s', it's already locked!", path.c_str());
-        return false;
+    int fd = open(path.c_str(), O_WRONLY | O_CREAT, 0666);
+    if (fd < 0) {
+        ZCM_DEBUG("Unable to open lockfile: '%s'", name);
+        return nullptr;
     }
 
-    // Create lockfile compatible with UUCP-1.2
-    int mask = umask(022);
-    int fd = open(path.c_str(), O_WRONLY | O_CREAT | O_EXCL, 0666);
-    if (fd < 0) {
-        ZCM_DEBUG("Unable to create lockfile: '%s'", path.c_str());
-        return false; // Cannot create lock file (possibly a race)
+    int lkRet = lockf(fd, F_TLOCK, 0);
+    if (lkRet < 0) {
+        ZCM_DEBUG("Failed to lock lockfile '%s'. Error: %s", name, strerror(errno));
+        close(fd);
+        return nullptr;
     }
-    umask(mask);
-    uid_t uid = getuid();
-    gid_t gid = getgid();
-    int ret1 = chown(path.c_str(), uid, gid);
-    (void) ret1;
-    char buf[128];
-    snprintf(buf, sizeof(buf), "%10ld zcm %.20s\n", (long)getpid(), (getpwuid(uid))->pw_name);
-    ssize_t ret2 = write(fd, buf, strlen(buf));
-    (void) ret2;
-    close(fd);
 
-    return true;
+    lockfile_t *ret = new lockfile_t;
+    ret->fd = fd;
+    ret->name = strdup(name);
+    return ret;
 }
 
-void lockfile_unlock(const char *name)
+void lockfile_unlock(lockfile_t* lf)
 {
-    auto path = makeLockfilePath(name);
-    int err = unlink(path.c_str());
+    int ret = lockf(lf->fd, F_ULOCK, 0);
+    if (ret < 0)
+        ZCM_DEBUG("Failed to unlock lockfile '%s'. Error: %s", lf->name, strerror(errno));
+
+    close(lf->fd);
+
+    int err = unlink(makeLockfilePath(lf->name).c_str());
     if (err < 0)
-        ZCM_DEBUG("Failed to unlink lockfile '%s'. Error: %s", name, strerror(errno));
+        ZCM_DEBUG("Failed to unlink lockfile '%s'. Error: %s", lf->name, strerror(errno));
+
+    free(lf->name);
+    delete lf;
 }

zcm/util/lockfile.h

@@ -15,8 +15,17 @@
 extern "C" {
 #endif
 
-bool lockfile_trylock(const char *name);
-void lockfile_unlock(const char *name);
+typedef struct
+{
+    int fd;
+    char* name;
+} lockfile_t;
+
+// returns < 0 if failed to acquire lockfile
+lockfile_t* lockfile_trylock(const char *name);
+
+// lf gets destroyed after call to unlock
+void lockfile_unlock(lockfile_t* lf);
 
 #ifdef __cplusplus
 }