[libcamera-devel,v8,0/4] IPA isolation: Part 3: Tests and documentation

11287 mbox series

v8 is split in three parts, core components, conversion + plumbing, and
tests + documentation.

This is part 3, and adds tests and documentation for IPA isolation.

1/4 tests the IPADataSerializer and 2/4 tests the IPCUnixSocket, both
in the same manner as how the generated IPA proxies would use them.

3/4 adds a guide about writing IPAs.

4/4 tests the generated serializer. v7 adds a test to test
(de)serialization of a vector of *generated* structs.


Changes in v8:
- (from v7.1) fix bullet points and update wordings in the ipa writer
  guide

Changes in v7:
- add test to test serdes of a vector of *generated* structs
- remove printing values of vectors/maps
- use the new sendSync/sendAsync API
- update IPA guide

Changes in v6:
- no longer need to initialize rpi ControlInfoMap, and no longer
  necessary it pass it to the ControlList serializer
- update documentation about the required namespacing, customizable
  start(), and that {pipeline_name}.h is no longer required
- use namespacing in the mojom file and test


Paul Elder (4):
  tests: Add IPADataSerializer test
  tests: Add test for IPCPipeUnixSocket
  Documentation: Add IPA writers guide
  tests: Test IPA serializer generation

 Documentation/guides/ipa.rst                  | 474 ++++++++++++++++++
 Documentation/index.rst                       |   1 +
 Documentation/meson.build                     |   1 +
 test/ipc/meson.build                          |   3 +-
 test/ipc/unixsocket_ipc.cpp                   | 233 +++++++++
 .../generated_serializer_test.cpp             | 156 ++++++
 .../generated_serializer/meson.build          |  49 ++
 .../generated_serializer/vimc.mojom           |  33 ++
 .../ipa_data_serializer_test.cpp              | 378 ++++++++++++++
 test/serialization/meson.build                |   5 +-
 10 files changed, 1331 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/guides/ipa.rst
 create mode 100644 test/ipc/unixsocket_ipc.cpp
 create mode 100644 test/serialization/generated_serializer/generated_serializer_test.cpp
 create mode 100644 test/serialization/generated_serializer/meson.build
 create mode 100644 test/serialization/generated_serializer/vimc.mojom
 create mode 100644 test/serialization/ipa_data_serializer_test.cpp

Hi Paul,

Thank you for the patch.

On Sat, Feb 13, 2021 at 01:23:10PM +0900, Paul Elder wrote:
> Test the IPC functions of IPCPipeUnixSocket.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> ---
> No change in v8
> 
> Changes in v7:
> - use the new sendSync/sendAsync API
> - remove redundant error messages
> - use PATH_MAX instead of 100
> 
> No change in v6
> 
> Changes in v5:
> - rename IPAIPCUnixSocket to IPCPipeUnixSocket
> - use IPCMessage
> 
> No change in v4
> 
> Changes in v3:
> - use readHeader, writeHeader, and eraseHeader as static class functions
>   of IPAIPCUnixSocket
> 
> New in v2
> ---
>  test/ipc/meson.build        |   3 +-
>  test/ipc/unixsocket_ipc.cpp | 233 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 235 insertions(+), 1 deletion(-)
>  create mode 100644 test/ipc/unixsocket_ipc.cpp
> 
> diff --git a/test/ipc/meson.build b/test/ipc/meson.build
> index 9f413ff6..ad47b2fe 100644
> --- a/test/ipc/meson.build
> +++ b/test/ipc/meson.build
> @@ -1,7 +1,8 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  ipc_tests = [
> -    ['unixsocket',   'unixsocket.cpp'],
> +    ['unixsocket_ipc', 'unixsocket_ipc.cpp'],
> +    ['unixsocket',     'unixsocket.cpp'],
>  ]
>  
>  foreach t : ipc_tests
> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
> new file mode 100644
> index 00000000..1787891b
> --- /dev/null
> +++ b/test/ipc/unixsocket_ipc.cpp
> @@ -0,0 +1,233 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * unixsocket_ipc.cpp - Unix socket IPC test
> + */
> +
> +#include <algorithm>
> +#include <fcntl.h>
> +#include <iostream>
> +#include <limits.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include "libcamera/internal/event_dispatcher.h"
> +#include "libcamera/internal/ipa_data_serializer.h"
> +#include "libcamera/internal/ipc_pipe.h"
> +#include "libcamera/internal/ipc_pipe_unixsocket.h"
> +#include "libcamera/internal/process.h"
> +#include "libcamera/internal/thread.h"
> +#include "libcamera/internal/timer.h"
> +#include "libcamera/internal/utils.h"
> +
> +#include "test.h"
> +
> +#define CMD_EXIT	0
> +#define CMD_GET_SYNC	1
> +#define CMD_SET_ASYNC	2

