[{"id":14282,"web_url":"https://patchwork.libcamera.org/comment/14282/","msgid":"","date":"2020-12-18T16:13:44","subject":"Re: [libcamera-devel] [PATCH v5 07/23] libcamera: Add IPCPipe\n\timplementation based on unix socket","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Paul,\n\nThanks for your work.\n\nOn 2020-12-05 19:30:50 +0900, Paul Elder wrote:\n> Add an implementation of IPCPipe using unix socket.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v5:\n> - rename IPAIPCUnixSocket to IPCPipeUnixSocket\n> - rename ipa_ipc_unisocket.[(cpp),h] ipc_pipe_unixsocket.[(cpp),h]\n> \n> Changes in v4:\n> - change snake_case to camelCase\n> - change proc_ and socket_ to unique pointers\n> - move inclusion of corresponding header to first in the include list\n> - reserve message data and fds size (for sending)\n> \n> Changes in v3:\n> - remove unused writeUInt32() and readUInt32()\n> - remove redundant definition of IPAIPCUnixSocket::isValid()\n> - remove & 0xff in writeHeader()\n> - make readHeader, writeHeader, and eraseHeader static class functions\n> of IPAIPCUnixSocket instead of globals\n> \n> Changes in v2:\n> - specify in doxygen to skip generating documentation for\n> IPAIPCUnixSocket\n> ---\n> Documentation/Doxyfile.in | 2 +\n> .../libcamera/internal/ipc_pipe_unixsocket.h | 50 ++++++\n> src/libcamera/ipc_pipe_unixsocket.cpp | 147 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 200 insertions(+)\n> create mode 100644 include/libcamera/internal/ipc_pipe_unixsocket.h\n> create mode 100644 src/libcamera/ipc_pipe_unixsocket.cpp\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index b18b8e9c..36a0cef3 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -837,8 +837,10 @@ RECURSIVE = YES\n> EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> \t\t\t @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \\\n> \t\t\t @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \\\n> +\t\t\t @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \\\n> +\t\t\t @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/proxy/ \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> new file mode 100644\n> index 00000000..fea3179c\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> @@ -0,0 +1,50 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipc_pipe_unixsocket.h - Image Processing Algorithm IPC module using unix socket\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__\n> +#define __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/ipc_pipe.h\"\n> +#include \"libcamera/internal/ipc_unixsocket.h\"\n> +\n> +namespace libcamera {\n> +\n> +class Process;\n> +\n> +class IPCPipeUnixSocket : public IPCPipe\n> +{\n> +public:\n> +\tIPCPipeUnixSocket(const char *ipaModulePath, const char *ipaProxyWorkerPath);\n> +\t~IPCPipeUnixSocket();\n> +\n> +\tint sendSync(uint32_t cmd,\n> +\t\t const IPCMessage &in,\n> +\t\t IPCMessage *out = nullptr) override;\n> +\n> +\tint sendAsync(uint32_t cmd, const IPCMessage &data) override;\n> +\n> +private:\n> +\tstruct CallData {\n> +\t\tIPCUnixSocket::Payload *response;\n> +\t\tbool done;\n> +\t};\n> +\n> +\tvoid readyRead(IPCUnixSocket *socket);\n> +\tint call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq);\n> +\n> +\tuint32_t seq_;\n> +\tstd::unique_ptr proc_;\n> +\tstd::unique_ptr socket_;\n> +\tstd::map callData_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__ */\n> diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp\n> new file mode 100644\n> index 00000000..b47c47d2\n> --- /dev/null\n> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> @@ -0,0 +1,147 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipc_pipe_unixsocket.cpp - Image Processing Algorithm IPC module using unix socket\n> + */\n> +\n> +#include \"libcamera/internal/ipc_pipe_unixsocket.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/event_dispatcher.h\"\n> +#include \"libcamera/internal/ipc_pipe.h\"\n> +#include \"libcamera/internal/ipc_unixsocket.h\"\n> +#include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/process.h\"\n> +#include \"libcamera/internal/thread.h\"\n> +#include \"libcamera/internal/timer.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(IPCPipe)\n> +\n> +IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath,\n> +\t\t\t\t const char *ipaProxyWorkerPath)\n> +\t: IPCPipe(), seq_(0),\n> +\t proc_(nullptr), socket_(nullptr)\n\nIs there a need to initialize proc_ and socket_ to nullptr, is not \nstd::unique_ptr<> already initialized as such?\n\n> +{\n> +\tstd::vector fds;\n> +\tstd::vector args;\n> +\targs.push_back(ipaModulePath);\n> +\n> +\tsocket_ = std::make_unique();\n> +\tint fd = socket_->create();\n> +\tif (fd < 0) {\n> +\t\tLOG(IPCPipe, Error) << \"Failed to create socket\";\n> +\t\treturn;\n> +\t}\n> +\tsocket_->readyRead.connect(this, &IPCPipeUnixSocket::readyRead);\n> +\targs.push_back(std::to_string(fd));\n> +\tfds.push_back(fd);\n> +\n> +\tproc_ = std::make_unique();\n> +\tint ret = proc_->start(ipaProxyWorkerPath, args, fds);\n> +\tif (ret) {\n> +\t\tLOG(IPCPipe, Error)\n> +\t\t\t<< \"Failed to start proxy worker process\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tconnected_ = true;\n\nThis is not checked anywhere in the code, is it a leftover from an \nearlier design? Would it make sens to add a mandatory connect() method \nin the base class that must be called successfully before any \ncommunication is allowed?\n\n> +}\n> +\n> +IPCPipeUnixSocket::~IPCPipeUnixSocket()\n> +{\n> +}\n> +\n> +int IPCPipeUnixSocket::sendSync(uint32_t cmd,\n> +\t\t\t\tconst IPCMessage &in, IPCMessage *out)\n> +{\n> +\tIPCUnixSocket::Payload message, response;\n> +\n> +\tconst IPCMessage::Header header{ cmd, ++seq_ };\n> +\tmessage = in.payload(&header);\n> +\n> +\tint ret = call(message, &response, seq_);\n> +\tif (ret) {\n> +\t\tLOG(IPCPipe, Error) << \"Failed to call sync\";\n> +\t\tcallData_.erase(seq_);\n\nI think you should clear callData in call() as you aready do for all but \none error path and drop it here.\n\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (out)\n> +\t\t*out = IPCMessage(response);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int IPCPipeUnixSocket::sendAsync(uint32_t cmd, const IPCMessage &data)\n> +{\n> +\tconst IPCMessage::Header header{ cmd, 0 };\n> +\tIPCUnixSocket::Payload message = data.payload(&header);\n> +\n> +\tint ret = socket_->send(message);\n> +\tif (ret) {\n> +\t\tLOG(IPCPipe, Error) << \"Failed to call async\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void IPCPipeUnixSocket::readyRead(IPCUnixSocket *socket)\n> +{\n> +\tIPCUnixSocket::Payload message;\n> +\tint ret = socket->receive(&message);\n> +\tif (ret) {\n> +\t\tLOG(IPCPipe, Error) << \"Receive message failed\" << ret;\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* \\todo Use span to avoid the double copy when callData is found. */\n> +\tIPCMessage ipcMessage(message);\n> +\n> +\tauto callData = callData_.find(ipcMessage.header().cookie);\n> +\tif (callData != callData_.end()) {\n> +\t\t*callData->second.response = std::move(message);\n> +\t\tcallData->second.done = true;\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* Received unexpected data, this means it's a call from the IPA. */\n> +\trecv.emit(ipcMessage.header().cmd, ipcMessage);\n> +\n> +\treturn;\n> +}\n> +\n> +int IPCPipeUnixSocket::call(const IPCUnixSocket::Payload &message,\n> +\t\t\t IPCUnixSocket::Payload *response, uint32_t cookie)\n> +{\n> +\tTimer timeout;\n> +\tint ret;\n> +\n> +\tcallData_[cookie].response = response;\n> +\tcallData_[cookie].done = false;\n> +\n> +\tret = socket_->send(message);\n> +\tif (ret)\n\nThis is the only error case where you don't clear callData_.\n\n> +\t\treturn ret;\n> +\n> +\ttimeout.start(2000);\n> +\twhile (!callData_[cookie].done) {\n> +\t\tif (!timeout.isRunning()) {\n> +\t\t\tLOG(IPCPipe, Error) << \"Call timeout!\";\n> +\t\t\tcallData_.erase(cookie);\n> +\t\t\treturn -ETIMEDOUT;\n> +\t\t}\n> +\n> +\t\tThread::current()->eventDispatcher()->processEvents();\n> +\t}\n> +\n> +\tcallData_.erase(cookie);\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index b7cfe0c2..cd3cf9e3 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -31,6 +31,7 @@ libcamera_sources = files([\n> 'ipa_module.cpp',\n> 'ipa_proxy.cpp',\n> 'ipc_pipe.cpp',\n> + 'ipc_pipe_unixsocket.cpp',\n> 'ipc_unixsocket.cpp',\n> 'log.cpp',\n> 'media_device.cpp',\n> -- \n> 2.27.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6B55ABD80A\n\tfor ;\n\tFri, 18 Dec 2020 16:13:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D41D561595;\n\tFri, 18 Dec 2020 17:13:48 +0100 (CET)","from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A83B76052C\n\tfor ;\n\tFri, 18 Dec 2020 17:13:47 +0100 (CET)","by mail-lf1-x144.google.com with SMTP id 23so6705444lfg.10\n\tfor ;\n\tFri, 18 Dec 2020 08:13:47 -0800 (PST)","from localhost ([185.224.57.161]) by smtp.gmail.com with ESMTPSA id\n\t197sm970457lfe.158.2020.12.18.08.13.45\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 18 Dec 2020 08:13:46 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"igm/4Mz7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=VnbmDcWHdG1BirA0BlhFbHtuWAMhXfp3mh1UmlGIiLY=;\n\tb=igm/4Mz7vMJuzh9LBhcXDinh7oNX5I3waXrrgt2moGgiNQk85anwqgyTdukHtUepyc\n\tGpkfGGBGgm21wRGhfrBwvwymKLGPIQSSbM6Vh0dNCnPc7IVr7YauAYyE1V9AjePJf4wn\n\tbjzvfEFj8P2nzeo0rTr9zW9tb0LPOMFtAQF6OHMNmFMTJShTNltioP42odnDGaf5JRBn\n\tEYRF0vKtUBI0NTjgUHyTi+4/XdEyTKWSxdpVIiWVJhhZV0RYzWuZ70tHmMwGGUYohX98\n\tLN6Z5h0z7+sgP7OcY75Ao4Sibd7eDbDbzVzMIJr2C9rD4gK9Wejghq77prpiglp+zPOR\n\tPaeg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=VnbmDcWHdG1BirA0BlhFbHtuWAMhXfp3mh1UmlGIiLY=;\n\tb=Sz5G6R8PCBMHHHA2qMHkzKowNGylMIUSfbu5etVxFz2KhLRiJ9Zxy/N6d6/uCLytwM\n\t85GOPX6l9w11b3K5q9gSsJ3JpZLWaY8ZH8a34vJM3AH7421YYXKUMMQorE2RShs+B3nz\n\twpU9BBKt1jtdWQo9gNwMoTx/jVxa0eYavDHv1fQ5bb4iX/voiZvcoS8RzRHz36HB3SGx\n\t8uRQM6XaIGxZJz6Ol8cEi/+78VmT1fY6DU6Gwt+XTTRuTQK2SOwWmtkSVpOQzh9lt9im\n\tea3l1VyicqwRDnVlSkHU8nVbk6wKYBiohRo5wrKG8FaK4Oj1RJqOjLmmx6tr0l++tGn5\n\t9iOw==","X-Gm-Message-State":"AOAM533oZ8vWIY3zrXOkx7qUgvyDlSCi4Kh/gVSTBZ/nQhvHh9LqUla/\n\tRrfbpOlSagSdOL53zLV9YvdMMg==","X-Google-Smtp-Source":"ABdhPJwZguheMva69tNyHYTO64+FVsgLovzCdBcIaH4VZ3kMhhhqD9Rjk1RfpiWJqrN69BFz9heRnQ==","X-Received":"by 2002:a19:801:: with SMTP id 1mr1695361lfi.113.1608308026888; \n\tFri, 18 Dec 2020 08:13:46 -0800 (PST)","Date":"Fri, 18 Dec 2020 17:13:44 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Paul Elder ","Message-ID":"","References":"<20201205103106.242080-1-paul.elder@ideasonboard.com>\n\t<20201205103106.242080-8-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201205103106.242080-8-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 07/23] libcamera: Add IPCPipe\n\timplementation based on unix socket","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14304,"web_url":"https://patchwork.libcamera.org/comment/14304/","msgid":"<20201222102444.GF2095@pyrite.rasen.tech>","date":"2020-12-22T10:24:44","subject":"Re: [libcamera-devel] [PATCH v5 07/23] libcamera: Add IPCPipe\n\timplementation based on unix socket","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the review.\n\nOn Fri, Dec 18, 2020 at 05:13:44PM +0100, Niklas Söderlund wrote:\n> Hi Paul,\n> \n> Thanks for your work.\n> \n> On 2020-12-05 19:30:50 +0900, Paul Elder wrote:\n> > Add an implementation of IPCPipe using unix socket.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v5:\n> > - rename IPAIPCUnixSocket to IPCPipeUnixSocket\n> > - rename ipa_ipc_unisocket.[(cpp),h] ipc_pipe_unixsocket.[(cpp),h]\n> > \n> > Changes in v4:\n> > - change snake_case to camelCase\n> > - change proc_ and socket_ to unique pointers\n> > - move inclusion of corresponding header to first in the include list\n> > - reserve message data and fds size (for sending)\n> > \n> > Changes in v3:\n> > - remove unused writeUInt32() and readUInt32()\n> > - remove redundant definition of IPAIPCUnixSocket::isValid()\n> > - remove & 0xff in writeHeader()\n> > - make readHeader, writeHeader, and eraseHeader static class functions\n> > of IPAIPCUnixSocket instead of globals\n> > \n> > Changes in v2:\n> > - specify in doxygen to skip generating documentation for\n> > IPAIPCUnixSocket\n> > ---\n> > Documentation/Doxyfile.in | 2 +\n> > .../libcamera/internal/ipc_pipe_unixsocket.h | 50 ++++++\n> > src/libcamera/ipc_pipe_unixsocket.cpp | 147 ++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 200 insertions(+)\n> > create mode 100644 include/libcamera/internal/ipc_pipe_unixsocket.h\n> > create mode 100644 src/libcamera/ipc_pipe_unixsocket.cpp\n> > \n> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > index b18b8e9c..36a0cef3 100644\n> > --- a/Documentation/Doxyfile.in\n> > +++ b/Documentation/Doxyfile.in\n> > @@ -837,8 +837,10 @@ RECURSIVE = YES\n> > EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> > \t\t\t @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \\\n> > \t\t\t @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \\\n> > +\t\t\t @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \\\n> > \t\t\t @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \\\n> > \t\t\t @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \\\n> > +\t\t\t @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \\\n> > \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> > \t\t\t @TOP_SRCDIR@/src/libcamera/proxy/ \\\n> > \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> > diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > new file mode 100644\n> > index 00000000..fea3179c\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > @@ -0,0 +1,50 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * ipc_pipe_unixsocket.h - Image Processing Algorithm IPC module using unix socket\n> > + */\n> > +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__\n> > +#define __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/ipc_pipe.h\"\n> > +#include \"libcamera/internal/ipc_unixsocket.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +class Process;\n> > +\n> > +class IPCPipeUnixSocket : public IPCPipe\n> > +{\n> > +public:\n> > +\tIPCPipeUnixSocket(const char *ipaModulePath, const char *ipaProxyWorkerPath);\n> > +\t~IPCPipeUnixSocket();\n> > +\n> > +\tint sendSync(uint32_t cmd,\n> > +\t\t const IPCMessage &in,\n> > +\t\t IPCMessage *out = nullptr) override;\n> > +\n> > +\tint sendAsync(uint32_t cmd, const IPCMessage &data) override;\n> > +\n> > +private:\n> > +\tstruct CallData {\n> > +\t\tIPCUnixSocket::Payload *response;\n> > +\t\tbool done;\n> > +\t};\n> > +\n> > +\tvoid readyRead(IPCUnixSocket *socket);\n> > +\tint call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq);\n> > +\n> > +\tuint32_t seq_;\n> > +\tstd::unique_ptr proc_;\n> > +\tstd::unique_ptr socket_;\n> > +\tstd::map callData_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__ */\n> > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp\n> > new file mode 100644\n> > index 00000000..b47c47d2\n> > --- /dev/null\n> > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> > @@ -0,0 +1,147 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * ipc_pipe_unixsocket.cpp - Image Processing Algorithm IPC module using unix socket\n> > + */\n> > +\n> > +#include \"libcamera/internal/ipc_pipe_unixsocket.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/event_dispatcher.h\"\n> > +#include \"libcamera/internal/ipc_pipe.h\"\n> > +#include \"libcamera/internal/ipc_unixsocket.h\"\n> > +#include \"libcamera/internal/log.h\"\n> > +#include \"libcamera/internal/process.h\"\n> > +#include \"libcamera/internal/thread.h\"\n> > +#include \"libcamera/internal/timer.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(IPCPipe)\n> > +\n> > +IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath,\n> > +\t\t\t\t const char *ipaProxyWorkerPath)\n> > +\t: IPCPipe(), seq_(0),\n> > +\t proc_(nullptr), socket_(nullptr)\n> \n> Is there a need to initialize proc_ and socket_ to nullptr, is not \n> std::unique_ptr<> already initialized as such?\n\nAh yes, you're right.\n\n> > +{\n> > +\tstd::vector fds;\n> > +\tstd::vector args;\n> > +\targs.push_back(ipaModulePath);\n> > +\n> > +\tsocket_ = std::make_unique();\n> > +\tint fd = socket_->create();\n> > +\tif (fd < 0) {\n> > +\t\tLOG(IPCPipe, Error) << \"Failed to create socket\";\n> > +\t\treturn;\n> > +\t}\n> > +\tsocket_->readyRead.connect(this, &IPCPipeUnixSocket::readyRead);\n> > +\targs.push_back(std::to_string(fd));\n> > +\tfds.push_back(fd);\n> > +\n> > +\tproc_ = std::make_unique();\n> > +\tint ret = proc_->start(ipaProxyWorkerPath, args, fds);\n> > +\tif (ret) {\n> > +\t\tLOG(IPCPipe, Error)\n> > +\t\t\t<< \"Failed to start proxy worker process\";\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tconnected_ = true;\n> \n> This is not checked anywhere in the code, is it a leftover from an \n> earlier design? Would it make sens to add a mandatory connect() method \n> in the base class that must be called successfully before any \n> communication is allowed?\n\nIt's used in IPCPipe's isConnected(), which is checked by proxies after\nconstructing the IPCPipe.\n\n> > +}\n> > +\n> > +IPCPipeUnixSocket::~IPCPipeUnixSocket()\n> > +{\n> > +}\n> > +\n> > +int IPCPipeUnixSocket::sendSync(uint32_t cmd,\n> > +\t\t\t\tconst IPCMessage &in, IPCMessage *out)\n> > +{\n> > +\tIPCUnixSocket::Payload message, response;\n> > +\n> > +\tconst IPCMessage::Header header{ cmd, ++seq_ };\n> > +\tmessage = in.payload(&header);\n> > +\n> > +\tint ret = call(message, &response, seq_);\n> > +\tif (ret) {\n> > +\t\tLOG(IPCPipe, Error) << \"Failed to call sync\";\n> > +\t\tcallData_.erase(seq_);\n> \n> I think you should clear callData in call() as you aready do for all but \n> one error path and drop it here.\n\nack\n\n\nThanks,\n\nPaul\n\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tif (out)\n> > +\t\t*out = IPCMessage(response);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int IPCPipeUnixSocket::sendAsync(uint32_t cmd, const IPCMessage &data)\n> > +{\n> > +\tconst IPCMessage::Header header{ cmd, 0 };\n> > +\tIPCUnixSocket::Payload message = data.payload(&header);\n> > +\n> > +\tint ret = socket_->send(message);\n> > +\tif (ret) {\n> > +\t\tLOG(IPCPipe, Error) << \"Failed to call async\";\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void IPCPipeUnixSocket::readyRead(IPCUnixSocket *socket)\n> > +{\n> > +\tIPCUnixSocket::Payload message;\n> > +\tint ret = socket->receive(&message);\n> > +\tif (ret) {\n> > +\t\tLOG(IPCPipe, Error) << \"Receive message failed\" << ret;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\t/* \\todo Use span to avoid the double copy when callData is found. */\n> > +\tIPCMessage ipcMessage(message);\n> > +\n> > +\tauto callData = callData_.find(ipcMessage.header().cookie);\n> > +\tif (callData != callData_.end()) {\n> > +\t\t*callData->second.response = std::move(message);\n> > +\t\tcallData->second.done = true;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\t/* Received unexpected data, this means it's a call from the IPA. */\n> > +\trecv.emit(ipcMessage.header().cmd, ipcMessage);\n> > +\n> > +\treturn;\n> > +}\n> > +\n> > +int IPCPipeUnixSocket::call(const IPCUnixSocket::Payload &message,\n> > +\t\t\t IPCUnixSocket::Payload *response, uint32_t cookie)\n> > +{\n> > +\tTimer timeout;\n> > +\tint ret;\n> > +\n> > +\tcallData_[cookie].response = response;\n> > +\tcallData_[cookie].done = false;\n> > +\n> > +\tret = socket_->send(message);\n> > +\tif (ret)\n> \n> This is the only error case where you don't clear callData_.\n> \n> > +\t\treturn ret;\n> > +\n> > +\ttimeout.start(2000);\n> > +\twhile (!callData_[cookie].done) {\n> > +\t\tif (!timeout.isRunning()) {\n> > +\t\t\tLOG(IPCPipe, Error) << \"Call timeout!\";\n> > +\t\t\tcallData_.erase(cookie);\n> > +\t\t\treturn -ETIMEDOUT;\n> > +\t\t}\n> > +\n> > +\t\tThread::current()->eventDispatcher()->processEvents();\n> > +\t}\n> > +\n> > +\tcallData_.erase(cookie);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index b7cfe0c2..cd3cf9e3 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -31,6 +31,7 @@ libcamera_sources = files([\n> > 'ipa_module.cpp',\n> > 'ipa_proxy.cpp',\n> > 'ipc_pipe.cpp',\n> > + 'ipc_pipe_unixsocket.cpp',\n> > 'ipc_unixsocket.cpp',\n> > 'log.cpp',\n> > 'media_device.cpp',\n> > -- \n> > 2.27.0\n> > \n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 61D26C0F1A\n\tfor ;\n\tTue, 22 Dec 2020 10:24:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D31C615AC;\n\tTue, 22 Dec 2020 11:24:55 +0100 (CET)","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 DEBEE60527\n\tfor ;\n\tTue, 22 Dec 2020 11:24:52 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F99D9E6;\n\tTue, 22 Dec 2020 11:24:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bA9oc0RK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1608632692;\n\tbh=qinwrnQ2PmWbP8xY0bdW/XvOzO/GjGtKlLpwg7bPsTY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bA9oc0RKyk7QV9l4DzJawvZcXSKANhFHBh/SzoYOF8hK4hPKJhfSetmIsyDkNMAK+\n\tjNX7s6azru21IpQFavp1nbcxCRSzuIXDwELzOtFtnVK93sHoebLsN2ZQh/KXNkwh4e\n\tPFSWW6i16bu/pcc3/cstYpRwgAWSiR3KPJ1OcZJs=","Date":"Tue, 22 Dec 2020 19:24:44 +0900","From":"paul.elder@ideasonboard.com","To":"Niklas =?iso-8859-1?q?S=F6derlund?= ","Message-ID":"<20201222102444.GF2095@pyrite.rasen.tech>","References":"<20201205103106.242080-1-paul.elder@ideasonboard.com>\n\t<20201205103106.242080-8-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 07/23] libcamera: Add IPCPipe\n\timplementation based on unix socket","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]