[{"id":15296,"web_url":"https://patchwork.libcamera.org/comment/15296/","msgid":"","date":"2021-02-23T00:39:51","subject":"Re: [libcamera-devel] [PATCH v8 2/4] tests: Add test for\n\tIPCPipeUnixSocket","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Sat, Feb 13, 2021 at 01:23:10PM +0900, Paul Elder wrote:\n> Test the IPC functions of IPCPipeUnixSocket.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Niklas Söderlund \n> \n> ---\n> No change in v8\n> \n> Changes in v7:\n> - use the new sendSync/sendAsync API\n> - remove redundant error messages\n> - use PATH_MAX instead of 100\n> \n> No change in v6\n> \n> Changes in v5:\n> - rename IPAIPCUnixSocket to IPCPipeUnixSocket\n> - use IPCMessage\n> \n> No change in v4\n> \n> Changes in v3:\n> - use readHeader, writeHeader, and eraseHeader as static class functions\n> of IPAIPCUnixSocket\n> \n> New in v2\n> ---\n> test/ipc/meson.build | 3 +-\n> test/ipc/unixsocket_ipc.cpp | 233 ++++++++++++++++++++++++++++++++++++\n> 2 files changed, 235 insertions(+), 1 deletion(-)\n> create mode 100644 test/ipc/unixsocket_ipc.cpp\n> \n> diff --git a/test/ipc/meson.build b/test/ipc/meson.build\n> index 9f413ff6..ad47b2fe 100644\n> --- a/test/ipc/meson.build\n> +++ b/test/ipc/meson.build\n> @@ -1,7 +1,8 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> ipc_tests = [\n> - ['unixsocket', 'unixsocket.cpp'],\n> + ['unixsocket_ipc', 'unixsocket_ipc.cpp'],\n> + ['unixsocket', 'unixsocket.cpp'],\n> ]\n> \n> foreach t : ipc_tests\n> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp\n> new file mode 100644\n> index 00000000..1787891b\n> --- /dev/null\n> +++ b/test/ipc/unixsocket_ipc.cpp\n> @@ -0,0 +1,233 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * unixsocket_ipc.cpp - Unix socket IPC test\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/event_dispatcher.h\"\n> +#include \"libcamera/internal/ipa_data_serializer.h\"\n> +#include \"libcamera/internal/ipc_pipe.h\"\n> +#include \"libcamera/internal/ipc_pipe_unixsocket.h\"\n> +#include \"libcamera/internal/process.h\"\n> +#include \"libcamera/internal/thread.h\"\n> +#include \"libcamera/internal/timer.h\"\n> +#include \"libcamera/internal/utils.h\"\n> +\n> +#include \"test.h\"\n> +\n> +#define CMD_EXIT\t0\n> +#define CMD_GET_SYNC\t1\n> +#define CMD_SET_ASYNC\t2\n\nI'd store this in an enum.\n\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +class UnixSocketTestIPCSlave\n> +{\n> +public:\n> +\tUnixSocketTestIPCSlave()\n> +\t\t: value_(1337), exitCode_(EXIT_FAILURE), exit_(false)\n> +\t{\n> +\t\tdispatcher_ = Thread::current()->eventDispatcher();\n> +\t\tipc_.readyRead.connect(this, &UnixSocketTestIPCSlave::readyRead);\n> +\t}\n> +\n> +\tint run(int fd)\n> +\t{\n> +\t\tif (ipc_.bind(fd)) {\n> +\t\t\tcerr << \"Failed to connect to IPC channel\" << endl;\n> +\t\t\treturn EXIT_FAILURE;\n> +\t\t}\n> +\n> +\t\twhile (!exit_)\n> +\t\t\tdispatcher_->processEvents();\n> +\n> +\t\tipc_.close();\n> +\n> +\t\treturn exitCode_;\n> +\t}\n> +\n> +private:\n> +\tvoid readyRead(IPCUnixSocket *ipc)\n> +\t{\n> +\t\tIPCUnixSocket::Payload message;\n> +\t\tint ret;\n> +\n> +\t\tret = ipc->receive(&message);\n> +\t\tif (ret) {\n> +\t\t\tcerr << \"Receive message failed: \" << ret << endl;\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tIPCMessage ipcMessage(message);\n> +\t\tuint32_t cmd = ipcMessage.header().cmd;\n> +\n> +\t\tswitch (cmd) {\n> +\t\tcase CMD_EXIT: {\n> +\t\t\texit_ = true;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tcase CMD_GET_SYNC: {\n> +\t\t\tIPCMessage::Header header = { cmd, ipcMessage.header().cookie };\n> +\t\t\tIPCMessage response(header);\n> +\n> +\t\t\tvector buf;\n> +\t\t\ttie(buf, ignore) = IPADataSerializer::serialize(value_);\n> +\t\t\tresponse.data().insert(response.data().end(), buf.begin(), buf.end());\n> +\n> +\t\t\tret = ipc_.send(response.payload());\n> +\t\t\tif (ret < 0) {\n> +\t\t\t\tcerr << \"Reply failed\" << endl;\n> +\t\t\t\tstop(ret);\n> +\t\t\t}\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tcase CMD_SET_ASYNC: {\n> +\t\t\tvalue_ = IPADataSerializer::deserialize(ipcMessage.data());\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\tvoid stop(int code)\n> +\t{\n> +\t\texitCode_ = code;\n> +\t\texit_ = true;\n> +\t}\n> +\n> +\tint32_t value_;\n> +\n> +\tIPCUnixSocket ipc_;\n> +\tEventDispatcher *dispatcher_;\n> +\tint exitCode_;\n> +\tbool exit_;\n> +};\n> +\n> +class UnixSocketTestIPC : public Test\n> +{\n> +protected:\n> +\tint init()\n> +\t{\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint setVal(int32_t val)\n\nsetValue() ?\n\n> +\t{\n> +\t\tIPCMessage msg(CMD_SET_ASYNC);\n> +\t\ttie(msg.data(), ignore) = IPADataSerializer::serialize(val);\n> +\n> +\t\tint ret = ipc_->sendAsync(msg);\n> +\t\tif (ret < 0) {\n> +\t\t\tcerr << \"Failed to call set value\" << endl;\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint getVal()\n\ngetValue() ?\n\n> +\t{\n> +\t\tIPCMessage msg(CMD_GET_SYNC);\n> +\t\tIPCMessage buf;\n> +\n> +\t\tint ret = ipc_->sendSync(msg, &buf);\n> +\t\tif (ret < 0) {\n> +\t\t\tcerr << \"Failed to call get value\" << endl;\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\treturn IPADataSerializer::deserialize(buf.data());\n> +\t}\n> +\n> +\tint exit()\n> +\t{\n> +\t\tIPCMessage msg(CMD_EXIT);\n> +\n> +\t\tint ret = ipc_->sendAsync(msg);\n> +\t\tif (ret < 0) {\n> +\t\t\tcerr << \"Failed to call exit\" << endl;\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint run()\n> +\t{\n> +\t\tchar selfpath[PATH_MAX];\n> +\t\tmemset(selfpath, 0, sizeof(selfpath));\n> +\t\tint ret = readlink(\"/proc/self/exe\", selfpath, sizeof(selfpath));\n\nreadlink returns the number of bytes, so you don't have to memset.\n\n\t\tchar selfpath[PATH_MAX];\n\t\tint ret = readlink(\"/proc/self/exe\", selfpath,\n\t\t\t\t sizeof(selfpath) - 1);\n\t\t/* error handling */\n\t\tselfpath[ret] = '\\0';\n\nThe -1 is there to ensure space for the \\0.\n\nBut do you actually need to read the link, can't you pass\n\"/proc/self/exe\" to IPCPipeUnixSocket ?\n\nI've noticed, when testing proc/self/exe directly, that isConnected()\nreturned true even if the executable can't be started (I made a typo\nwhen typing the string). This is due to the failure occurring after the\nfork(), so Process() doesn't realize. It's a bit annoying, but I'm not\nsure we could do much about it. Maybe checking that the file exists\nbefore forking would be nice, it's not 100% fullproof as it's subject to\nraces, but it would allow reporting the most common errors.\n\n> +\t\tif (ret < 0) {\n> +\t\t\tint err = errno;\n> +\t\t\tcerr << \"Failed to get path: \" << strerror(err) << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tipc_ = std::make_unique(\"\", selfpath);\n> +\t\tif (!ipc_->isConnected()) {\n> +\t\t\tcerr << \"Failed to create IPCPipe\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tret = getVal();\n> +\t\tif (ret != 1337) {\n\nMaybe the value could be defined as a constant ?\n\n> +\t\t\tcerr << \"Wrong inital value, expected 1337, got \" << ret << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tret = setVal(9001);\n> +\t\tif (ret < 0) {\n> +\t\t\tcerr << \"Failed to set value: \" << strerror(-ret) << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tret = getVal();\n> +\t\tif (ret != 9001) {\n> +\t\t\tcerr << \"Wrong set value, expected 9001, got \" << ret << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tret = exit();\n> +\t\tif (ret < 0) {\n> +\t\t\tcerr << \"Failed to exit: \" << strerror(-ret) << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +private:\n> +\tProcessManager processManager_;\n> +\n> +\tunique_ptr ipc_;\n> +};\n> +\n> +/*\n> + * Can't use TEST_REGISTER() as single binary needs to act as both proxy\n> + * master and slave.\n\nThere's no proxy here as it's just IPC, not IPA. Maybe it should then be\ncalled client and server following the usual network terminology ?\n\nReviewed-by: Laurent Pinchart \n\n> + */\n> +int main(int argc, char **argv)\n> +{\n> +\t/* IPCPipeUnixSocket passes IPA module path in argv[1] */\n> +\tif (argc == 3) {\n> +\t\tint ipcfd = std::stoi(argv[2]);\n> +\t\tUnixSocketTestIPCSlave slave;\n> +\t\treturn slave.run(ipcfd);\n> +\t}\n> +\n> +\treturn UnixSocketTestIPC().execute();\n> +}","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 ACCCBBD1F6\n\tfor ;\n\tTue, 23 Feb 2021 00:40:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 267C068A20;\n\tTue, 23 Feb 2021 01:40:20 +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 CBF2D60106\n\tfor ;\n\tTue, 23 Feb 2021 01:40:18 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 24F0966;\n\tTue, 23 Feb 2021 01:40:18 +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=\"hxidNQKR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614040818;\n\tbh=wey8loZP61hyu4ZWrMinlUEXNUml2mbbbmTL/VBbHyY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hxidNQKReS04Jd4xwJQ8jFMB/LNO0scmbiFXtj1SzQJd/F1NgrRcNcxol7u1YlM6X\n\tPvdkrTiFxkLQNVL0VXS+somdSdW/nuzuUmvckeQDdUEf8SMrIJO9duuDIrAhULOqYH\n\to3mDl9KRK5Todi6WN/0A2OjfW4eZK8eY/JvhPzfw=","Date":"Tue, 23 Feb 2021 02:39:51 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210213042312.112572-1-paul.elder@ideasonboard.com>\n\t<20210213042312.112572-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210213042312.112572-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v8 2/4] tests: Add test for\n\tIPCPipeUnixSocket","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]