[libcamera-devel] [RFC 2/2] test: ipc: unix: Add test for IPCUnixSocket
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 24 17:20:01 CEST 2019
Hi Niklas,
On Mon, Jun 24, 2019 at 03:00:05PM +0200, Niklas Söderlund wrote:
> On 2019-06-24 14:13:58 +0300, Laurent Pinchart wrote:
> > On Mon, Jun 24, 2019 at 09:18:40AM +0100, Kieran Bingham wrote:
> >> On 21/06/2019 05:15, Niklas Söderlund wrote:
> >>> Test that the IPC supports sending data and file descriptors over the
> >>> IPC medium. To be able execute the test two executables are needed, one
> >>> to drive the test and act as the libcamera (master) and a one to act as
> >>> the IPA (slave).
> >>>
> >>> The master drives the testing posting requests to the slave to process
> >>> and sometime respond to. A few different tests are preformed.
> >>>
> >>> - Master sends a string to the slave which responds with the reversed
> >>> string. The master verifies that a reversed string is indeed returned.
> >>>
> >>> - Master sends a list of file descriptors and ask the salve to calculate
> >>> and respond with the sum of the size of the files. The master verifies
> >>> that the calculate size is correct.
> >>>
> >>> - Master send a pre-computed size and a list of file descriptors and
> >>> ask the slave to verify that the pre-computed size matches the sum of
> >>> the size of the file descriptors.
> >>>
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >>> ---
> >>> test/ipc/meson.build | 20 ++++
> >>> test/ipc/unixsocket-slave.cpp | 92 ++++++++++++++++
> >>> test/ipc/unixsocket.cpp | 200 ++++++++++++++++++++++++++++++++++
> >>> test/ipc/unixsocket.h | 27 +++++
> >>> test/meson.build | 1 +
> >>> 5 files changed, 340 insertions(+)
> >>> create mode 100644 test/ipc/meson.build
> >>> create mode 100644 test/ipc/unixsocket-slave.cpp
> >>> create mode 100644 test/ipc/unixsocket.cpp
> >>> create mode 100644 test/ipc/unixsocket.h
> >>>
> >>> diff --git a/test/ipc/meson.build b/test/ipc/meson.build
> >>> new file mode 100644
> >>> index 0000000000000000..0a425d4e7241c753
> >>> --- /dev/null
> >>> +++ b/test/ipc/meson.build
> >>> @@ -0,0 +1,20 @@
> >>> +# Tests are listed in order of complexity.
> >>> +# They are not alphabetically sorted.
> >>> +ipc_tests = [
> >>> + [ 'unixsocket', 'unixsocket.cpp', 'unixsocket-slave', 'unixsocket-slave.cpp' ],
> >>> +]
> >>> +
> >>> +foreach t : ipc_tests
> >>> + exe = executable(t[0], t[1],
> >>> + dependencies : libcamera_dep,
> >>> + link_with : test_libraries,
> >>> + include_directories : test_includes_internal)
> >>> +
> >>> + slave = executable(t[2], t[3],
> >>> + dependencies : libcamera_dep,
> >>> + include_directories : test_includes_internal)
> >>> +
> >>> + test(t[0], exe, suite : 'ipc', is_parallel : false)
> >>> +endforeach
> >>
> >> Constructing multiple binaries for each test sounds like it's going to
> >> prove somewhat awkward to validate the tests, requiring starting and
> >> stopping external binaries, and checking their return status.
> >>
> >> Couldn't we use threads in a single binary to contain each test?
> >
> > We need multiple processes, IPC stands for inter process communication
> > :-) We could of course use it to communicate within a single process,
> > and even a single thread, but the environment being different may lead
> > to different results. However, if the IPC and process management
> > components are well designed and their APIs don't overlap, it could
> > still be an option.
> >
> > Niklas, what is your opinion, could we start with an IPC test that runs
> > within a single thread ?
>
> I think it's a good idea to have test cases which run as close as
> possible to the real thing. I really think this should be two
> executables, if we test IPC between two threads we might not catch an
> issue in time.
It can also one executable with two processes, it can re-execute itself
in the child process with a command line argument to turn child mode on.
> The idea is to use the process manager once it's ready in this test case
> to setup and monitor the proxy slave so I don't see any cost maintaining
> monitoring of the slave process.
>
> On a final note I see no real problem with having hardcoded pathes in
> the binaries. This is test cases not intended for users but to sanity
> check the code base. It would be nice if they where not needed but I see
> no harm in having them.
>From an automated testing point of view, tests may run on a different
machine than they are built, so it would be good to be able to install
them.
> >>> +
> >>> +config_h.set('IPC_TEST_DIR', '"' + meson.current_build_dir() + '"')
> >>
> >> This means the tests can never be executed anywhere except inside the
> >> build directory ... I'm not very fond of this at all :-S
> >>
> >>> diff --git a/test/ipc/unixsocket-slave.cpp b/test/ipc/unixsocket-slave.cpp
> >>> new file mode 100644
> >>> index 0000000000000000..ec27f6bf29823173
> >>> --- /dev/null
> >>> +++ b/test/ipc/unixsocket-slave.cpp
> >>> @@ -0,0 +1,92 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> +/*
> >>> + * Copyright (C) 2019, Google Inc.
> >>> + *
> >>> + * unixsocket-slave.cpp - Unix socket IPC slave runner
> >>> + */
> >>> +
> >>> +#include "unixsocket.h"
> >>> +
> >>> +#include <algorithm>
> >>> +#include <iostream>
> >>> +#include <string.h>
> >>> +#include <unistd.h>
> >>> +
> >>> +#include "ipc_unixsocket.h"
> >>> +
> >>> +using namespace std;
> >>> +using namespace libcamera;
> >>> +
> >>> +int main(int argc, char **argv)
> >>> +{
> >>> + if (argc != 2) {
> >>> + cerr << "usage: %s <ipc fd>" << endl;
> >>> + return EXIT_FAILURE;
> >>> + }
> >>> +
> >>> + int ipcfd = std::stoi(argv[1]);
> >>> + IPCUnixSocket ipc(ipcfd);
> >>> +
> >>> + if (ipc.connect()) {
> >>> + cerr << "Failed to connect to IPC" << endl;
> >>> + return EXIT_FAILURE;
> >>> + }
> >>> +
> >>> + bool run = true;
> >>> + while (run) {
> >>> + int ret = 0;
> >>> + IPCUnixSocket::Payload payload, response;
> >>> +
> >>> + ret = ipc.recv(&payload, 100);
> >>> + if (ret < 0) {
> >>> + if (ret == -ETIMEDOUT)
> >>> + continue;
> >>> + return ret;
> >>> + }
> >>> + switch (payload.priv) {
> >>> + case CMD_CLOSE:
> >>> + run = false;
> >>> + break;
> >>> + case CMD_REVERESE: {
> >>> + std::string str(payload.data.begin(), payload.data.end());
> >>> + std::reverse(str.begin(), str.end());
> >>> + response.data = std::vector<uint8_t>(str.begin(), str.end());
> >>> + ret = ipc.send(response);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> + break;
> >>> + }
> >>> + case CMD_LEN_CALC: {
> >>> + int size = 0;
> >>> + for (int fd : payload.fds)
> >>> + size += calcLength(fd);
> >>> +
> >>> + response.data.resize(sizeof(size));
> >>> + memcpy(response.data.data(), &size, sizeof(size));
> >>> + ret = ipc.send(response);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> + break;
> >>> + }
> >>> + case CMD_LEN_CMP: {
> >>> + int size = 0;
> >>> + for (int fd : payload.fds)
> >>> + size += calcLength(fd);
> >>> +
> >>> + int cmp;
> >>> + memcpy(&cmp, payload.data.data(), sizeof(cmp));
> >>> +
> >>> + if (cmp != size)
> >>> + return -ERANGE;
> >>> + break;
> >>> + }
> >>> + default:
> >>> + cerr << "Unkown command " << payload.priv << endl;
> >>> + return -EINVAL;
> >>> + }
> >>> + }
> >>> +
> >>> + ipc.close();
> >>> +
> >>> + return 0;
> >>> +}
> >>> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> >>> new file mode 100644
> >>> index 0000000000000000..ad2609764166a852
> >>> --- /dev/null
> >>> +++ b/test/ipc/unixsocket.cpp
> >>> @@ -0,0 +1,200 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> +/*
> >>> + * Copyright (C) 2019, Google Inc.
> >>> + *
> >>> + * unixsocket.cpp - Unix socket IPC test
> >>> + */
> >>> +
> >>> +#include "unixsocket.h"
> >>> +
> >>> +#include <fcntl.h>
> >>> +#include <iostream>
> >>> +#include <string.h>
> >>> +#include <sys/stat.h>
> >>> +#include <sys/wait.h>
> >>> +#include <unistd.h>
> >>> +
> >>> +#include "ipc_unixsocket.h"
> >>> +#include "test.h"
> >>> +
> >>> +#define MASTER_BIN IPC_TEST_DIR "/unixsocket"
> >>> +#define SLAVE_BIN IPC_TEST_DIR "/unixsocket-slave"
> >>
> >> Why not just have all the code built into the same executable - and
> >> decide what path to take based on the return of the fork?
> >
> > Because the use case is to use IPC to communicate with another process
> > after exec(). But as stated above, we could decide to perform the basic
> > IPC tests within a single thread and process, and leave the parts that
> > would depend on actually spawning another process and calling exec() to
> > another test, for latter, if the need arises.
> >
> >> In fact - is MASTER_BIN only ever used as test data, and a test FD?
> >> (While SLAVE_BIN appears to be the same usage, but also an exec()? )
>
> Yes for the RFC I just needed to file descriptors to test with, why not
> use whats already there? We could generate test files with dd as a build
> step but if that seems like a better idea?
>
> >>> +
> >>> +using namespace std;
> >>> +using namespace libcamera;
> >>> +
> >>> +class UnixSocketTest : public Test
> >>> +{
> >>> +protected:
> >>> + int slaveStart(int fd)
> >>> + {
> >>> + pid_ = fork();
> >>> +
> >>> + if (pid_ == -1)
> >>> + return TestFail;
> >>> +
> >>> + if (!pid_) {
> >>> + std::string arg = std::to_string(fd);
> >>> + execl(SLAVE_BIN, SLAVE_BIN, arg.c_str());
> >>> +
> >>
> >> Or instead of threads - if the slave code was in this binary - I think
> >> it could just be executed here... It would have it's own process memory
> >> by this point.... (or is that hacky? :S)
> >
> > That could be done, we won't be able to use TEST_REGISTER() in that case
> > though, but it could be doable. However, this would get in the way of
> > bundling multiple tests in a single binary (but wouldn't be impossible
> > to do either, we would probably just need a two steps dispatch
> > mechanism based on command line arguments, from main() to the main
> > function of the test, and from there to the child process code).
As discussed separately today, I think this would be the best option.
You can then just exec /proc/self/exe without caring about the actual
path.
> >>> + /* Only get here if exec fails. */
> >>> + exit(TestFail);
> >>> + }
> >>> +
> >>> + return TestPass;
> >>> + }
> >>> +
> >>> + int slaveStop()
> >>> + {
> >>> + int status;
> >>> +
> >>> + if (pid_ < 0)
> >>> + return TestFail;
> >>> +
> >>> + if (waitpid(pid_, &status, 0) < 0)
> >>> + return TestFail;
> >>> +
> >>> + if (!WIFEXITED(status) || WEXITSTATUS(status))
> >>> + return TestFail;
> >>> +
> >>> + return TestPass;
> >>> + }
> >>> +
> >>> + int testReverse()
> >>> + {
> >>> + std::string input = "FooBar";
> >>> + std::string match = "raBooF";
> >>> +
> >>> + IPCUnixSocket::Payload payload, response;
> >>> +
> >>> + payload.priv = CMD_REVERESE;
> >>> + payload.data = std::vector<uint8_t>(input.begin(), input.end());
> >>> +
> >>> + if (ipc_.call(payload, &response, 100))
> >>> + return TestFail;
> >>> +
> >>> + std::string output(response.data.begin(), response.data.end());
> >>> +
> >>> + if (output != match)
> >>> + return TestFail;
> >>> +
> >>> + return 0;
> >>> + }
> >>> +
> >>> + int testCalc()
> >>> + {
> >>> + int fdM = open(MASTER_BIN, O_RDONLY);
> >>> + int fdS = open(SLAVE_BIN, O_RDONLY);
> >>> +
> >>> + if (fdM < 0 || fdS < 0)
> >>> + return TestFail;
> >>> +
> >>> + int size = 0;
> >>> + size += calcLength(fdM);
> >>> + size += calcLength(fdS);
> >>> +
> >>> + IPCUnixSocket::Payload payload, response;
> >>> +
> >>> + payload.priv = CMD_LEN_CALC;
> >>> + payload.fds.push_back(fdM);
> >>> + payload.fds.push_back(fdS);
> >>> +
> >>> + if (ipc_.call(payload, &response, 100))
> >>> + return TestFail;
> >>> +
> >>> + int output;
> >>> + memcpy(&output, response.data.data(), sizeof(output));
> >>> +
> >>> + if (output != size)
> >>> + return TestFail;
> >>> +
> >>> + return 0;
> >>> + }
> >>> +
> >>> + int testCmp()
> >>> + {
> >>> + int fdM = open(MASTER_BIN, O_RDONLY);
> >>> + int fdS = open(SLAVE_BIN, O_RDONLY);
> >>> +
> >>> + if (fdM < 0 || fdS < 0)
> >>> + return TestFail;
> >>> +
> >>> + int size = 0;
> >>> + size += calcLength(fdM);
> >>> + size += calcLength(fdS);
> >>> +
> >>> + IPCUnixSocket::Payload payload, response;
> >>> +
> >>> + payload.priv = CMD_LEN_CMP;
> >>> + payload.data.resize(sizeof(size));
> >>> + memcpy(payload.data.data(), &size, sizeof(size));
> >>> + payload.fds.push_back(fdM);
> >>> + payload.fds.push_back(fdS);
> >>> +
> >>> + if (ipc_.send(payload))
> >>> + return TestFail;
> >>> +
> >>> + return 0;
> >>> + }
> >>> +
> >>> + int testClose()
> >>> + {
> >>> + IPCUnixSocket::Payload payload;
> >>> +
> >>> + payload.priv = CMD_CLOSE;
> >>> +
> >>> + if (ipc_.send(payload))
> >>> + return TestFail;
> >>> +
> >>> + return 0;
> >>> + }
> >>> +
> >>> + int run()
> >>> + {
> >>> + int slavefd;
> >>> +
> >>> + slavefd = ipc_.create();
> >>> + if (slavefd < 0)
> >>> + return TestFail;
> >>> +
> >>> + if (slaveStart(slavefd))
> >>> + return TestFail;
> >>> +
> >>> + if (ipc_.connect()) {
> >>> + cerr << "Failed to connect to IPC" << endl;
> >>> + return TestFail;
> >>> + }
> >>> + if (testReverse()) {
> >>> + cerr << "String reverse fail" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + if (testCalc()) {
> >>> + cerr << "Size calc fail" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + if (testCmp()) {
> >>> + cerr << "Compare fail" << endl;
> >>> + return TestFail;
> >>> + }
> >>> +
> >>> + if (testClose())
> >>> + return TestFail;
> >>> +
> >>> + printf("Master OK!\n");
> >>> +
> >>> + ipc_.close();
> >>> +
> >>> + if (slaveStop())
> >>> + return TestFail;
> >>> +
> >>> + return TestPass;
> >>> + }
> >>> +
> >>> +private:
> >>> + pid_t pid_;
> >>> + IPCUnixSocket ipc_;
> >>> +};
> >>> +
> >>> +TEST_REGISTER(UnixSocketTest)
> >>> diff --git a/test/ipc/unixsocket.h b/test/ipc/unixsocket.h
> >>> new file mode 100644
> >>> index 0000000000000000..5ae223c76108a4f6
> >>> --- /dev/null
> >>> +++ b/test/ipc/unixsocket.h
> >>> @@ -0,0 +1,27 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> +/*
> >>> + * Copyright (C) 2019, Google Inc.
> >>> + *
> >>> + * unixsocket.h - Unix socket IPC test
> >>> + *
> >>> + */
> >>> +#ifndef __LIBCAMERA_IPCUNIXSOCKET_TEST_H__
> >>> +#define __LIBCAMERA_IPCUNIXSOCKET_TEST_H__
> >>> +
> >>> +#include <unistd.h>
> >>> +
> >>> +#define CMD_CLOSE 0
> >>> +#define CMD_REVERESE 1
> >>
> >> s/REVERESE/REVERSE/
> >>
> >>> +#define CMD_LEN_CALC 2
> >>> +#define CMD_LEN_CMP 3
> >>> +
> >>> +int calcLength(int fd)
> >>> +{
> >>> + lseek(fd, 0, 0);
> >>> + int size = lseek(fd, 0, SEEK_END);
> >>> + lseek(fd, 0, 0);
> >>> +
> >>> + return size;
> >>> +}
> >>> +
> >>> +#endif /* __LIBCAMERA_IPCUNIXSOCKET_TEST_H__ */
> >>> diff --git a/test/meson.build b/test/meson.build
> >>> index c36ac24796367501..3666f6b2385bd4ca 100644
> >>> --- a/test/meson.build
> >>> +++ b/test/meson.build
> >>> @@ -2,6 +2,7 @@ subdir('libtest')
> >>>
> >>> subdir('camera')
> >>> subdir('ipa')
> >>> +subdir('ipc')
> >>> subdir('media_device')
> >>> subdir('pipeline')
> >>> subdir('stream')
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list