Ack. Thanks, Ramesh.
On 2/10/2017 7:59 PM, Anders Widell wrote: > src/base/Makefile.am | 3 + > src/base/file_descriptor.cc | 27 +++++++++++++++++ > src/base/file_descriptor.h | 27 +++++++++++++++++ > src/base/tests/unix_socket_test.cc | 60 > +++++++++++++++++++++++++++++++++---- > src/base/unix_client_socket.cc | 10 +----- > src/base/unix_server_socket.cc | 12 ++++++- > src/base/unix_socket.cc | 32 +++++++++++++------ > src/base/unix_socket.h | 49 ++++++++++++++++++++---------- > 8 files changed, 177 insertions(+), 43 deletions(-) > > > Multiple issues have been fixed in the UnixSocket class: > > * The UnixServerSocket failed if the named already exists in the file system. > * The UnixSocket used a mutex to protect Open() and Close(), but was is not > enough to make it thread safe. The mutex has been removed and users are now > responsible for protecting the instance if needed. > * The fd() method was not so useful since it required a message to be sent or > received before it returned a valid file descriptor. The fd() method now > opens > the socket if it is not already open. > * The UnixClientSocket implemented a wait-loop in case connect() returns > EINPROGRESS. This has been changed to use a blocking call to connect() > instead. > > diff --git a/src/base/Makefile.am b/src/base/Makefile.am > --- a/src/base/Makefile.am > +++ b/src/base/Makefile.am > @@ -32,6 +32,7 @@ lib_libopensaf_core_la_LDFLAGS += \ > lib_libopensaf_core_la_SOURCES += \ > src/base/condition_variable.cc \ > src/base/daemon.c \ > + src/base/file_descriptor.cc \ > src/base/file_notify.cc \ > src/base/getenv.cc \ > src/base/hj_dec.c \ > @@ -76,6 +77,7 @@ noinst_HEADERS += \ > src/base/buffer.h \ > src/base/condition_variable.h \ > src/base/daemon.h \ > + src/base/file_descriptor.h \ > src/base/file_notify.h \ > src/base/getenv.h \ > src/base/log_message.h \ > @@ -180,6 +182,7 @@ bin_libbase_test_LDFLAGS = \ > src/base/lib_libopensaf_core_la-getenv.lo \ > src/base/lib_libopensaf_core_la-log_message.lo \ > src/base/lib_libopensaf_core_la-process.lo \ > + src/base/lib_libopensaf_core_la-file_descriptor.lo \ > src/base/lib_libopensaf_core_la-file_notify.lo > > bin_libbase_test_SOURCES = \ > diff --git a/src/base/file_descriptor.cc b/src/base/file_descriptor.cc > new file mode 100644 > --- /dev/null > +++ b/src/base/file_descriptor.cc > @@ -0,0 +1,27 @@ > +/* -*- OpenSAF -*- > + * > + * Copyright Ericsson AB 2017 - All Rights Reserved. > + * > + * This program 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. This file and program are licensed > + * under the GNU Lesser General Public License Version 2.1, February 1999. > + * The complete license can be accessed from the following location: > + * http://opensource.org/licenses/lgpl-license.php > + * See the Copying file included with the OpenSAF distribution for full > + * licensing terms. > + * > + */ > + > +#include "base/file_descriptor.h" > +#include <fcntl.h> > +#include <unistd.h> > + > +namespace base { > + > +bool MakeFdNonblocking(int fd) { > + int flags = fcntl(fd, F_GETFL); > + return flags != -1 && fcntl(fd, F_SETFL, flags | O_NONBLOCK) != -1; > +} > + > +} // namespace base > diff --git a/src/base/file_descriptor.h b/src/base/file_descriptor.h > new file mode 100644 > --- /dev/null > +++ b/src/base/file_descriptor.h > @@ -0,0 +1,27 @@ > +/* -*- OpenSAF -*- > + * > + * Copyright Ericsson AB 2017 - All Rights Reserved. > + * > + * This program 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. This file and program are licensed > + * under the GNU Lesser General Public License Version 2.1, February 1999. > + * The complete license can be accessed from the following location: > + * http://opensource.org/licenses/lgpl-license.php > + * See the Copying file included with the OpenSAF distribution for full > + * licensing terms. > + * > + */ > + > +#ifndef BASE_FILE_DESCRIPTOR_H_ > +#define BASE_FILE_DESCRIPTOR_H_ > + > +namespace base { > + > +// Set the a file descriptor's non-blocking flag. Returns true if successful, > +// and false otherwise. > +bool MakeFdNonblocking(int fd); > + > +} // namespace base > + > +#endif // BASE_FILE_DESCRIPTOR_H_ > diff --git a/src/base/tests/unix_socket_test.cc > b/src/base/tests/unix_socket_test.cc > --- a/src/base/tests/unix_socket_test.cc > +++ b/src/base/tests/unix_socket_test.cc > @@ -1,6 +1,7 @@ > /* -*- OpenSAF -*- > * > * (C) Copyright 2016 The OpenSAF Foundation > + * Copyright Ericsson AB 2017 - All Rights Reserved. > * > * This program is distributed in the hope that it will be useful, but > * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > @@ -15,18 +16,18 @@ > * > */ > > +#include <sys/socket.h> > #include <sys/stat.h> > #include <unistd.h> > +#include <cerrno> > #include <cstdlib> > +#include <cstring> > #include <string> > #include "base/unix_server_socket.h" > #include "gtest/gtest.h" > > class UnixSocketTest : public ::testing::Test { > - public: > - > protected: > - > UnixSocketTest() { > } > > @@ -43,7 +44,6 @@ class UnixSocketTest : public ::testing: > return result == 0 && S_ISSOCK(buf.st_mode); > } > > - // cppcheck-suppress unusedFunction > virtual void SetUp() { > char dir_tmpl[] = "/tmp/unix_socket_testXXXXXX"; > char* dir_name = mkdtemp(dir_tmpl); > @@ -51,7 +51,6 @@ class UnixSocketTest : public ::testing: > tmpdir_ = std::string{dir_name}; > } > > - // cppcheck-suppress unusedFunction > virtual void TearDown() { > if (!tmpdir_.empty()) { > unlink(SocketName().c_str()); > @@ -62,8 +61,8 @@ class UnixSocketTest : public ::testing: > std::string tmpdir_; > }; > > -TEST_F(UnixSocketTest, CheckThatServerUnlinksSocket) { > - EXPECT_FALSE(SocketExists()); > +TEST_F(UnixSocketTest, DestructorShallUnlinkSocket) { > + ASSERT_FALSE(SocketExists()); > base::UnixServerSocket* sock = new base::UnixServerSocket(SocketName()); > EXPECT_FALSE(SocketExists()); > char buf[32]; > @@ -72,3 +71,50 @@ TEST_F(UnixSocketTest, CheckThatServerUn > delete sock; > EXPECT_FALSE(SocketExists()); > } > + > +TEST_F(UnixSocketTest, OpenShallUnlinkExistingSocket) { > + ASSERT_FALSE(SocketExists()); > + int sock = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0); > + ASSERT_GE(sock, 0); > + struct sockaddr_un addr{AF_UNIX, {}}; > + memcpy(addr.sun_path, SocketName().c_str(), SocketName().size() + 1); > + ASSERT_EQ(bind(sock, > + reinterpret_cast<const struct sockaddr*>(&addr), > + sizeof(addr)), 0); > + ASSERT_EQ(close(sock), 0); > + EXPECT_TRUE(SocketExists()); > + base::UnixServerSocket* server = new base::UnixServerSocket(SocketName()); > + char buf[32]; > + ssize_t result = server->Recv(buf, sizeof(buf)); > + int saved_errno = errno; > + EXPECT_EQ(result, -1); > + EXPECT_EQ(saved_errno, EAGAIN); > + EXPECT_GE(server->fd(), 0); > + delete server; > +} > + > +TEST_F(UnixSocketTest, DontUnlinkExistingSocketIfServerStillAlive) { > + ASSERT_FALSE(SocketExists()); > + base::UnixServerSocket* sock = new base::UnixServerSocket(SocketName()); > + char buf[32]; > + ssize_t result = sock->Recv(buf, sizeof(buf)); > + EXPECT_EQ(result, -1); > + EXPECT_EQ(errno, EAGAIN); > + base::UnixServerSocket* sock2 = new base::UnixServerSocket(SocketName()); > + result = sock2->Recv(buf, sizeof(buf)); > + EXPECT_EQ(result, -1); > + EXPECT_EQ(errno, EADDRINUSE); > + delete sock2; > + EXPECT_TRUE(SocketExists()); > + delete sock; > + EXPECT_FALSE(SocketExists()); > +} > + > +TEST_F(UnixSocketTest, FdMethodShallOpenTheSocket) { > + ASSERT_FALSE(SocketExists()); > + base::UnixServerSocket* sock = new base::UnixServerSocket(SocketName()); > + int fd = sock->fd(); > + EXPECT_GE(fd, 0); > + EXPECT_TRUE(SocketExists()); > + delete sock; > +} > diff --git a/src/base/unix_client_socket.cc b/src/base/unix_client_socket.cc > --- a/src/base/unix_client_socket.cc > +++ b/src/base/unix_client_socket.cc > @@ -1,6 +1,7 @@ > /* -*- OpenSAF -*- > * > * (C) Copyright 2016 The OpenSAF Foundation > + * Copyright Ericsson AB 2017 - All Rights Reserved. > * > * This program is distributed in the hope that it will be useful, but > * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > @@ -17,7 +18,6 @@ > > #include "base/unix_client_socket.h" > #include <sys/socket.h> > -#include <time.h> > #include <cerrno> > > namespace base { > @@ -31,15 +31,9 @@ UnixClientSocket::~UnixClientSocket() { > > bool UnixClientSocket::OpenHook(int sock) { > int result; > - int e; > do { > result = connect(sock, addr(), addrlen()); > - e = errno; > - if (result != 0 && (e == EALREADY || e == EINPROGRESS)) { > - struct timespec delay{0, 10000000}; > - clock_nanosleep(CLOCK_MONOTONIC, 0, &delay, nullptr); > - } > - } while (result != 0 && (e == EINTR || e == EALREADY || e == EINPROGRESS)); > + } while (result != 0 && errno == EINTR); > return result == 0; > } > > diff --git a/src/base/unix_server_socket.cc b/src/base/unix_server_socket.cc > --- a/src/base/unix_server_socket.cc > +++ b/src/base/unix_server_socket.cc > @@ -1,6 +1,7 @@ > /* -*- OpenSAF -*- > * > * (C) Copyright 2016 The OpenSAF Foundation > + * Copyright Ericsson AB 2017 - All Rights Reserved. > * > * This program is distributed in the hope that it will be useful, but > * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > @@ -27,10 +28,19 @@ UnixServerSocket::UnixServerSocket(const > } > > UnixServerSocket::~UnixServerSocket() { > - if (fd() >= 0) UnixServerSocket::CloseHook(); > + if (get_fd() >= 0) UnixServerSocket::CloseHook(); > } > > bool UnixServerSocket::OpenHook(int sock) { > + int tmp_sock = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0); > + int connect_result; > + int connect_errno; > + do { > + connect_result = connect(tmp_sock, addr(), addrlen()); > + connect_errno = errno; > + } while (connect_result != 0 && connect_errno == EINTR); > + close(tmp_sock); > + if (connect_result != 0 && connect_errno == ECONNREFUSED) unlink(path()); > return bind(sock, addr(), addrlen()) == 0; > } > > diff --git a/src/base/unix_socket.cc b/src/base/unix_socket.cc > --- a/src/base/unix_socket.cc > +++ b/src/base/unix_socket.cc > @@ -1,6 +1,7 @@ > /* -*- OpenSAF -*- > * > * (C) Copyright 2016 The OpenSAF Foundation > + * Copyright Ericsson AB 2017 - All Rights Reserved. > * > * This program is distributed in the hope that it will be useful, but > * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > @@ -19,14 +20,17 @@ > #include <sys/socket.h> > #include <unistd.h> > #include <cstring> > +#include "base/file_descriptor.h" > #include "base/osaf_utility.h" > +#include "base/time.h" > > namespace base { > > UnixSocket::UnixSocket(const std::string& path) : > fd_{-1}, > addr_{AF_UNIX, {}}, > - mutex_{} { > + last_failed_open_{}, > + saved_errno_{} { > if (path.size() < sizeof(addr_.sun_path)) { > memcpy(addr_.sun_path, path.c_str(), path.size() + 1); > } else { > @@ -35,18 +39,27 @@ UnixSocket::UnixSocket(const std::string > } > > int UnixSocket::Open() { > - Lock lock(mutex_); > int sock = fd_; > if (sock < 0) { > if (addr_.sun_path[0] != '\0') { > - sock = socket(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK | SOCK_CLOEXEC, 0); > - if (sock >= 0 && !OpenHook(sock)) { > - int e = errno; > - close(sock); > - sock = -1; > - errno = e; > + struct timespec current_time = ReadMonotonicClock(); > + if (TimespecToMillis(current_time - last_failed_open_) != 0) { > + sock = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0); > + if (sock >= 0 && !OpenHook(sock)) { > + int e = errno; > + close(sock); > + sock = -1; > + errno = e; > + } > + fd_ = sock; > + if (sock >=0 && MakeFdNonblocking(sock) == false) Close(); > + if (fd_ < 0) { > + last_failed_open_ = current_time; > + saved_errno_ = errno; > + } > + } else { > + errno = saved_errno_; > } > - fd_ = sock; > } else { > errno = ENAMETOOLONG; > } > @@ -61,7 +74,6 @@ UnixSocket::~UnixSocket() { > } > > void UnixSocket::Close() { > - Lock lock(mutex_); > int sock = fd_; > if (sock >= 0) { > int e = errno; > diff --git a/src/base/unix_socket.h b/src/base/unix_socket.h > --- a/src/base/unix_socket.h > +++ b/src/base/unix_socket.h > @@ -1,6 +1,7 @@ > /* -*- OpenSAF -*- > * > * (C) Copyright 2016 The OpenSAF Foundation > + * Copyright Ericsson AB 2017 - All Rights Reserved. > * > * This program is distributed in the hope that it will be useful, but > * WITHOUT ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > @@ -21,25 +22,40 @@ > #include <pthread.h> > #include <sys/socket.h> > #include <sys/un.h> > +#include <time.h> > #include <cerrno> > #include <string> > #include "base/macros.h" > -#include "base/mutex.h" > > namespace base { > > -// A class implementing non-blocking operations on a UNIX domain socket. > +// A class implementing non-blocking operations on a UNIX domain socket. This > +// class is not thread-safe and must be protected by a mutex if shared > between > +// multiple threads. > class UnixSocket { > public: > // Close the socket. > virtual ~UnixSocket(); > + // Returns the current file descriptor for this UNIX socket, or -1 if the > + // socket could not be opened. Note that the Send() and Recv() methods may > + // open and/or close the socket, and the file descriptor can therefore > + // potentially be change as a result of calling any of these two methods. > If a > + // UnixSocket instance is shared between multiple threads (and therefore > + // protected with a mutex), this means that the result from the fd() > method is > + // only valid for as long as the mutex protecting the instance is held. > + int fd() { > + int sock = fd_; > + if (sock < 0) sock = Open(); > + return sock; > + } > // Send a message in non-blocking mode. This call will open the socket if > it > // was not already open. The EINTR error code from the send() libc > function is > - // handled by retrying the send() call in a loop. In case of other errors, > the > - // socket will be closed. > + // handled by retrying the send() call in a loop. In case of other errors, > -1 > + // is returned and errno contains one of the codes listed in the recv() > libc > + // function. The socket will be closed in case the error was not EINTR, > EAGAIN > + // or EWOULDBLOCK. > ssize_t Send(const void* buffer, size_t length) { > - int sock = fd_; > - if (sock < 0) sock = Open(); > + int sock = fd(); > ssize_t result = -1; > if (sock >= 0) { > do { > @@ -52,10 +68,11 @@ class UnixSocket { > // Receive a message in non-blocking mode. This call will open the socket > if > // it was not already open. The EINTR error code from the recv() libc > function > // is handled by retrying the recv() call in a loop. In case of other > errors, > - // the socket will be closed. > + // -1 is returned and errno contains one of the codes listed in the recv() > + // libc function. The socket will be closed in case the error was not > EINTR, > + // EAGAIN or EWOULDBLOCK. > ssize_t Recv(void* buffer, size_t length) { > - int sock = fd_; > - if (sock < 0) sock = Open(); > + int sock = fd(); > ssize_t result = -1; > if (sock >= 0) { > do { > @@ -65,28 +82,26 @@ class UnixSocket { > } > return result; > } > - // Returns the current file descriptor for this UNIX socket, or -1 if the > - // socket is currently not open. Note that the Send() and Recv() methods > may > - // open and/or close the socket, and potentially the file descriptor will > be > - // different after a call to any of these two methods. > - int fd() const { return fd_; } > > protected: > explicit UnixSocket(const std::string& path); > - int Open(); > - void Close(); > virtual bool OpenHook(int sock); > virtual void CloseHook(); > const struct sockaddr* addr() const { > return reinterpret_cast<const struct sockaddr*>(&addr_); > } > static socklen_t addrlen() { return sizeof(addr_); } > + int get_fd() const { return fd_; } > const char* path() const { return addr_.sun_path; } > > private: > + int Open(); > + void Close(); > + > int fd_; > struct sockaddr_un addr_; > - Mutex mutex_; > + struct timespec last_failed_open_; > + int saved_errno_; > > DELETE_COPY_AND_MOVE_OPERATORS(UnixSocket); > }; ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
