[libcamera-devel,RFC,2/2] test: ipc: unix: Add test for IPCUnixSocket

1492 diff mbox series

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@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

Hi Niklas,

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@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?


> +
> +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?


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()? )


> +
> +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)

> +			/* 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')
>

Hi Kieran,

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@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 ?

> > +
> > +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()? )
> 
> > +
> > +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).

> > +			/* 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')

On 2019-06-24 14:13:58 +0300, Laurent Pinchart wrote:
> Hi Kieran,
> 
> 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@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.

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.

> 
> > > +
> > > +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).
> 
> > > +			/* 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

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@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')