[{"id":1989,"web_url":"https://patchwork.libcamera.org/comment/1989/","msgid":"","date":"2019-06-24T08:18:40","subject":"Re: [libcamera-devel] [RFC 2/2] test: ipc: unix: Add test for\n\tIPCUnixSocket","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 21/06/2019 05:15, Niklas Söderlund wrote:\n> Test that the IPC supports sending data and file descriptors over the\n> IPC medium. To be able execute the test two executables are needed, one\n> to drive the test and act as the libcamera (master) and a one to act as\n> the IPA (slave).\n> \n> The master drives the testing posting requests to the slave to process\n> and sometime respond to. A few different tests are preformed.\n> \n> - Master sends a string to the slave which responds with the reversed\n> string. The master verifies that a reversed string is indeed returned.\n> \n> - Master sends a list of file descriptors and ask the salve to calculate\n> and respond with the sum of the size of the files. The master verifies\n> that the calculate size is correct.\n> \n> - Master send a pre-computed size and a list of file descriptors and\n> ask the slave to verify that the pre-computed size matches the sum of\n> the size of the file descriptors.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> test/ipc/meson.build | 20 ++++\n> test/ipc/unixsocket-slave.cpp | 92 ++++++++++++++++\n> test/ipc/unixsocket.cpp | 200 ++++++++++++++++++++++++++++++++++\n> test/ipc/unixsocket.h | 27 +++++\n> test/meson.build | 1 +\n> 5 files changed, 340 insertions(+)\n> create mode 100644 test/ipc/meson.build\n> create mode 100644 test/ipc/unixsocket-slave.cpp\n> create mode 100644 test/ipc/unixsocket.cpp\n> create mode 100644 test/ipc/unixsocket.h\n> \n> diff --git a/test/ipc/meson.build b/test/ipc/meson.build\n> new file mode 100644\n> index 0000000000000000..0a425d4e7241c753\n> --- /dev/null\n> +++ b/test/ipc/meson.build\n> @@ -0,0 +1,20 @@\n> +# Tests are listed in order of complexity.\n> +# They are not alphabetically sorted.\n> +ipc_tests = [\n> + [ 'unixsocket', 'unixsocket.cpp', 'unixsocket-slave', 'unixsocket-slave.cpp' ],\n> +]\n> +\n> +foreach t : ipc_tests\n> + exe = executable(t[0], t[1],\n> + dependencies : libcamera_dep,\n> + link_with : test_libraries,\n> + include_directories : test_includes_internal)\n> +\n> + slave = executable(t[2], t[3],\n> + dependencies : libcamera_dep,\n> + include_directories : test_includes_internal)\n> +\n> + test(t[0], exe, suite : 'ipc', is_parallel : false)\n> +endforeach\n\n\nConstructing multiple binaries for each test sounds like it's going to\nprove somewhat awkward to validate the tests, requiring starting and\nstopping external binaries, and checking their return status.\n\nCouldn't we use threads in a single binary to contain each test?\n\n\n> +\n> +config_h.set('IPC_TEST_DIR', '\"' + meson.current_build_dir() + '\"')\n\nThis means the tests can never be executed anywhere except inside the\nbuild directory ... I'm not very fond of this at all :-S\n\n\n\n> diff --git a/test/ipc/unixsocket-slave.cpp b/test/ipc/unixsocket-slave.cpp\n> new file mode 100644\n> index 0000000000000000..ec27f6bf29823173\n> --- /dev/null\n> +++ b/test/ipc/unixsocket-slave.cpp\n> @@ -0,0 +1,92 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * unixsocket-slave.cpp - Unix socket IPC slave runner\n> + */\n> +\n> +#include \"unixsocket.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"ipc_unixsocket.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +int main(int argc, char **argv)\n> +{\n> +\tif (argc != 2) {\n> +\t\tcerr << \"usage: %s \" << endl;\n> +\t\treturn EXIT_FAILURE;\n> +\t}\n> +\n> +\tint ipcfd = std::stoi(argv[1]);\n> +\tIPCUnixSocket ipc(ipcfd);\n> +\n> +\tif (ipc.connect()) {\n> +\t\tcerr << \"Failed to connect to IPC\" << endl;\n> +\t\treturn EXIT_FAILURE;\n> +\t}\n> +\n> +\tbool run = true;\n> +\twhile (run) {\n> +\t\tint ret = 0;\n> +\t\tIPCUnixSocket::Payload payload, response;\n> +\n> +\t\tret = ipc.recv(&payload, 100);\n> +\t\tif (ret < 0) {\n> +\t\t\tif (ret == -ETIMEDOUT)\n> +\t\t\t\tcontinue;\n> +\t\t\treturn ret;\n> +\t\t}\n> +\t\tswitch (payload.priv) {\n> +\t\tcase CMD_CLOSE:\n> +\t\t\trun = false;\n> +\t\t\tbreak;\n> +\t\tcase CMD_REVERESE: {\n> +\t\t\tstd::string str(payload.data.begin(), payload.data.end());\n> +\t\t\tstd::reverse(str.begin(), str.end());\n> +\t\t\tresponse.data = std::vector(str.begin(), str.end());\n> +\t\t\tret = ipc.send(response);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\treturn ret;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase CMD_LEN_CALC: {\n> +\t\t\tint size = 0;\n> +\t\t\tfor (int fd : payload.fds)\n> +\t\t\t\tsize += calcLength(fd);\n> +\n> +\t\t\tresponse.data.resize(sizeof(size));\n> +\t\t\tmemcpy(response.data.data(), &size, sizeof(size));\n> +\t\t\tret = ipc.send(response);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\treturn ret;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase CMD_LEN_CMP: {\n> +\t\t\tint size = 0;\n> +\t\t\tfor (int fd : payload.fds)\n> +\t\t\t\tsize += calcLength(fd);\n> +\n> +\t\t\tint cmp;\n> +\t\t\tmemcpy(&cmp, payload.data.data(), sizeof(cmp));\n> +\n> +\t\t\tif (cmp != size)\n> +\t\t\t\treturn -ERANGE;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tdefault:\n> +\t\t\tcerr << \"Unkown command \" << payload.priv << endl;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t}\n> +\n> +\tipc.close();\n> +\n> +\treturn 0;\n> +}\n> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> new file mode 100644\n> index 0000000000000000..ad2609764166a852\n> --- /dev/null\n> +++ b/test/ipc/unixsocket.cpp\n> @@ -0,0 +1,200 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * unixsocket.cpp - Unix socket IPC test\n> + */\n> +\n> +#include \"unixsocket.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"ipc_unixsocket.h\"\n> +#include \"test.h\"\n> +\n> +#define MASTER_BIN IPC_TEST_DIR \"/unixsocket\"\n> +#define SLAVE_BIN IPC_TEST_DIR \"/unixsocket-slave\"\n\nWhy not just have all the code built into the same executable - and\ndecide what path to take based on the return of the fork?\n\n\nIn fact - is MASTER_BIN only ever used as test data, and a test FD?\n(While SLAVE_BIN appears to be the same usage, but also an exec()? )\n\n\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +class UnixSocketTest : public Test\n> +{\n> +protected:\n> +\tint slaveStart(int fd)\n> +\t{\n> +\t\tpid_ = fork();\n> +\n> +\t\tif (pid_ == -1)\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (!pid_) {\n> +\t\t\tstd::string arg = std::to_string(fd);\n> +\t\t\texecl(SLAVE_BIN, SLAVE_BIN, arg.c_str());\n> +\n\nOr instead of threads - if the slave code was in this binary - I think\nit could just be executed here... It would have it's own process memory\nby this point.... (or is that hacky? :S)\n\n> +\t\t\t/* Only get here if exec fails. */\n> +\t\t\texit(TestFail);\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint slaveStop()\n> +\t{\n> +\t\tint status;\n> +\n> +\t\tif (pid_ < 0)\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (waitpid(pid_, &status, 0) < 0)\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (!WIFEXITED(status) || WEXITSTATUS(status))\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint testReverse()\n> +\t{\n> +\t\tstd::string input = \"FooBar\";\n> +\t\tstd::string match = \"raBooF\";\n> +\n> +\t\tIPCUnixSocket::Payload payload, response;\n> +\n> +\t\tpayload.priv = CMD_REVERESE;\n> +\t\tpayload.data = std::vector(input.begin(), input.end());\n> +\n> +\t\tif (ipc_.call(payload, &response, 100))\n> +\t\t\treturn TestFail;\n> +\n> +\t\tstd::string output(response.data.begin(), response.data.end());\n> +\n> +\t\tif (output != match)\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint testCalc()\n> +\t{\n> +\t\tint fdM = open(MASTER_BIN, O_RDONLY);\n> +\t\tint fdS = open(SLAVE_BIN, O_RDONLY);\n> +\n> +\t\tif (fdM < 0 || fdS < 0)\n> +\t\t\treturn TestFail;\n> +\n> +\t\tint size = 0;\n> +\t\tsize += calcLength(fdM);\n> +\t\tsize += calcLength(fdS);\n> +\n> +\t\tIPCUnixSocket::Payload payload, response;\n> +\n> +\t\tpayload.priv = CMD_LEN_CALC;\n> +\t\tpayload.fds.push_back(fdM);\n> +\t\tpayload.fds.push_back(fdS);\n> +\n> +\t\tif (ipc_.call(payload, &response, 100))\n> +\t\t\treturn TestFail;\n> +\n> +\t\tint output;\n> +\t\tmemcpy(&output, response.data.data(), sizeof(output));\n> +\n> +\t\tif (output != size)\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint testCmp()\n> +\t{\n> +\t\tint fdM = open(MASTER_BIN, O_RDONLY);\n> +\t\tint fdS = open(SLAVE_BIN, O_RDONLY);\n> +\n> +\t\tif (fdM < 0 || fdS < 0)\n> +\t\t\treturn TestFail;\n> +\n> +\t\tint size = 0;\n> +\t\tsize += calcLength(fdM);\n> +\t\tsize += calcLength(fdS);\n> +\n> +\t\tIPCUnixSocket::Payload payload, response;\n> +\n> +\t\tpayload.priv = CMD_LEN_CMP;\n> +\t\tpayload.data.resize(sizeof(size));\n> +\t\tmemcpy(payload.data.data(), &size, sizeof(size));\n> +\t\tpayload.fds.push_back(fdM);\n> +\t\tpayload.fds.push_back(fdS);\n> +\n> +\t\tif (ipc_.send(payload))\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint testClose()\n> +\t{\n> +\t\tIPCUnixSocket::Payload payload;\n> +\n> +\t\tpayload.priv = CMD_CLOSE;\n> +\n> +\t\tif (ipc_.send(payload))\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint run()\n> +\t{\n> +\t\tint slavefd;\n> +\n> +\t\tslavefd = ipc_.create();\n> +\t\tif (slavefd < 0)\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (slaveStart(slavefd))\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (ipc_.connect()) {\n> +\t\t\tcerr << \"Failed to connect to IPC\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\t\tif (testReverse()) {\n> +\t\t\tcerr << \"String reverse fail\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (testCalc()) {\n> +\t\t\tcerr << \"Size calc fail\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (testCmp()) {\n> +\t\t\tcerr << \"Compare fail\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (testClose())\n> +\t\t\treturn TestFail;\n> +\n> +\t\tprintf(\"Master OK!\\n\");\n> +\n> +\t\tipc_.close();\n> +\n> +\t\tif (slaveStop())\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +private:\n> +\tpid_t pid_;\n> +\tIPCUnixSocket ipc_;\n> +};\n> +\n> +TEST_REGISTER(UnixSocketTest)\n> diff --git a/test/ipc/unixsocket.h b/test/ipc/unixsocket.h\n> new file mode 100644\n> index 0000000000000000..5ae223c76108a4f6\n> --- /dev/null\n> +++ b/test/ipc/unixsocket.h\n> @@ -0,0 +1,27 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * unixsocket.h - Unix socket IPC test\n> + *\n> + */\n> +#ifndef __LIBCAMERA_IPCUNIXSOCKET_TEST_H__\n> +#define __LIBCAMERA_IPCUNIXSOCKET_TEST_H__\n> +\n> +#include \n> +\n> +#define CMD_CLOSE 0\n> +#define CMD_REVERESE 1\n\ns/REVERESE/REVERSE/\n\n> +#define CMD_LEN_CALC 2\n> +#define CMD_LEN_CMP 3\n> +\n> +int calcLength(int fd)\n> +{\n> +\tlseek(fd, 0, 0);\n> +\tint size = lseek(fd, 0, SEEK_END);\n> +\tlseek(fd, 0, 0);\n> +\n> +\treturn size;\n> +}\n> +\n> +#endif /* __LIBCAMERA_IPCUNIXSOCKET_TEST_H__ */\n> diff --git a/test/meson.build b/test/meson.build\n> index c36ac24796367501..3666f6b2385bd4ca 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -2,6 +2,7 @@ subdir('libtest')\n> \n> subdir('camera')\n> subdir('ipa')\n> +subdir('ipc')\n> subdir('media_device')\n> subdir('pipeline')\n> subdir('stream')\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C117460C29\n\tfor ;\n\tMon, 24 Jun 2019 10:18:43 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C1CD323;\n\tMon, 24 Jun 2019 10:18:43 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561364323;\n\tbh=Qm+K3B0Kp3iIYjnV5LDs0AwF1yQtBInfjRX0dr9af/w=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=SccOnw348biuJEY92wtMUeXrN1pPCeDZUxhgxChDf2z320g1Jb+Ce4kr8FkiEWN6L\n\tYYafBWdiHmMkyYvwnNzv+Gt6fiOKrCF1cUTcS642RmvY9s+1UaOu7C2cTn9ZzuBfQk\n\tuPjpeHVemaObzS6OGShs7W5WjwqaJG2Okqv5V8bU=","Reply-To":"kieran.bingham@ideasonboard.com","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20190621041519.29689-1-niklas.soderlund@ragnatech.se>\n\t<20190621041519.29689-3-niklas.soderlund@ragnatech.se>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 24 Jun 2019 09:18:40 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190621041519.29689-3-niklas.soderlund@ragnatech.se>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC 2/2] test: ipc: unix: Add test for\n\tIPCUnixSocket","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 08:18:43 -0000"}},{"id":1996,"web_url":"https://patchwork.libcamera.org/comment/1996/","msgid":"<20190624111358.GA5737@pendragon.ideasonboard.com>","date":"2019-06-24T11:13:58","subject":"Re: [libcamera-devel] [RFC 2/2] test: ipc: unix: Add test for\n\tIPCUnixSocket","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jun 24, 2019 at 09:18:40AM +0100, Kieran Bingham wrote:\n> On 21/06/2019 05:15, Niklas Söderlund wrote:\n> > Test that the IPC supports sending data and file descriptors over the\n> > IPC medium. To be able execute the test two executables are needed, one\n> > to drive the test and act as the libcamera (master) and a one to act as\n> > the IPA (slave).\n> > \n> > The master drives the testing posting requests to the slave to process\n> > and sometime respond to. A few different tests are preformed.\n> > \n> > - Master sends a string to the slave which responds with the reversed\n> > string. The master verifies that a reversed string is indeed returned.\n> > \n> > - Master sends a list of file descriptors and ask the salve to calculate\n> > and respond with the sum of the size of the files. The master verifies\n> > that the calculate size is correct.\n> > \n> > - Master send a pre-computed size and a list of file descriptors and\n> > ask the slave to verify that the pre-computed size matches the sum of\n> > the size of the file descriptors.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > test/ipc/meson.build | 20 ++++\n> > test/ipc/unixsocket-slave.cpp | 92 ++++++++++++++++\n> > test/ipc/unixsocket.cpp | 200 ++++++++++++++++++++++++++++++++++\n> > test/ipc/unixsocket.h | 27 +++++\n> > test/meson.build | 1 +\n> > 5 files changed, 340 insertions(+)\n> > create mode 100644 test/ipc/meson.build\n> > create mode 100644 test/ipc/unixsocket-slave.cpp\n> > create mode 100644 test/ipc/unixsocket.cpp\n> > create mode 100644 test/ipc/unixsocket.h\n> > \n> > diff --git a/test/ipc/meson.build b/test/ipc/meson.build\n> > new file mode 100644\n> > index 0000000000000000..0a425d4e7241c753\n> > --- /dev/null\n> > +++ b/test/ipc/meson.build\n> > @@ -0,0 +1,20 @@\n> > +# Tests are listed in order of complexity.\n> > +# They are not alphabetically sorted.\n> > +ipc_tests = [\n> > + [ 'unixsocket', 'unixsocket.cpp', 'unixsocket-slave', 'unixsocket-slave.cpp' ],\n> > +]\n> > +\n> > +foreach t : ipc_tests\n> > + exe = executable(t[0], t[1],\n> > + dependencies : libcamera_dep,\n> > + link_with : test_libraries,\n> > + include_directories : test_includes_internal)\n> > +\n> > + slave = executable(t[2], t[3],\n> > + dependencies : libcamera_dep,\n> > + include_directories : test_includes_internal)\n> > +\n> > + test(t[0], exe, suite : 'ipc', is_parallel : false)\n> > +endforeach\n> \n> Constructing multiple binaries for each test sounds like it's going to\n> prove somewhat awkward to validate the tests, requiring starting and\n> stopping external binaries, and checking their return status.\n> \n> Couldn't we use threads in a single binary to contain each test?\n\nWe need multiple processes, IPC stands for inter process communication\n:-) We could of course use it to communicate within a single process,\nand even a single thread, but the environment being different may lead\nto different results. However, if the IPC and process management\ncomponents are well designed and their APIs don't overlap, it could\nstill be an option.\n\nNiklas, what is your opinion, could we start with an IPC test that runs\nwithin a single thread ?\n\n> > +\n> > +config_h.set('IPC_TEST_DIR', '\"' + meson.current_build_dir() + '\"')\n> \n> This means the tests can never be executed anywhere except inside the\n> build directory ... I'm not very fond of this at all :-S\n> \n> > diff --git a/test/ipc/unixsocket-slave.cpp b/test/ipc/unixsocket-slave.cpp\n> > new file mode 100644\n> > index 0000000000000000..ec27f6bf29823173\n> > --- /dev/null\n> > +++ b/test/ipc/unixsocket-slave.cpp\n> > @@ -0,0 +1,92 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * unixsocket-slave.cpp - Unix socket IPC slave runner\n> > + */\n> > +\n> > +#include \"unixsocket.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"ipc_unixsocket.h\"\n> > +\n> > +using namespace std;\n> > +using namespace libcamera;\n> > +\n> > +int main(int argc, char **argv)\n> > +{\n> > +\tif (argc != 2) {\n> > +\t\tcerr << \"usage: %s \" << endl;\n> > +\t\treturn EXIT_FAILURE;\n> > +\t}\n> > +\n> > +\tint ipcfd = std::stoi(argv[1]);\n> > +\tIPCUnixSocket ipc(ipcfd);\n> > +\n> > +\tif (ipc.connect()) {\n> > +\t\tcerr << \"Failed to connect to IPC\" << endl;\n> > +\t\treturn EXIT_FAILURE;\n> > +\t}\n> > +\n> > +\tbool run = true;\n> > +\twhile (run) {\n> > +\t\tint ret = 0;\n> > +\t\tIPCUnixSocket::Payload payload, response;\n> > +\n> > +\t\tret = ipc.recv(&payload, 100);\n> > +\t\tif (ret < 0) {\n> > +\t\t\tif (ret == -ETIMEDOUT)\n> > +\t\t\t\tcontinue;\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\t\tswitch (payload.priv) {\n> > +\t\tcase CMD_CLOSE:\n> > +\t\t\trun = false;\n> > +\t\t\tbreak;\n> > +\t\tcase CMD_REVERESE: {\n> > +\t\t\tstd::string str(payload.data.begin(), payload.data.end());\n> > +\t\t\tstd::reverse(str.begin(), str.end());\n> > +\t\t\tresponse.data = std::vector(str.begin(), str.end());\n> > +\t\t\tret = ipc.send(response);\n> > +\t\t\tif (ret < 0)\n> > +\t\t\t\treturn ret;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\tcase CMD_LEN_CALC: {\n> > +\t\t\tint size = 0;\n> > +\t\t\tfor (int fd : payload.fds)\n> > +\t\t\t\tsize += calcLength(fd);\n> > +\n> > +\t\t\tresponse.data.resize(sizeof(size));\n> > +\t\t\tmemcpy(response.data.data(), &size, sizeof(size));\n> > +\t\t\tret = ipc.send(response);\n> > +\t\t\tif (ret < 0)\n> > +\t\t\t\treturn ret;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\tcase CMD_LEN_CMP: {\n> > +\t\t\tint size = 0;\n> > +\t\t\tfor (int fd : payload.fds)\n> > +\t\t\t\tsize += calcLength(fd);\n> > +\n> > +\t\t\tint cmp;\n> > +\t\t\tmemcpy(&cmp, payload.data.data(), sizeof(cmp));\n> > +\n> > +\t\t\tif (cmp != size)\n> > +\t\t\t\treturn -ERANGE;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t\tdefault:\n> > +\t\t\tcerr << \"Unkown command \" << payload.priv << endl;\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tipc.close();\n> > +\n> > +\treturn 0;\n> > +}\n> > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> > new file mode 100644\n> > index 0000000000000000..ad2609764166a852\n> > --- /dev/null\n> > +++ b/test/ipc/unixsocket.cpp\n> > @@ -0,0 +1,200 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * unixsocket.cpp - Unix socket IPC test\n> > + */\n> > +\n> > +#include \"unixsocket.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"ipc_unixsocket.h\"\n> > +#include \"test.h\"\n> > +\n> > +#define MASTER_BIN IPC_TEST_DIR \"/unixsocket\"\n> > +#define SLAVE_BIN IPC_TEST_DIR \"/unixsocket-slave\"\n> \n> Why not just have all the code built into the same executable - and\n> decide what path to take based on the return of the fork?\n\nBecause the use case is to use IPC to communicate with another process\nafter exec(). But as stated above, we could decide to perform the basic\nIPC tests within a single thread and process, and leave the parts that\nwould depend on actually spawning another process and calling exec() to\nanother test, for latter, if the need arises.\n\n> In fact - is MASTER_BIN only ever used as test data, and a test FD?\n> (While SLAVE_BIN appears to be the same usage, but also an exec()? )\n> \n> > +\n> > +using namespace std;\n> > +using namespace libcamera;\n> > +\n> > +class UnixSocketTest : public Test\n> > +{\n> > +protected:\n> > +\tint slaveStart(int fd)\n> > +\t{\n> > +\t\tpid_ = fork();\n> > +\n> > +\t\tif (pid_ == -1)\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tif (!pid_) {\n> > +\t\t\tstd::string arg = std::to_string(fd);\n> > +\t\t\texecl(SLAVE_BIN, SLAVE_BIN, arg.c_str());\n> > +\n> \n> Or instead of threads - if the slave code was in this binary - I think\n> it could just be executed here... It would have it's own process memory\n> by this point.... (or is that hacky? :S)\n\nThat could be done, we won't be able to use TEST_REGISTER() in that case\nthough, but it could be doable. However, this would get in the way of\nbundling multiple tests in a single binary (but wouldn't be impossible\nto do either, we would probably just need a two steps dispatch\nmechanism based on command line arguments, from main() to the main\nfunction of the test, and from there to the child process code).\n\n> > +\t\t\t/* Only get here if exec fails. */\n> > +\t\t\texit(TestFail);\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tint slaveStop()\n> > +\t{\n> > +\t\tint status;\n> > +\n> > +\t\tif (pid_ < 0)\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tif (waitpid(pid_, &status, 0) < 0)\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tif (!WIFEXITED(status) || WEXITSTATUS(status))\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tint testReverse()\n> > +\t{\n> > +\t\tstd::string input = \"FooBar\";\n> > +\t\tstd::string match = \"raBooF\";\n> > +\n> > +\t\tIPCUnixSocket::Payload payload, response;\n> > +\n> > +\t\tpayload.priv = CMD_REVERESE;\n> > +\t\tpayload.data = std::vector(input.begin(), input.end());\n> > +\n> > +\t\tif (ipc_.call(payload, &response, 100))\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tstd::string output(response.data.begin(), response.data.end());\n> > +\n> > +\t\tif (output != match)\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tint testCalc()\n> > +\t{\n> > +\t\tint fdM = open(MASTER_BIN, O_RDONLY);\n> > +\t\tint fdS = open(SLAVE_BIN, O_RDONLY);\n> > +\n> > +\t\tif (fdM < 0 || fdS < 0)\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tint size = 0;\n> > +\t\tsize += calcLength(fdM);\n> > +\t\tsize += calcLength(fdS);\n> > +\n> > +\t\tIPCUnixSocket::Payload payload, response;\n> > +\n> > +\t\tpayload.priv = CMD_LEN_CALC;\n> > +\t\tpayload.fds.push_back(fdM);\n> > +\t\tpayload.fds.push_back(fdS);\n> > +\n> > +\t\tif (ipc_.call(payload, &response, 100))\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tint output;\n> > +\t\tmemcpy(&output, response.data.data(), sizeof(output));\n> > +\n> > +\t\tif (output != size)\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tint testCmp()\n> > +\t{\n> > +\t\tint fdM = open(MASTER_BIN, O_RDONLY);\n> > +\t\tint fdS = open(SLAVE_BIN, O_RDONLY);\n> > +\n> > +\t\tif (fdM < 0 || fdS < 0)\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tint size = 0;\n> > +\t\tsize += calcLength(fdM);\n> > +\t\tsize += calcLength(fdS);\n> > +\n> > +\t\tIPCUnixSocket::Payload payload, response;\n> > +\n> > +\t\tpayload.priv = CMD_LEN_CMP;\n> > +\t\tpayload.data.resize(sizeof(size));\n> > +\t\tmemcpy(payload.data.data(), &size, sizeof(size));\n> > +\t\tpayload.fds.push_back(fdM);\n> > +\t\tpayload.fds.push_back(fdS);\n> > +\n> > +\t\tif (ipc_.send(payload))\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tint testClose()\n> > +\t{\n> > +\t\tIPCUnixSocket::Payload payload;\n> > +\n> > +\t\tpayload.priv = CMD_CLOSE;\n> > +\n> > +\t\tif (ipc_.send(payload))\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tint run()\n> > +\t{\n> > +\t\tint slavefd;\n> > +\n> > +\t\tslavefd = ipc_.create();\n> > +\t\tif (slavefd < 0)\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tif (slaveStart(slavefd))\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tif (ipc_.connect()) {\n> > +\t\t\tcerr << \"Failed to connect to IPC\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\t\tif (testReverse()) {\n> > +\t\t\tcerr << \"String reverse fail\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (testCalc()) {\n> > +\t\t\tcerr << \"Size calc fail\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (testCmp()) {\n> > +\t\t\tcerr << \"Compare fail\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (testClose())\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tprintf(\"Master OK!\\n\");\n> > +\n> > +\t\tipc_.close();\n> > +\n> > +\t\tif (slaveStop())\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +private:\n> > +\tpid_t pid_;\n> > +\tIPCUnixSocket ipc_;\n> > +};\n> > +\n> > +TEST_REGISTER(UnixSocketTest)\n> > diff --git a/test/ipc/unixsocket.h b/test/ipc/unixsocket.h\n> > new file mode 100644\n> > index 0000000000000000..5ae223c76108a4f6\n> > --- /dev/null\n> > +++ b/test/ipc/unixsocket.h\n> > @@ -0,0 +1,27 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * unixsocket.h - Unix socket IPC test\n> > + *\n> > + */\n> > +#ifndef __LIBCAMERA_IPCUNIXSOCKET_TEST_H__\n> > +#define __LIBCAMERA_IPCUNIXSOCKET_TEST_H__\n> > +\n> > +#include \n> > +\n> > +#define CMD_CLOSE 0\n> > +#define CMD_REVERESE 1\n> \n> s/REVERESE/REVERSE/\n> \n> > +#define CMD_LEN_CALC 2\n> > +#define CMD_LEN_CMP 3\n> > +\n> > +int calcLength(int fd)\n> > +{\n> > +\tlseek(fd, 0, 0);\n> > +\tint size = lseek(fd, 0, SEEK_END);\n> > +\tlseek(fd, 0, 0);\n> > +\n> > +\treturn size;\n> > +}\n> > +\n> > +#endif /* __LIBCAMERA_IPCUNIXSOCKET_TEST_H__ */\n> > diff --git a/test/meson.build b/test/meson.build\n> > index c36ac24796367501..3666f6b2385bd4ca 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -2,6 +2,7 @@ subdir('libtest')\n> > \n> > subdir('camera')\n> > subdir('ipa')\n> > +subdir('ipc')\n> > subdir('media_device')\n> > subdir('pipeline')\n> > subdir('stream')","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3235600EB\n\tfor ;\n\tMon, 24 Jun 2019 13:14:18 +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 3C243323;\n\tMon, 24 Jun 2019 13:14:18 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561374858;\n\tbh=rh9iDQlpSWNXufh+Aol5j6214Q0fW9JBb490sq9KGvo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lpH15IVyT9c7ilYPTcA2kyXQ0MZSAIM8DIgH057asGidlq4bc+QSdeEJF0/HVF9VA\n\tgo8N8aYCQrOEPh5cjTPCrEosjTNzan9Xd3gnF+vTCyFPbzrKeY8LhF6G6vhm9GXJ4h\n\tZHdxqqjr57/QzIJAR5N84zo4YjJuKo3yBYZBE/zk=","Date":"Mon, 24 Jun 2019 14:13:58 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190624111358.GA5737@pendragon.ideasonboard.com>","References":"<20190621041519.29689-1-niklas.soderlund@ragnatech.se>\n\t<20190621041519.29689-3-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 2/2] test: ipc: unix: Add test for\n\tIPCUnixSocket","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 11:14:19 -0000"}},{"id":2003,"web_url":"https://patchwork.libcamera.org/comment/2003/","msgid":"<20190624130005.GH32581@bigcity.dyn.berto.se>","date":"2019-06-24T13:00:05","subject":"Re: [libcamera-devel] [RFC 2/2] test: ipc: unix: Add test for\n\tIPCUnixSocket","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"On 2019-06-24 14:13:58 +0300, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Mon, Jun 24, 2019 at 09:18:40AM +0100, Kieran Bingham wrote:\n> > On 21/06/2019 05:15, Niklas Söderlund wrote:\n> > > Test that the IPC supports sending data and file descriptors over the\n> > > IPC medium. To be able execute the test two executables are needed, one\n> > > to drive the test and act as the libcamera (master) and a one to act as\n> > > the IPA (slave).\n> > > \n> > > The master drives the testing posting requests to the slave to process\n> > > and sometime respond to. A few different tests are preformed.\n> > > \n> > > - Master sends a string to the slave which responds with the reversed\n> > > string. The master verifies that a reversed string is indeed returned.\n> > > \n> > > - Master sends a list of file descriptors and ask the salve to calculate\n> > > and respond with the sum of the size of the files. The master verifies\n> > > that the calculate size is correct.\n> > > \n> > > - Master send a pre-computed size and a list of file descriptors and\n> > > ask the slave to verify that the pre-computed size matches the sum of\n> > > the size of the file descriptors.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > test/ipc/meson.build | 20 ++++\n> > > test/ipc/unixsocket-slave.cpp | 92 ++++++++++++++++\n> > > test/ipc/unixsocket.cpp | 200 ++++++++++++++++++++++++++++++++++\n> > > test/ipc/unixsocket.h | 27 +++++\n> > > test/meson.build | 1 +\n> > > 5 files changed, 340 insertions(+)\n> > > create mode 100644 test/ipc/meson.build\n> > > create mode 100644 test/ipc/unixsocket-slave.cpp\n> > > create mode 100644 test/ipc/unixsocket.cpp\n> > > create mode 100644 test/ipc/unixsocket.h\n> > > \n> > > diff --git a/test/ipc/meson.build b/test/ipc/meson.build\n> > > new file mode 100644\n> > > index 0000000000000000..0a425d4e7241c753\n> > > --- /dev/null\n> > > +++ b/test/ipc/meson.build\n> > > @@ -0,0 +1,20 @@\n> > > +# Tests are listed in order of complexity.\n> > > +# They are not alphabetically sorted.\n> > > +ipc_tests = [\n> > > + [ 'unixsocket', 'unixsocket.cpp', 'unixsocket-slave', 'unixsocket-slave.cpp' ],\n> > > +]\n> > > +\n> > > +foreach t : ipc_tests\n> > > + exe = executable(t[0], t[1],\n> > > + dependencies : libcamera_dep,\n> > > + link_with : test_libraries,\n> > > + include_directories : test_includes_internal)\n> > > +\n> > > + slave = executable(t[2], t[3],\n> > > + dependencies : libcamera_dep,\n> > > + include_directories : test_includes_internal)\n> > > +\n> > > + test(t[0], exe, suite : 'ipc', is_parallel : false)\n> > > +endforeach\n> > \n> > Constructing multiple binaries for each test sounds like it's going to\n> > prove somewhat awkward to validate the tests, requiring starting and\n> > stopping external binaries, and checking their return status.\n> > \n> > Couldn't we use threads in a single binary to contain each test?\n> \n> We need multiple processes, IPC stands for inter process communication\n> :-) We could of course use it to communicate within a single process,\n> and even a single thread, but the environment being different may lead\n> to different results. However, if the IPC and process management\n> components are well designed and their APIs don't overlap, it could\n> still be an option.\n> \n> Niklas, what is your opinion, could we start with an IPC test that runs\n> within a single thread ?\n\nI think it's a good idea to have test cases which run as close as \npossible to the real thing. I really think this should be two \nexecutables, if we test IPC between two threads we might not catch an \nissue in time.\n\nThe idea is to use the process manager once it's ready in this test case \nto setup and monitor the proxy slave so I don't see any cost maintaining \nmonitoring of the slave process.\n\nOn a final note I see no real problem with having hardcoded pathes in \nthe binaries. This is test cases not intended for users but to sanity \ncheck the code base. It would be nice if they where not needed but I see \nno harm in having them.\n\n> \n> > > +\n> > > +config_h.set('IPC_TEST_DIR', '\"' + meson.current_build_dir() + '\"')\n> > \n> > This means the tests can never be executed anywhere except inside the\n> > build directory ... I'm not very fond of this at all :-S\n> > \n> > > diff --git a/test/ipc/unixsocket-slave.cpp b/test/ipc/unixsocket-slave.cpp\n> > > new file mode 100644\n> > > index 0000000000000000..ec27f6bf29823173\n> > > --- /dev/null\n> > > +++ b/test/ipc/unixsocket-slave.cpp\n> > > @@ -0,0 +1,92 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * unixsocket-slave.cpp - Unix socket IPC slave runner\n> > > + */\n> > > +\n> > > +#include \"unixsocket.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"ipc_unixsocket.h\"\n> > > +\n> > > +using namespace std;\n> > > +using namespace libcamera;\n> > > +\n> > > +int main(int argc, char **argv)\n> > > +{\n> > > +\tif (argc != 2) {\n> > > +\t\tcerr << \"usage: %s \" << endl;\n> > > +\t\treturn EXIT_FAILURE;\n> > > +\t}\n> > > +\n> > > +\tint ipcfd = std::stoi(argv[1]);\n> > > +\tIPCUnixSocket ipc(ipcfd);\n> > > +\n> > > +\tif (ipc.connect()) {\n> > > +\t\tcerr << \"Failed to connect to IPC\" << endl;\n> > > +\t\treturn EXIT_FAILURE;\n> > > +\t}\n> > > +\n> > > +\tbool run = true;\n> > > +\twhile (run) {\n> > > +\t\tint ret = 0;\n> > > +\t\tIPCUnixSocket::Payload payload, response;\n> > > +\n> > > +\t\tret = ipc.recv(&payload, 100);\n> > > +\t\tif (ret < 0) {\n> > > +\t\t\tif (ret == -ETIMEDOUT)\n> > > +\t\t\t\tcontinue;\n> > > +\t\t\treturn ret;\n> > > +\t\t}\n> > > +\t\tswitch (payload.priv) {\n> > > +\t\tcase CMD_CLOSE:\n> > > +\t\t\trun = false;\n> > > +\t\t\tbreak;\n> > > +\t\tcase CMD_REVERESE: {\n> > > +\t\t\tstd::string str(payload.data.begin(), payload.data.end());\n> > > +\t\t\tstd::reverse(str.begin(), str.end());\n> > > +\t\t\tresponse.data = std::vector(str.begin(), str.end());\n> > > +\t\t\tret = ipc.send(response);\n> > > +\t\t\tif (ret < 0)\n> > > +\t\t\t\treturn ret;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t\tcase CMD_LEN_CALC: {\n> > > +\t\t\tint size = 0;\n> > > +\t\t\tfor (int fd : payload.fds)\n> > > +\t\t\t\tsize += calcLength(fd);\n> > > +\n> > > +\t\t\tresponse.data.resize(sizeof(size));\n> > > +\t\t\tmemcpy(response.data.data(), &size, sizeof(size));\n> > > +\t\t\tret = ipc.send(response);\n> > > +\t\t\tif (ret < 0)\n> > > +\t\t\t\treturn ret;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t\tcase CMD_LEN_CMP: {\n> > > +\t\t\tint size = 0;\n> > > +\t\t\tfor (int fd : payload.fds)\n> > > +\t\t\t\tsize += calcLength(fd);\n> > > +\n> > > +\t\t\tint cmp;\n> > > +\t\t\tmemcpy(&cmp, payload.data.data(), sizeof(cmp));\n> > > +\n> > > +\t\t\tif (cmp != size)\n> > > +\t\t\t\treturn -ERANGE;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t\tdefault:\n> > > +\t\t\tcerr << \"Unkown command \" << payload.priv << endl;\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tipc.close();\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> > > new file mode 100644\n> > > index 0000000000000000..ad2609764166a852\n> > > --- /dev/null\n> > > +++ b/test/ipc/unixsocket.cpp\n> > > @@ -0,0 +1,200 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * unixsocket.cpp - Unix socket IPC test\n> > > + */\n> > > +\n> > > +#include \"unixsocket.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"ipc_unixsocket.h\"\n> > > +#include \"test.h\"\n> > > +\n> > > +#define MASTER_BIN IPC_TEST_DIR \"/unixsocket\"\n> > > +#define SLAVE_BIN IPC_TEST_DIR \"/unixsocket-slave\"\n> > \n> > Why not just have all the code built into the same executable - and\n> > decide what path to take based on the return of the fork?\n> \n> Because the use case is to use IPC to communicate with another process\n> after exec(). But as stated above, we could decide to perform the basic\n> IPC tests within a single thread and process, and leave the parts that\n> would depend on actually spawning another process and calling exec() to\n> another test, for latter, if the need arises.\n> \n> > In fact - is MASTER_BIN only ever used as test data, and a test FD?\n> > (While SLAVE_BIN appears to be the same usage, but also an exec()? )\n\nYes for the RFC I just needed to file descriptors to test with, why not \nuse whats already there? We could generate test files with dd as a build \nstep but if that seems like a better idea?\n\n> > \n> > > +\n> > > +using namespace std;\n> > > +using namespace libcamera;\n> > > +\n> > > +class UnixSocketTest : public Test\n> > > +{\n> > > +protected:\n> > > +\tint slaveStart(int fd)\n> > > +\t{\n> > > +\t\tpid_ = fork();\n> > > +\n> > > +\t\tif (pid_ == -1)\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\tif (!pid_) {\n> > > +\t\t\tstd::string arg = std::to_string(fd);\n> > > +\t\t\texecl(SLAVE_BIN, SLAVE_BIN, arg.c_str());\n> > > +\n> > \n> > Or instead of threads - if the slave code was in this binary - I think\n> > it could just be executed here... It would have it's own process memory\n> > by this point.... (or is that hacky? :S)\n> \n> That could be done, we won't be able to use TEST_REGISTER() in that case\n> though, but it could be doable. However, this would get in the way of\n> bundling multiple tests in a single binary (but wouldn't be impossible\n> to do either, we would probably just need a two steps dispatch\n> mechanism based on command line arguments, from main() to the main\n> function of the test, and from there to the child process code).\n> \n> > > +\t\t\t/* Only get here if exec fails. */\n> > > +\t\t\texit(TestFail);\n> > > +\t\t}\n> > > +\n> > > +\t\treturn TestPass;\n> > > +\t}\n> > > +\n> > > +\tint slaveStop()\n> > > +\t{\n> > > +\t\tint status;\n> > > +\n> > > +\t\tif (pid_ < 0)\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\tif (waitpid(pid_, &status, 0) < 0)\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\tif (!WIFEXITED(status) || WEXITSTATUS(status))\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\treturn TestPass;\n> > > +\t}\n> > > +\n> > > +\tint testReverse()\n> > > +\t{\n> > > +\t\tstd::string input = \"FooBar\";\n> > > +\t\tstd::string match = \"raBooF\";\n> > > +\n> > > +\t\tIPCUnixSocket::Payload payload, response;\n> > > +\n> > > +\t\tpayload.priv = CMD_REVERESE;\n> > > +\t\tpayload.data = std::vector(input.begin(), input.end());\n> > > +\n> > > +\t\tif (ipc_.call(payload, &response, 100))\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\tstd::string output(response.data.begin(), response.data.end());\n> > > +\n> > > +\t\tif (output != match)\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\tint testCalc()\n> > > +\t{\n> > > +\t\tint fdM = open(MASTER_BIN, O_RDONLY);\n> > > +\t\tint fdS = open(SLAVE_BIN, O_RDONLY);\n> > > +\n> > > +\t\tif (fdM < 0 || fdS < 0)\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\tint size = 0;\n> > > +\t\tsize += calcLength(fdM);\n> > > +\t\tsize += calcLength(fdS);\n> > > +\n> > > +\t\tIPCUnixSocket::Payload payload, response;\n> > > +\n> > > +\t\tpayload.priv = CMD_LEN_CALC;\n> > > +\t\tpayload.fds.push_back(fdM);\n> > > +\t\tpayload.fds.push_back(fdS);\n> > > +\n> > > +\t\tif (ipc_.call(payload, &response, 100))\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\tint output;\n> > > +\t\tmemcpy(&output, response.data.data(), sizeof(output));\n> > > +\n> > > +\t\tif (output != size)\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\tint testCmp()\n> > > +\t{\n> > > +\t\tint fdM = open(MASTER_BIN, O_RDONLY);\n> > > +\t\tint fdS = open(SLAVE_BIN, O_RDONLY);\n> > > +\n> > > +\t\tif (fdM < 0 || fdS < 0)\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\tint size = 0;\n> > > +\t\tsize += calcLength(fdM);\n> > > +\t\tsize += calcLength(fdS);\n> > > +\n> > > +\t\tIPCUnixSocket::Payload payload, response;\n> > > +\n> > > +\t\tpayload.priv = CMD_LEN_CMP;\n> > > +\t\tpayload.data.resize(sizeof(size));\n> > > +\t\tmemcpy(payload.data.data(), &size, sizeof(size));\n> > > +\t\tpayload.fds.push_back(fdM);\n> > > +\t\tpayload.fds.push_back(fdS);\n> > > +\n> > > +\t\tif (ipc_.send(payload))\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\tint testClose()\n> > > +\t{\n> > > +\t\tIPCUnixSocket::Payload payload;\n> > > +\n> > > +\t\tpayload.priv = CMD_CLOSE;\n> > > +\n> > > +\t\tif (ipc_.send(payload))\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\tint run()\n> > > +\t{\n> > > +\t\tint slavefd;\n> > > +\n> > > +\t\tslavefd = ipc_.create();\n> > > +\t\tif (slavefd < 0)\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\tif (slaveStart(slavefd))\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\tif (ipc_.connect()) {\n> > > +\t\t\tcerr << \"Failed to connect to IPC\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\t\tif (testReverse()) {\n> > > +\t\t\tcerr << \"String reverse fail\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (testCalc()) {\n> > > +\t\t\tcerr << \"Size calc fail\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (testCmp()) {\n> > > +\t\t\tcerr << \"Compare fail\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (testClose())\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\tprintf(\"Master OK!\\n\");\n> > > +\n> > > +\t\tipc_.close();\n> > > +\n> > > +\t\tif (slaveStop())\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\treturn TestPass;\n> > > +\t}\n> > > +\n> > > +private:\n> > > +\tpid_t pid_;\n> > > +\tIPCUnixSocket ipc_;\n> > > +};\n> > > +\n> > > +TEST_REGISTER(UnixSocketTest)\n> > > diff --git a/test/ipc/unixsocket.h b/test/ipc/unixsocket.h\n> > > new file mode 100644\n> > > index 0000000000000000..5ae223c76108a4f6\n> > > --- /dev/null\n> > > +++ b/test/ipc/unixsocket.h\n> > > @@ -0,0 +1,27 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * unixsocket.h - Unix socket IPC test\n> > > + *\n> > > + */\n> > > +#ifndef __LIBCAMERA_IPCUNIXSOCKET_TEST_H__\n> > > +#define __LIBCAMERA_IPCUNIXSOCKET_TEST_H__\n> > > +\n> > > +#include \n> > > +\n> > > +#define CMD_CLOSE 0\n> > > +#define CMD_REVERESE 1\n> > \n> > s/REVERESE/REVERSE/\n> > \n> > > +#define CMD_LEN_CALC 2\n> > > +#define CMD_LEN_CMP 3\n> > > +\n> > > +int calcLength(int fd)\n> > > +{\n> > > +\tlseek(fd, 0, 0);\n> > > +\tint size = lseek(fd, 0, SEEK_END);\n> > > +\tlseek(fd, 0, 0);\n> > > +\n> > > +\treturn size;\n> > > +}\n> > > +\n> > > +#endif /* __LIBCAMERA_IPCUNIXSOCKET_TEST_H__ */\n> > > diff --git a/test/meson.build b/test/meson.build\n> > > index c36ac24796367501..3666f6b2385bd4ca 100644\n> > > --- a/test/meson.build\n> > > +++ b/test/meson.build\n> > > @@ -2,6 +2,7 @@ subdir('libtest')\n> > > \n> > > subdir('camera')\n> > > subdir('ipa')\n> > > +subdir('ipc')\n> > > subdir('media_device')\n> > > subdir('pipeline')\n> > > subdir('stream')\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1317661580\n\tfor ;\n\tMon, 24 Jun 2019 15:00:08 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id 131so12530736ljf.4\n\tfor ;\n\tMon, 24 Jun 2019 06:00:08 -0700 (PDT)","from localhost (customer-145-14-112-32.stosn.net. [145.14.112.32])\n\tby smtp.gmail.com with ESMTPSA id\n\tr20sm1268338ljr.20.2019.06.24.06.00.06\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tMon, 24 Jun 2019 06:00:06 -0700 (PDT)"],"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\t:user-agent; bh=cTliibPNzTDUQ4zVHUDFQCAFHjjuEwDp65ac6XERPhY=;\n\tb=MZWy26+ls87CP7YOx4S81KclkwmzHDhZl75jUhpPF7hfY1l4yB/SXJDHmAg4nw4qSg\n\tjaiRR8FQqsW/foMsX7RiZojt3MWdO7o9Gvg73jp86mDpIS68WLuGwg/wWtBbIOCwSTjs\n\ted7t0+N0k+CcaUTu1Aczn6VK3vVCVKg3cMQNRPC+gcLdg7Bgcweym76BfGYjOuBRIGbU\n\tsTL/93riBaprarT5gchkPcUqNSeDffSYlLnGF9aL2Uj40YhCjLsFJVHOOSLCU6NqULPg\n\tARdqRX5urbFeEd3295iyKmKJvZoPaxw4vAHGd9bJKzI4gEvYKtj5OwV+a1kKCW6iGckZ\n\tl44g==","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:user-agent;\n\tbh=cTliibPNzTDUQ4zVHUDFQCAFHjjuEwDp65ac6XERPhY=;\n\tb=ARIpcrgoV0rEqjQ5T2FW3CsImGpr1EGuL2NGVxXbUOpE7Mi/y+I/cmyVptsJiSfYHo\n\tdyPYrHDzFdSZt+GhkqfFhL237002+cWm1hb1R1hanISRl62NxgSi42nwEsF61ePw+VD5\n\tWeAKfE3VU84o6EXp82j/anfg5RrK1HANzDqJpZ5/rqjM6RWeQdP1Vy1NfIw9P9T2Nqz2\n\tl11g/KSe9qrJJilWu64GGoKpmG9Zqk6Q0pfVs/JVMjsSo1Y83MoTrawavmpbYSHnIVd3\n\toMEQdltxgVs3ZwdQADvlgQ29FXTazP4qKW/NuBkzYx8kRgujUgWtXjQlHVZiqfkoE4xR\n\tH15Q==","X-Gm-Message-State":"APjAAAWgGv7ZPk0xaLZGFfJXZL/TZOhgA1OXc2UeC3fScSI/UnDKVvdb\n\tnu0bUoVHcnuPpSuaUuM74ps1WA==","X-Google-Smtp-Source":"APXvYqw/EUQLplyy5hTP+6cOIEEGjnBVk0sio/dN1PjF6EjnNIcHPa4Na1vbwgaK6p731YO6G+Txwg==","X-Received":"by 2002:a2e:970a:: with SMTP id\n\tr10mr21293947lji.115.1561381207141; \n\tMon, 24 Jun 2019 06:00:07 -0700 (PDT)","Date":"Mon, 24 Jun 2019 15:00:05 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190624130005.GH32581@bigcity.dyn.berto.se>","References":"<20190621041519.29689-1-niklas.soderlund@ragnatech.se>\n\t<20190621041519.29689-3-niklas.soderlund@ragnatech.se>\n\t\n\t<20190624111358.GA5737@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190624111358.GA5737@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [RFC 2/2] test: ipc: unix: Add test for\n\tIPCUnixSocket","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 13:00:08 -0000"}},{"id":2004,"web_url":"https://patchwork.libcamera.org/comment/2004/","msgid":"<20190624152001.GL5737@pendragon.ideasonboard.com>","date":"2019-06-24T15:20:01","subject":"Re: [libcamera-devel] [RFC 2/2] test: ipc: unix: Add test for\n\tIPCUnixSocket","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Mon, Jun 24, 2019 at 03:00:05PM +0200, Niklas Söderlund wrote:\n> On 2019-06-24 14:13:58 +0300, Laurent Pinchart wrote:\n> > On Mon, Jun 24, 2019 at 09:18:40AM +0100, Kieran Bingham wrote:\n> >> On 21/06/2019 05:15, Niklas Söderlund wrote:\n> >>> Test that the IPC supports sending data and file descriptors over the\n> >>> IPC medium. To be able execute the test two executables are needed, one\n> >>> to drive the test and act as the libcamera (master) and a one to act as\n> >>> the IPA (slave).\n> >>> \n> >>> The master drives the testing posting requests to the slave to process\n> >>> and sometime respond to. A few different tests are preformed.\n> >>> \n> >>> - Master sends a string to the slave which responds with the reversed\n> >>> string. The master verifies that a reversed string is indeed returned.\n> >>> \n> >>> - Master sends a list of file descriptors and ask the salve to calculate\n> >>> and respond with the sum of the size of the files. The master verifies\n> >>> that the calculate size is correct.\n> >>> \n> >>> - Master send a pre-computed size and a list of file descriptors and\n> >>> ask the slave to verify that the pre-computed size matches the sum of\n> >>> the size of the file descriptors.\n> >>> \n> >>> Signed-off-by: Niklas Söderlund \n> >>> ---\n> >>> test/ipc/meson.build | 20 ++++\n> >>> test/ipc/unixsocket-slave.cpp | 92 ++++++++++++++++\n> >>> test/ipc/unixsocket.cpp | 200 ++++++++++++++++++++++++++++++++++\n> >>> test/ipc/unixsocket.h | 27 +++++\n> >>> test/meson.build | 1 +\n> >>> 5 files changed, 340 insertions(+)\n> >>> create mode 100644 test/ipc/meson.build\n> >>> create mode 100644 test/ipc/unixsocket-slave.cpp\n> >>> create mode 100644 test/ipc/unixsocket.cpp\n> >>> create mode 100644 test/ipc/unixsocket.h\n> >>> \n> >>> diff --git a/test/ipc/meson.build b/test/ipc/meson.build\n> >>> new file mode 100644\n> >>> index 0000000000000000..0a425d4e7241c753\n> >>> --- /dev/null\n> >>> +++ b/test/ipc/meson.build\n> >>> @@ -0,0 +1,20 @@\n> >>> +# Tests are listed in order of complexity.\n> >>> +# They are not alphabetically sorted.\n> >>> +ipc_tests = [\n> >>> + [ 'unixsocket', 'unixsocket.cpp', 'unixsocket-slave', 'unixsocket-slave.cpp' ],\n> >>> +]\n> >>> +\n> >>> +foreach t : ipc_tests\n> >>> + exe = executable(t[0], t[1],\n> >>> + dependencies : libcamera_dep,\n> >>> + link_with : test_libraries,\n> >>> + include_directories : test_includes_internal)\n> >>> +\n> >>> + slave = executable(t[2], t[3],\n> >>> + dependencies : libcamera_dep,\n> >>> + include_directories : test_includes_internal)\n> >>> +\n> >>> + test(t[0], exe, suite : 'ipc', is_parallel : false)\n> >>> +endforeach\n> >> \n> >> Constructing multiple binaries for each test sounds like it's going to\n> >> prove somewhat awkward to validate the tests, requiring starting and\n> >> stopping external binaries, and checking their return status.\n> >> \n> >> Couldn't we use threads in a single binary to contain each test?\n> > \n> > We need multiple processes, IPC stands for inter process communication\n> > :-) We could of course use it to communicate within a single process,\n> > and even a single thread, but the environment being different may lead\n> > to different results. However, if the IPC and process management\n> > components are well designed and their APIs don't overlap, it could\n> > still be an option.\n> > \n> > Niklas, what is your opinion, could we start with an IPC test that runs\n> > within a single thread ?\n> \n> I think it's a good idea to have test cases which run as close as \n> possible to the real thing. I really think this should be two \n> executables, if we test IPC between two threads we might not catch an \n> issue in time.\n\nIt can also one executable with two processes, it can re-execute itself\nin the child process with a command line argument to turn child mode on.\n\n> The idea is to use the process manager once it's ready in this test case \n> to setup and monitor the proxy slave so I don't see any cost maintaining \n> monitoring of the slave process.\n> \n> On a final note I see no real problem with having hardcoded pathes in \n> the binaries. This is test cases not intended for users but to sanity \n> check the code base. It would be nice if they where not needed but I see \n> no harm in having them.\n\n>From an automated testing point of view, tests may run on a different\nmachine than they are built, so it would be good to be able to install\nthem.\n\n> >>> +\n> >>> +config_h.set('IPC_TEST_DIR', '\"' + meson.current_build_dir() + '\"')\n> >> \n> >> This means the tests can never be executed anywhere except inside the\n> >> build directory ... I'm not very fond of this at all :-S\n> >> \n> >>> diff --git a/test/ipc/unixsocket-slave.cpp b/test/ipc/unixsocket-slave.cpp\n> >>> new file mode 100644\n> >>> index 0000000000000000..ec27f6bf29823173\n> >>> --- /dev/null\n> >>> +++ b/test/ipc/unixsocket-slave.cpp\n> >>> @@ -0,0 +1,92 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2019, Google Inc.\n> >>> + *\n> >>> + * unixsocket-slave.cpp - Unix socket IPC slave runner\n> >>> + */\n> >>> +\n> >>> +#include \"unixsocket.h\"\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +#include \"ipc_unixsocket.h\"\n> >>> +\n> >>> +using namespace std;\n> >>> +using namespace libcamera;\n> >>> +\n> >>> +int main(int argc, char **argv)\n> >>> +{\n> >>> +\tif (argc != 2) {\n> >>> +\t\tcerr << \"usage: %s \" << endl;\n> >>> +\t\treturn EXIT_FAILURE;\n> >>> +\t}\n> >>> +\n> >>> +\tint ipcfd = std::stoi(argv[1]);\n> >>> +\tIPCUnixSocket ipc(ipcfd);\n> >>> +\n> >>> +\tif (ipc.connect()) {\n> >>> +\t\tcerr << \"Failed to connect to IPC\" << endl;\n> >>> +\t\treturn EXIT_FAILURE;\n> >>> +\t}\n> >>> +\n> >>> +\tbool run = true;\n> >>> +\twhile (run) {\n> >>> +\t\tint ret = 0;\n> >>> +\t\tIPCUnixSocket::Payload payload, response;\n> >>> +\n> >>> +\t\tret = ipc.recv(&payload, 100);\n> >>> +\t\tif (ret < 0) {\n> >>> +\t\t\tif (ret == -ETIMEDOUT)\n> >>> +\t\t\t\tcontinue;\n> >>> +\t\t\treturn ret;\n> >>> +\t\t}\n> >>> +\t\tswitch (payload.priv) {\n> >>> +\t\tcase CMD_CLOSE:\n> >>> +\t\t\trun = false;\n> >>> +\t\t\tbreak;\n> >>> +\t\tcase CMD_REVERESE: {\n> >>> +\t\t\tstd::string str(payload.data.begin(), payload.data.end());\n> >>> +\t\t\tstd::reverse(str.begin(), str.end());\n> >>> +\t\t\tresponse.data = std::vector(str.begin(), str.end());\n> >>> +\t\t\tret = ipc.send(response);\n> >>> +\t\t\tif (ret < 0)\n> >>> +\t\t\t\treturn ret;\n> >>> +\t\t\tbreak;\n> >>> +\t\t}\n> >>> +\t\tcase CMD_LEN_CALC: {\n> >>> +\t\t\tint size = 0;\n> >>> +\t\t\tfor (int fd : payload.fds)\n> >>> +\t\t\t\tsize += calcLength(fd);\n> >>> +\n> >>> +\t\t\tresponse.data.resize(sizeof(size));\n> >>> +\t\t\tmemcpy(response.data.data(), &size, sizeof(size));\n> >>> +\t\t\tret = ipc.send(response);\n> >>> +\t\t\tif (ret < 0)\n> >>> +\t\t\t\treturn ret;\n> >>> +\t\t\tbreak;\n> >>> +\t\t}\n> >>> +\t\tcase CMD_LEN_CMP: {\n> >>> +\t\t\tint size = 0;\n> >>> +\t\t\tfor (int fd : payload.fds)\n> >>> +\t\t\t\tsize += calcLength(fd);\n> >>> +\n> >>> +\t\t\tint cmp;\n> >>> +\t\t\tmemcpy(&cmp, payload.data.data(), sizeof(cmp));\n> >>> +\n> >>> +\t\t\tif (cmp != size)\n> >>> +\t\t\t\treturn -ERANGE;\n> >>> +\t\t\tbreak;\n> >>> +\t\t}\n> >>> +\t\tdefault:\n> >>> +\t\t\tcerr << \"Unkown command \" << payload.priv << endl;\n> >>> +\t\t\treturn -EINVAL;\n> >>> +\t\t}\n> >>> +\t}\n> >>> +\n> >>> +\tipc.close();\n> >>> +\n> >>> +\treturn 0;\n> >>> +}\n> >>> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> >>> new file mode 100644\n> >>> index 0000000000000000..ad2609764166a852\n> >>> --- /dev/null\n> >>> +++ b/test/ipc/unixsocket.cpp\n> >>> @@ -0,0 +1,200 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2019, Google Inc.\n> >>> + *\n> >>> + * unixsocket.cpp - Unix socket IPC test\n> >>> + */\n> >>> +\n> >>> +#include \"unixsocket.h\"\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +#include \"ipc_unixsocket.h\"\n> >>> +#include \"test.h\"\n> >>> +\n> >>> +#define MASTER_BIN IPC_TEST_DIR \"/unixsocket\"\n> >>> +#define SLAVE_BIN IPC_TEST_DIR \"/unixsocket-slave\"\n> >> \n> >> Why not just have all the code built into the same executable - and\n> >> decide what path to take based on the return of the fork?\n> > \n> > Because the use case is to use IPC to communicate with another process\n> > after exec(). But as stated above, we could decide to perform the basic\n> > IPC tests within a single thread and process, and leave the parts that\n> > would depend on actually spawning another process and calling exec() to\n> > another test, for latter, if the need arises.\n> > \n> >> In fact - is MASTER_BIN only ever used as test data, and a test FD?\n> >> (While SLAVE_BIN appears to be the same usage, but also an exec()? )\n> \n> Yes for the RFC I just needed to file descriptors to test with, why not \n> use whats already there? We could generate test files with dd as a build \n> step but if that seems like a better idea?\n> \n> >>> +\n> >>> +using namespace std;\n> >>> +using namespace libcamera;\n> >>> +\n> >>> +class UnixSocketTest : public Test\n> >>> +{\n> >>> +protected:\n> >>> +\tint slaveStart(int fd)\n> >>> +\t{\n> >>> +\t\tpid_ = fork();\n> >>> +\n> >>> +\t\tif (pid_ == -1)\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\tif (!pid_) {\n> >>> +\t\t\tstd::string arg = std::to_string(fd);\n> >>> +\t\t\texecl(SLAVE_BIN, SLAVE_BIN, arg.c_str());\n> >>> +\n> >> \n> >> Or instead of threads - if the slave code was in this binary - I think\n> >> it could just be executed here... It would have it's own process memory\n> >> by this point.... (or is that hacky? :S)\n> > \n> > That could be done, we won't be able to use TEST_REGISTER() in that case\n> > though, but it could be doable. However, this would get in the way of\n> > bundling multiple tests in a single binary (but wouldn't be impossible\n> > to do either, we would probably just need a two steps dispatch\n> > mechanism based on command line arguments, from main() to the main\n> > function of the test, and from there to the child process code).\n\nAs discussed separately today, I think this would be the best option.\nYou can then just exec /proc/self/exe without caring about the actual\npath.\n\n> >>> +\t\t\t/* Only get here if exec fails. */\n> >>> +\t\t\texit(TestFail);\n> >>> +\t\t}\n> >>> +\n> >>> +\t\treturn TestPass;\n> >>> +\t}\n> >>> +\n> >>> +\tint slaveStop()\n> >>> +\t{\n> >>> +\t\tint status;\n> >>> +\n> >>> +\t\tif (pid_ < 0)\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\tif (waitpid(pid_, &status, 0) < 0)\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\tif (!WIFEXITED(status) || WEXITSTATUS(status))\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\treturn TestPass;\n> >>> +\t}\n> >>> +\n> >>> +\tint testReverse()\n> >>> +\t{\n> >>> +\t\tstd::string input = \"FooBar\";\n> >>> +\t\tstd::string match = \"raBooF\";\n> >>> +\n> >>> +\t\tIPCUnixSocket::Payload payload, response;\n> >>> +\n> >>> +\t\tpayload.priv = CMD_REVERESE;\n> >>> +\t\tpayload.data = std::vector(input.begin(), input.end());\n> >>> +\n> >>> +\t\tif (ipc_.call(payload, &response, 100))\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\tstd::string output(response.data.begin(), response.data.end());\n> >>> +\n> >>> +\t\tif (output != match)\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\treturn 0;\n> >>> +\t}\n> >>> +\n> >>> +\tint testCalc()\n> >>> +\t{\n> >>> +\t\tint fdM = open(MASTER_BIN, O_RDONLY);\n> >>> +\t\tint fdS = open(SLAVE_BIN, O_RDONLY);\n> >>> +\n> >>> +\t\tif (fdM < 0 || fdS < 0)\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\tint size = 0;\n> >>> +\t\tsize += calcLength(fdM);\n> >>> +\t\tsize += calcLength(fdS);\n> >>> +\n> >>> +\t\tIPCUnixSocket::Payload payload, response;\n> >>> +\n> >>> +\t\tpayload.priv = CMD_LEN_CALC;\n> >>> +\t\tpayload.fds.push_back(fdM);\n> >>> +\t\tpayload.fds.push_back(fdS);\n> >>> +\n> >>> +\t\tif (ipc_.call(payload, &response, 100))\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\tint output;\n> >>> +\t\tmemcpy(&output, response.data.data(), sizeof(output));\n> >>> +\n> >>> +\t\tif (output != size)\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\treturn 0;\n> >>> +\t}\n> >>> +\n> >>> +\tint testCmp()\n> >>> +\t{\n> >>> +\t\tint fdM = open(MASTER_BIN, O_RDONLY);\n> >>> +\t\tint fdS = open(SLAVE_BIN, O_RDONLY);\n> >>> +\n> >>> +\t\tif (fdM < 0 || fdS < 0)\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\tint size = 0;\n> >>> +\t\tsize += calcLength(fdM);\n> >>> +\t\tsize += calcLength(fdS);\n> >>> +\n> >>> +\t\tIPCUnixSocket::Payload payload, response;\n> >>> +\n> >>> +\t\tpayload.priv = CMD_LEN_CMP;\n> >>> +\t\tpayload.data.resize(sizeof(size));\n> >>> +\t\tmemcpy(payload.data.data(), &size, sizeof(size));\n> >>> +\t\tpayload.fds.push_back(fdM);\n> >>> +\t\tpayload.fds.push_back(fdS);\n> >>> +\n> >>> +\t\tif (ipc_.send(payload))\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\treturn 0;\n> >>> +\t}\n> >>> +\n> >>> +\tint testClose()\n> >>> +\t{\n> >>> +\t\tIPCUnixSocket::Payload payload;\n> >>> +\n> >>> +\t\tpayload.priv = CMD_CLOSE;\n> >>> +\n> >>> +\t\tif (ipc_.send(payload))\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\treturn 0;\n> >>> +\t}\n> >>> +\n> >>> +\tint run()\n> >>> +\t{\n> >>> +\t\tint slavefd;\n> >>> +\n> >>> +\t\tslavefd = ipc_.create();\n> >>> +\t\tif (slavefd < 0)\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\tif (slaveStart(slavefd))\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\tif (ipc_.connect()) {\n> >>> +\t\t\tcerr << \"Failed to connect to IPC\" << endl;\n> >>> +\t\t\treturn TestFail;\n> >>> +\t\t}\n> >>> +\t\tif (testReverse()) {\n> >>> +\t\t\tcerr << \"String reverse fail\" << endl;\n> >>> +\t\t\treturn TestFail;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tif (testCalc()) {\n> >>> +\t\t\tcerr << \"Size calc fail\" << endl;\n> >>> +\t\t\treturn TestFail;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tif (testCmp()) {\n> >>> +\t\t\tcerr << \"Compare fail\" << endl;\n> >>> +\t\t\treturn TestFail;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tif (testClose())\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\tprintf(\"Master OK!\\n\");\n> >>> +\n> >>> +\t\tipc_.close();\n> >>> +\n> >>> +\t\tif (slaveStop())\n> >>> +\t\t\treturn TestFail;\n> >>> +\n> >>> +\t\treturn TestPass;\n> >>> +\t}\n> >>> +\n> >>> +private:\n> >>> +\tpid_t pid_;\n> >>> +\tIPCUnixSocket ipc_;\n> >>> +};\n> >>> +\n> >>> +TEST_REGISTER(UnixSocketTest)\n> >>> diff --git a/test/ipc/unixsocket.h b/test/ipc/unixsocket.h\n> >>> new file mode 100644\n> >>> index 0000000000000000..5ae223c76108a4f6\n> >>> --- /dev/null\n> >>> +++ b/test/ipc/unixsocket.h\n> >>> @@ -0,0 +1,27 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2019, Google Inc.\n> >>> + *\n> >>> + * unixsocket.h - Unix socket IPC test\n> >>> + *\n> >>> + */\n> >>> +#ifndef __LIBCAMERA_IPCUNIXSOCKET_TEST_H__\n> >>> +#define __LIBCAMERA_IPCUNIXSOCKET_TEST_H__\n> >>> +\n> >>> +#include \n> >>> +\n> >>> +#define CMD_CLOSE 0\n> >>> +#define CMD_REVERESE 1\n> >> \n> >> s/REVERESE/REVERSE/\n> >> \n> >>> +#define CMD_LEN_CALC 2\n> >>> +#define CMD_LEN_CMP 3\n> >>> +\n> >>> +int calcLength(int fd)\n> >>> +{\n> >>> +\tlseek(fd, 0, 0);\n> >>> +\tint size = lseek(fd, 0, SEEK_END);\n> >>> +\tlseek(fd, 0, 0);\n> >>> +\n> >>> +\treturn size;\n> >>> +}\n> >>> +\n> >>> +#endif /* __LIBCAMERA_IPCUNIXSOCKET_TEST_H__ */\n> >>> diff --git a/test/meson.build b/test/meson.build\n> >>> index c36ac24796367501..3666f6b2385bd4ca 100644\n> >>> --- a/test/meson.build\n> >>> +++ b/test/meson.build\n> >>> @@ -2,6 +2,7 @@ subdir('libtest')\n> >>> \n> >>> subdir('camera')\n> >>> subdir('ipa')\n> >>> +subdir('ipc')\n> >>> subdir('media_device')\n> >>> subdir('pipeline')\n> >>> subdir('stream')","headers":{"Return-Path":"","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 EE55261580\n\tfor ;\n\tMon, 24 Jun 2019 17:20:21 +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 4FCD4323;\n\tMon, 24 Jun 2019 17:20:21 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561389621;\n\tbh=Rz6JCmwwBdejgXoa9+bzk9wVqpA+FYZFv9I+GvNkjvY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dbkFQcbE5vd8kV6xA5pvEwmEMHnkKM1TjyBkyiC6Xy63kIsCFpfQwZw95mn7z6/Lr\n\tpWJ1Y839law9l49RD2ZUac2zkxb8xv2r0TjPnfh+Zuj2AbsRuQhjsW1isaZPkyf5FM\n\t8MOo4+hjcRcTuylyj7nGz45JE2UcgCQLCGsR0kcA=","Date":"Mon, 24 Jun 2019 18:20:01 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190624152001.GL5737@pendragon.ideasonboard.com>","References":"<20190621041519.29689-1-niklas.soderlund@ragnatech.se>\n\t<20190621041519.29689-3-niklas.soderlund@ragnatech.se>\n\t\n\t<20190624111358.GA5737@pendragon.ideasonboard.com>\n\t<20190624130005.GH32581@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190624130005.GH32581@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 2/2] test: ipc: unix: Add test for\n\tIPCUnixSocket","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 15:20:22 -0000"}}]