IPC via abstract unix domain sockets #4

serges147 · 2024-12-23T12:45:09Z

Added new "SDK" library target; added ocvsmd-cli executable target to consume the SDK.
Posix calls now are wrapped with platform::posixSyscallError (which does retries on EINTR). For now only IPC stuff correctly wrapped; the same for other places (like executors, udp sockets, etc.) to be continued...

Also:

Now compiles on macOS by adding kqueue-based implementation of executor for Darwin. Thank you @thirtytwobits for sharing your original implementation (I did some fixes related to kevent timeout calculations - now it's nanoseconds based instead of seconds).

- Added Nunavut generation - Added UDP platform stuff

# Conflicts: # CMakeLists.txt # README.md # src/cli/CMakeLists.txt # src/common/CMakeLists.txt # src/daemon/CMakeLists.txt # src/daemon/engine/CMakeLists.txt # src/daemon/engine/application.cpp # src/daemon/engine/application.hpp # src/daemon/engine/platform/debian/epoll_single_threaded_executor.hpp # src/daemon/engine/platform/defines.hpp # src/daemon/engine/platform/udp/udp_sockets.hpp

…erface.

src/daemon/main.cpp

serges147 · 2024-12-23T17:40:53Z

src/common/ipc/unix_socket_server.cpp

+    // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay,hicpp-no-array-decay)
+    std::memcpy(addr.sun_path,
+                abstract_socket_path.c_str(),
+                std::min(sizeof(addr.sun_path), abstract_socket_path.size()));


On BSD I'm gonna return back to usual Unix domain sockets - BSD doesn't support abstract ones.

BTW, @thirtytwobits let me know please, do you seek that our ocvsmd could run on macOS?
For now it is not a problem (besides the above comment about abstract unix domain sockets), but I would like to know whether it worth to spend a bit extra efforts on this or not. Thnx

It's not required for the contract work, no. It's always helpful to have for debugging.

thirtytwobits

So far so good but how does the client get server notifications? For example, what's the flow here if the client starts a firmware update and the server is monitoring it? How does the client get the firmware update progress from the server?

thirtytwobits · 2024-12-23T17:28:18Z

src/common/ipc/unix_socket_client.hpp

+    ~UnixSocketClient();
+
+    bool connect_to_server();
+    void send_message(const std::string& message) const;


can we also support string_view?

this send_message was just to prove send and receive. real communication api of course will be different. probably passing back and forth some serialized structures.

thirtytwobits · 2024-12-23T17:41:18Z

src/common/ipc/unix_socket_server.hpp

+    CETL_NODISCARD libcyphal::IExecutor::Callback::Any registerListenCallback(
+        libcyphal::IExecutor::Callback::Function&& function) const;
+
+    void accept();


Shouldn't this return an error?

Normally it should but in my case there is nowhere to return it actually. See call site:

ipc_server_callback_ = ipc_server_.registerListenCallback([this](const auto&) { // ipc_server_.accept(); });

thirtytwobits · 2024-12-23T17:47:26Z

src/common/ipc/unix_socket_server.cpp

+    // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay,hicpp-no-array-decay)
+    std::memcpy(addr.sun_path,
+                abstract_socket_path.c_str(),
+                std::min(sizeof(addr.sun_path), abstract_socket_path.size()));


It's not required for the contract work, no. It's always helpful to have for debugging.

thirtytwobits · 2024-12-23T17:54:35Z

src/common/ipc/unix_socket_server.hpp

+    const std::string                                               socket_path_;
+    int                                                             server_fd_;
+    platform::IPosixExecutorExtension* const                        posix_executor_ext_;
+    std::unordered_map<int, std::unique_ptr<detail::ClientContext>> client_contexts_;


nit: might be more elegant to use a std::set<std::unique_ptr<detail::ClientContext>> where detail::ClientContext had a int GetHandle() const method and you provided a custom comparitor that used that.

pavel-kirienko · 2024-12-23T21:02:01Z

CMakeLists.txt

@@ -67,6 +67,13 @@ add_definitions(
        -DVCS_REVISION_ID=0x${vcs_revision_id}ULL
        -DNODE_NAME="org.opencyphal.ocvsmd"
 )
+if (DEFINED PLATFORM_LINUX_TYPE)
+    if(${PLATFORM_LINUX_TYPE} STREQUAL "bsd")
+        add_definitions(-DPLATFORM_LINUX_TYPE_BSD)


BSD is not Linux, it's a different kernel family, so PLATFORM_LINUX_TYPE may need renaming.

pavel-kirienko · 2024-12-23T21:07:19Z

CMakeLists.txt

+    if(${PLATFORM_LINUX_TYPE} STREQUAL "bsd")
+        add_definitions(-DPLATFORM_LINUX_TYPE_BSD)
+    elseif(${PLATFORM_LINUX_TYPE} STREQUAL "debian")
+        add_definitions(-DPLATFORM_LINUX_TYPE_DEBIAN)


Debian comes with BSD kernels as well, not just Linux. While we are at it, Linux is a kernel, not an OS, that would be GNU/Linux. Should we not simply go like PLATFORM_DEBIAN, PLATFORM_BSD? Although then it seems odd that one of them is an OS while the other is a kernel.

serges147 added 30 commits December 16, 2024 19:31

llvm 18; --dev parameter to skip daemonization.

235179b

added submodules

c631b31

added platform executor files

0521422

fix style

6c2820f

with submodules recursive

079e35f

- Added transport, presentation, node & getInfoProvider

0911edb

- Added Nunavut generation - Added UDP platform stuff

fixed epoll failure

5fb0d1d

added cmake presets

192a3a8

fix build

0627e4e

fix build

e385438

fix build

fd743dd

fix build

330cafa

fix build

59c827f

fix build

644d4cf

try to fix build

ad48c31

minor fixes

2f27bda

try ubuntu-24.04 runner

437e1d1

added kqueue-based BSD executor

3776ce4

fix style

2977eb0

fix style

587d558

fix style

e0dad41

fix style

873161b

added first draft of Unix socket IPC client/server

a19e4e9

fix style

db9e001

fix clang-tidy

99188fe

introduced posix utils

240f7f4

added support of client context

2a1f04d

added SDK library

0ce543c

fix build

ca67bb8

serges147 added 3 commits December 23, 2024 11:52

pollAwaitableResourcesFor now part of IPosixExecutorExtension int…

5416290

…erface.

fix mac os build

6ad1df7

try abstract unix domain sockets

d600da0

serges147 commented Dec 23, 2024

View reviewed changes

src/daemon/main.cpp Show resolved Hide resolved

serges147 marked this pull request as ready for review December 23, 2024 12:58

serges147 requested review from pavel-kirienko and thirtytwobits December 23, 2024 12:58

minor fix

98fb766

serges147 commented Dec 23, 2024

View reviewed changes

thirtytwobits reviewed Dec 23, 2024

View reviewed changes

thirtytwobits approved these changes Dec 23, 2024

View reviewed changes

pavel-kirienko approved these changes Dec 23, 2024

View reviewed changes

serges147 merged commit 94fb76d into main Dec 27, 2024
14 checks passed

serges147 deleted the sshirokov/ipc_unix_sock branch December 27, 2024 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IPC via abstract unix domain sockets #4

IPC via abstract unix domain sockets #4

serges147 commented Dec 23, 2024 •

edited

Loading

serges147 Dec 23, 2024

serges147 Dec 23, 2024 •

edited

Loading

thirtytwobits Dec 23, 2024

thirtytwobits left a comment

thirtytwobits Dec 23, 2024

serges147 Dec 23, 2024

thirtytwobits Dec 23, 2024

serges147 Dec 23, 2024 •

edited

Loading

thirtytwobits Dec 23, 2024

thirtytwobits Dec 23, 2024

pavel-kirienko Dec 23, 2024

pavel-kirienko Dec 23, 2024

IPC via abstract unix domain sockets #4

IPC via abstract unix domain sockets #4

Conversation

serges147 commented Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

serges147 Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thirtytwobits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serges147 Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serges147 commented Dec 23, 2024 •

edited

Loading

serges147 Dec 23, 2024 •

edited

Loading

serges147 Dec 23, 2024 •

edited

Loading