I'd store this in an enum.

> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class UnixSocketTestIPCSlave
> +{
> +public:
> +	UnixSocketTestIPCSlave()
> +		: value_(1337), exitCode_(EXIT_FAILURE), exit_(false)
> +	{
> +		dispatcher_ = Thread::current()->eventDispatcher();
> +		ipc_.readyRead.connect(this, &UnixSocketTestIPCSlave::readyRead);
> +	}
> +
> +	int run(int fd)
> +	{
> +		if (ipc_.bind(fd)) {
> +			cerr << "Failed to connect to IPC channel" << endl;
> +			return EXIT_FAILURE;
> +		}
> +
> +		while (!exit_)
> +			dispatcher_->processEvents();
> +
> +		ipc_.close();
> +
> +		return exitCode_;
> +	}
> +
> +private:
> +	void readyRead(IPCUnixSocket *ipc)
> +	{
> +		IPCUnixSocket::Payload message;
> +		int ret;
> +
> +		ret = ipc->receive(&message);
> +		if (ret) {
> +			cerr << "Receive message failed: " << ret << endl;
> +			return;
> +		}
> +
> +		IPCMessage ipcMessage(message);
> +		uint32_t cmd = ipcMessage.header().cmd;
> +
> +		switch (cmd) {
> +		case CMD_EXIT: {
> +			exit_ = true;
> +			break;
> +		}
> +
> +		case CMD_GET_SYNC: {
> +			IPCMessage::Header header = { cmd, ipcMessage.header().cookie };
> +			IPCMessage response(header);
> +
> +			vector<uint8_t> buf;
> +			tie(buf, ignore) = IPADataSerializer<int32_t>::serialize(value_);
> +			response.data().insert(response.data().end(), buf.begin(), buf.end());
> +
> +			ret = ipc_.send(response.payload());
> +			if (ret < 0) {
> +				cerr << "Reply failed" << endl;
> +				stop(ret);
> +			}
> +			break;
> +		}
> +
> +		case CMD_SET_ASYNC: {
> +			value_ = IPADataSerializer<int32_t>::deserialize(ipcMessage.data());
> +			break;
> +		}
> +		}
> +	}
> +
> +	void stop(int code)
> +	{
> +		exitCode_ = code;
> +		exit_ = true;
> +	}
> +
> +	int32_t value_;
> +
> +	IPCUnixSocket ipc_;
> +	EventDispatcher *dispatcher_;
> +	int exitCode_;
> +	bool exit_;
> +};
> +
> +class UnixSocketTestIPC : public Test
> +{
> +protected:
> +	int init()
> +	{
> +		return 0;
> +	}
> +
> +	int setVal(int32_t val)

setValue() ?

> +	{
> +		IPCMessage msg(CMD_SET_ASYNC);
> +		tie(msg.data(), ignore) = IPADataSerializer<int32_t>::serialize(val);
> +
> +		int ret = ipc_->sendAsync(msg);
> +		if (ret < 0) {
> +			cerr << "Failed to call set value" << endl;
> +			return ret;
> +		}
> +
> +		return 0;
> +	}
> +
> +	int getVal()

getValue() ?

> +	{
> +		IPCMessage msg(CMD_GET_SYNC);
> +		IPCMessage buf;
> +
> +		int ret = ipc_->sendSync(msg, &buf);
> +		if (ret < 0) {
> +			cerr << "Failed to call get value" << endl;
> +			return ret;
> +		}
> +
> +		return IPADataSerializer<int32_t>::deserialize(buf.data());
> +	}
> +
> +	int exit()
> +	{
> +		IPCMessage msg(CMD_EXIT);
> +
> +		int ret = ipc_->sendAsync(msg);
> +		if (ret < 0) {
> +			cerr << "Failed to call exit" << endl;
> +			return ret;
> +		}
> +
> +		return 0;
> +	}
> +
> +	int run()
> +	{
> +		char selfpath[PATH_MAX];
> +		memset(selfpath, 0, sizeof(selfpath));
> +		int ret = readlink("/proc/self/exe", selfpath, sizeof(selfpath));

readlink returns the number of bytes, so you don't have to memset.

		char selfpath[PATH_MAX];
		int ret = readlink("/proc/self/exe", selfpath,
				   sizeof(selfpath) - 1);
		/* error handling */
		selfpath[ret] = '\0';

The -1 is there to ensure space for the \0.

But do you actually need to read the link, can't you pass
"/proc/self/exe" to IPCPipeUnixSocket ?

I've noticed, when testing proc/self/exe directly, that isConnected()
returned true even if the executable can't be started (I made a typo
when typing the string). This is due to the failure occurring after the
fork(), so Process() doesn't realize. It's a bit annoying, but I'm not
sure we could do much about it. Maybe checking that the file exists
before forking would be nice, it's not 100% fullproof as it's subject to
races, but it would allow reporting the most common errors.

> +		if (ret < 0) {
> +			int err = errno;
> +			cerr << "Failed to get path: " << strerror(err) << endl;
> +			return TestFail;
> +		}
> +
> +		ipc_ = std::make_unique<IPCPipeUnixSocket>("", selfpath);
> +		if (!ipc_->isConnected()) {
> +			cerr << "Failed to create IPCPipe" << endl;
> +			return TestFail;
> +		}
> +
> +		ret = getVal();
> +		if (ret != 1337) {

Maybe the value could be defined as a constant ?

> +			cerr << "Wrong inital value, expected 1337, got " << ret << endl;
> +			return TestFail;
> +		}
> +
> +		ret = setVal(9001);
> +		if (ret < 0) {
> +			cerr << "Failed to set value: " << strerror(-ret) << endl;
> +			return TestFail;
> +		}
> +
> +		ret = getVal();
> +		if (ret != 9001) {
> +			cerr << "Wrong set value, expected 9001, got " << ret << endl;
> +			return TestFail;
> +		}
> +
> +		ret = exit();
> +		if (ret < 0) {
> +			cerr << "Failed to exit: " << strerror(-ret) << endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +private:
> +	ProcessManager processManager_;
> +
> +	unique_ptr<IPCPipeUnixSocket> ipc_;
> +};
> +
> +/*
> + * Can't use TEST_REGISTER() as single binary needs to act as both proxy
> + * master and slave.

There's no proxy here as it's just IPC, not IPA. Maybe it should then be
called client and server following the usual network terminology ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + */
> +int main(int argc, char **argv)
> +{
> +	/* IPCPipeUnixSocket passes IPA module path in argv[1] */
> +	if (argc == 3) {
> +		int ipcfd = std::stoi(argv[2]);
> +		UnixSocketTestIPCSlave slave;
> +		return slave.run(ipcfd);
> +	}
> +
> +	return UnixSocketTestIPC().execute();
> +}