[{"id":2060,"web_url":"https://patchwork.libcamera.org/comment/2060/","msgid":"<20190630235557.GM7043@pendragon.ideasonboard.com>","date":"2019-06-30T23:55:57","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: ipc: unix: Add a\n\tIPC mechanism based on Unix sockets","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sun, Jun 30, 2019 at 06:25:13PM +0200, Niklas Söderlund wrote:\n> To be able to isolate an IPA component in a separate process an IPC\n> mechanism is needed to communicate with it. Add an IPC mechanism based\n> on Unix sockets which allows users to pass both data and file descriptors\n> to and from the IPA process.\n> \n> The implementation allows users to send both data and file descriptors\n> in the same message. This allows users to more easily implement\n> serialization and deserialization of objects as all elements belonging\n> to an object can be sent in one message.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  src/libcamera/include/ipc_unixsocket.h |  57 +++++\n>  src/libcamera/ipc_unixsocket.cpp       | 312 +++++++++++++++++++++++++\n>  src/libcamera/meson.build              |   2 +\n>  3 files changed, 371 insertions(+)\n>  create mode 100644 src/libcamera/include/ipc_unixsocket.h\n>  create mode 100644 src/libcamera/ipc_unixsocket.cpp\n> \n> diff --git a/src/libcamera/include/ipc_unixsocket.h b/src/libcamera/include/ipc_unixsocket.h\n> new file mode 100644\n> index 0000000000000000..84d786e8424ccf54\n> --- /dev/null\n> +++ b/src/libcamera/include/ipc_unixsocket.h\n> @@ -0,0 +1,57 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipc_unixsocket.h - IPC mechanism based on Unix sockets\n> + */\n> +\n> +#ifndef __LIBCAMERA_IPC_UNIXSOCKET_H__\n> +#define __LIBCAMERA_IPC_UNIXSOCKET_H__\n> +\n> +#include <cstdint>\n> +#include <sys/types.h>\n> +#include <vector>\n> +\n> +#include <libcamera/event_notifier.h>\n> +\n> +namespace libcamera {\n> +\n> +class IPCUnixSocket\n> +{\n> +public:\n> +\tstruct Payload {\n> +\t\tstd::vector<uint8_t> data;\n> +\t\tstd::vector<int32_t> fds;\n> +\t};\n> +\n> +\tIPCUnixSocket();\n> +\t~IPCUnixSocket();\n> +\n> +\tint create();\n> +\tint bind(int fd);\n> +\tvoid close();\n> +\tbool isBound();\n\n\tbool isBound() const;\n\n> +\n> +\tint send(const Payload &payload);\n> +\tint receive(Payload *payload);\n> +\n> +\tSignal<IPCUnixSocket *> readyRead;\n> +\n> +private:\n> +\tstruct Header {\n> +\t\tuint32_t data;\n> +\t\tuint8_t fds;\n> +\t};\n> +\n> +\tint sendData(const void *buffer, size_t length, const int32_t *fds, unsigned int num);\n> +\tint recvData(void *buffer, size_t length, int32_t *fds, unsigned int num);\n> +\n> +\tvoid dataNotifier(EventNotifier *notifier);\n> +\n> +\tint fd_;\n> +\tEventNotifier *notifier_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPC_UNIXSOCKET_H__ */\n> diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> new file mode 100644\n> index 0000000000000000..f9ef4dafa4db0652\n> --- /dev/null\n> +++ b/src/libcamera/ipc_unixsocket.cpp\n> @@ -0,0 +1,312 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * ipc_unixsocket.cpp - IPC mechanism based on Unix sockets\n> + */\n> +\n> +#include \"ipc_unixsocket.h\"\n> +\n> +#include <string.h>\n> +#include <sys/socket.h>\n> +#include <unistd.h>\n> +\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file ipc_unixsocket.h\n> + * \\brief IPC mechanism based on Unix sockets\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPCUnixSocket)\n> +\n> +/**\n> + * \\struct IPCUnixSocket::Payload\n> + * \\brief Container for an IPC payload\n> + *\n> + * Holds an array of bytes and an array of file descriptors that can be\n> + * transported across a IPC boundary.\n> + */\n> +\n> +/**\n> + * \\var IPCUnixSocket::Payload::data\n> + * \\brief Array of bytes to cross IPC boundary\n> + */\n> +\n> +/**\n> + * \\var IPCUnixSocket::Payload::fds\n> + * \\brief Array of file descriptors to cross IPC boundary\n> + */\n> +\n> +/**\n> + * \\class IPCUnixSocket\n> + * \\brief IPC mechanism based on Unix sockets\n> + *\n> + * The Unix socket IPC allows bidirectional communication between two processes\n> + * through unnamed Unix sockets. It implements datagram-based communication,\n> + * transporting entire payloads with guaranteed ordering.\n> + *\n> + * The IPC design is asynchronous, a message is queued to a receiver which gets\n> + * notified that a message is ready to be consumed by a signal. The queuer of\n> + * the message gets no notification when a message is delivered nor processed.\n> + * If such interactions are needed a protocol specific to the users use-case\n> + * should be implemented on top of the IPC objects.\n> + *\n> + * The IPC design can transmit messages in any direction and the only difference\n> + * from a master and slave operation is how they are created and bound to one\n> + * another. After the two parts are setup the operation to send and receive\n> + * messages are the same for both.\n\nI think you can drop the above paragraph as the next one contains the\nsame information and more.\n\n> + * Establishment of an IPC channel is asymmetrical. The side that initiates\n> + * communication first instantiates a local side socket and creates the channel\n> + * with create(). The method returns a file descriptor for the remote side of\n> + * the channel, which is passed to the remote process through an out-of-band\n> + * communication method. The remote side then instantiates a socket, and binds\n> + * it to the other side by passing the file descriptor to bind(). At that point\n> + * the channel is operation and communication is bidirectional and symmmetrical.\n> + */\n> +\n> +IPCUnixSocket::IPCUnixSocket()\n> +\t: fd_(-1), notifier_(nullptr)\n> +{\n> +}\n> +\n> +IPCUnixSocket::~IPCUnixSocket()\n> +{\n> +\tclose();\n> +}\n> +\n> +/**\n> + * \\brief Create an new IPC channel\n> + *\n> + * This method creates a new IPC channel. The socket instance is bound to the\n> + * local side of the channel, and the method returns a file descriptor bound to\n> + * the remote side. The caller is responsible for passing the file descriptor to\n> + * the remote process, where it can be used with IPCUnixSocket::bind() to bind\n> + * the remote side socket.\n> + *\n> + * \\return A file descriptor on success, negative error code on failure\n> + */\n> +int IPCUnixSocket::create()\n> +{\n> +\tint sockets[2];\n> +\tint ret;\n> +\n> +\tret = socketpair(AF_UNIX, SOCK_DGRAM, 0, sockets);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(IPCUnixSocket, Error)\n> +\t\t\t<< \"Failed to create socket pair: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = bind(sockets[0]);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn sockets[1];\n> +}\n> +\n> +/**\n> + * \\brief Bind to an existing IPC channel\n> + * \\param[in] fd File descriptor\n> + *\n> + * This method binds the socket instance to an existing IPC channel identified\n> + * by the file descriptor \\a fd. The file descriptor is obtained from the\n> + * IPCUnixSocket::create() method.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int IPCUnixSocket::bind(int fd)\n> +{\n> +\tif (isBound())\n> +\t\treturn -EINVAL;\n> +\n> +\tfd_ = fd;\n> +\tnotifier_ = new EventNotifier(fd_, EventNotifier::Read);\n> +\tnotifier_->activated.connect(this, &IPCUnixSocket::dataNotifier);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Close the IPC channel\n> + *\n> + * No communication is possible after close() has been called.\n> + */\n> +void IPCUnixSocket::close()\n> +{\n> +\tif (!isBound())\n> +\t\treturn;\n> +\n> +\tdelete notifier_;\n> +\tnotifier_ = nullptr;\n> +\n> +\t::close(fd_);\n> +\n> +\tfd_ = -1;\n> +}\n> +\n> +/**\n> + * \\brief Check if the IPC channel is bound\n> + * \\return True if the IPC channel is bound, false otherwise\n> + */\n> +bool IPCUnixSocket::isBound()\n> +{\n> +\treturn fd_ != -1;\n> +}\n> +\n> +/**\n> + * \\brief Send a message payload\n> + * \\param[in] payload Message payload to send\n> + *\n> + * This method queues the message payload for transmission to the other end of\n> + * the IPC channel. This method returns immediately, before the message is\n\ns/This method/It/\n\n> + * delivered to the remote side.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int IPCUnixSocket::send(const Payload &payload)\n> +{\n> +\tHeader hdr;\n\nYou can declare this variable just before using it.\n\n> +\tint ret;\n> +\n> +\tif (!isBound())\n> +\t\treturn -ENOTCONN;\n> +\n> +\thdr.data = payload.data.size();\n> +\thdr.fds = payload.fds.size();\n> +\n> +\tret = ::send(fd_, &hdr, sizeof(hdr), 0);\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(IPCUnixSocket, Error)\n> +\t\t\t<< \"Failed to send: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn sendData(payload.data.data(), hdr.data, payload.fds.data(), hdr.fds);\n> +}\n> +\n> +/**\n> + * \\brief Receive a message payload\n> + * \\param[out] payload Payload where to write the received message\n> + *\n> + * This method receives the message payload from the IPC channel and writes it\n> + * to the \\a payload. This method blocks until one message is received, if an\n\ns/This method/It/\n\n> + * asynchronous behavior is desired this method should be called when the\n> + * readyRead signal is emitted.\n\nHmmm... We'll have a problem here, if the remove side sends us a header\nbut no payload, we'll block forever. I think we'll have to implement a\nstate machine to fix this. It could possibly be done on top of this\npatch series to avoid blocking development that depends on IPC, but then\nyou should record a \\todo item.\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int IPCUnixSocket::receive(Payload *payload)\n> +{\n> +\tHeader hdr;\n> +\tint ret;\n> +\n> +\tif (!isBound())\n> +\t\treturn -ENOTCONN;\n> +\n> +\tif (!payload)\n> +\t\treturn -EINVAL;\n> +\n> +\tret = ::recv(fd_, &hdr, sizeof(hdr), 0);\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(IPCUnixSocket, Error)\n> +\t\t\t<< \"Failed to recv header: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tpayload->data.resize(hdr.data);\n> +\tpayload->fds.resize(hdr.fds);\n> +\n> +\tret = recvData(payload->data.data(), hdr.data, payload->fds.data(), hdr.fds);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n\nJust\n\n\treturn recvData(...);\n\n> +}\n> +\n> +/**\n> + * \\var IPCUnixSocket::readyRead\n> + * \\brief A Signal emitted when a message is ready to be read\n> + */\n> +\n> +int IPCUnixSocket::sendData(const void *buffer, size_t length, const int32_t *fds, unsigned int num)\n> +{\n> +\tstruct iovec iov[1];\n> +\tiov[0].iov_base = const_cast<void *>(buffer);\n> +\tiov[0].iov_len = length;\n> +\n> +\tchar buf[CMSG_SPACE(num * sizeof(uint32_t))];\n> +\tmemset(buf, 0, sizeof(buf));\n> +\n> +\tstruct cmsghdr *cmsg = (struct cmsghdr *)buf;\n> +\tcmsg->cmsg_len = CMSG_LEN(num * sizeof(uint32_t));\n> +\tcmsg->cmsg_level = SOL_SOCKET;\n> +\tcmsg->cmsg_type = SCM_RIGHTS;\n> +\n> +\tstruct msghdr msg;\n> +\tmsg.msg_name = nullptr;\n> +\tmsg.msg_namelen = 0;\n> +\tmsg.msg_iov = iov;\n> +\tmsg.msg_iovlen = 1;\n> +\tmsg.msg_control = cmsg;\n> +\tmsg.msg_controllen = cmsg->cmsg_len;\n> +\tmsg.msg_flags = 0;\n> +\tmemcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));\n> +\n> +\tif (sendmsg(fd_, &msg, 0) < 0) {\n> +\t\tint ret = -errno;\n> +\t\tLOG(IPCUnixSocket, Error)\n> +\t\t\t<< \"Failed to sendmsg: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int IPCUnixSocket::recvData(void *buffer, size_t length, int32_t *fds, unsigned int num)\n> +{\n> +\tstruct iovec iov[1];\n> +\tiov[0].iov_base = buffer;\n> +\tiov[0].iov_len = length;\n> +\n> +\tchar buf[CMSG_SPACE(num * sizeof(uint32_t))];\n> +\tmemset(buf, 0, sizeof(buf));\n> +\n> +\tstruct cmsghdr *cmsg = (struct cmsghdr *)buf;\n> +\tcmsg->cmsg_len = CMSG_LEN(num * sizeof(uint32_t));\n> +\tcmsg->cmsg_level = SOL_SOCKET;\n> +\tcmsg->cmsg_type = SCM_RIGHTS;\n> +\n> +\tstruct msghdr msg;\n> +\tmsg.msg_name = nullptr;\n> +\tmsg.msg_namelen = 0;\n> +\tmsg.msg_iov = iov;\n> +\tmsg.msg_iovlen = 1;\n> +\tmsg.msg_control = cmsg;\n> +\tmsg.msg_controllen = cmsg->cmsg_len;\n> +\tmsg.msg_flags = 0;\n> +\n> +\tif (recvmsg(fd_, &msg, 0) < 0) {\n> +\t\tint ret = -errno;\n> +\t\tLOG(IPCUnixSocket, Error)\n> +\t\t\t<< \"Failed to recvmsg: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tmemcpy(fds, CMSG_DATA(cmsg), num * sizeof(uint32_t));\n> +\n> +\treturn 0;\n> +}\n> +\n> +void IPCUnixSocket::dataNotifier(EventNotifier *notifier)\n> +{\n> +\treadyRead.emit(this);\n\nI should implement support for connecting signals to signals with the\nneed for an intermediate slot.\n\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 985aa7e8ab0eb6ce..45bd9d1793aa0b19 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -13,6 +13,7 @@ libcamera_sources = files([\n>      'ipa_interface.cpp',\n>      'ipa_manager.cpp',\n>      'ipa_module.cpp',\n> +    'ipc_unixsocket.cpp',\n>      'log.cpp',\n>      'media_device.cpp',\n>      'media_object.cpp',\n> @@ -38,6 +39,7 @@ libcamera_headers = files([\n>      'include/formats.h',\n>      'include/ipa_manager.h',\n>      'include/ipa_module.h',\n> +    'include/ipc_unixsocket.h',\n>      'include/log.h',\n>      'include/media_device.h',\n>      'include/media_object.h',","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A77E260BC0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  1 Jul 2019 01:56:16 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 49570255;\n\tMon,  1 Jul 2019 01:56:16 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561938976;\n\tbh=CanzHdbUotDMbO/mtquLymG0NUE71dr8f1LizbEyhqw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uNqIGBIZroSfO9tvalw7Gxe72DpHKuFGSggldlsR2+GzAkQdBGroJlg4lzs8eZKZY\n\tQnDnKL1qUOsCWQDwjPn32mjFsKlLM2IqcapL92lxxW/zIhiKIIZfgb8bJCsv/RA6/o\n\t92eQzCwH9lCyrzDNEm0lYkW710N0c0UZ5kjIIV7Q=","Date":"Mon, 1 Jul 2019 02:55:57 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190630235557.GM7043@pendragon.ideasonboard.com>","References":"<20190630162514.20522-1-niklas.soderlund@ragnatech.se>\n\t<20190630162514.20522-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190630162514.20522-3-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: ipc: unix: Add a\n\tIPC mechanism based on Unix sockets","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sun, 30 Jun 2019 23:56:16 -0000"}}]