[libcamera-devel] [PATCH 3/3] test: file_descriptor: Add test

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Dec 16, 2019 at 01:10:29PM +0100, Niklas Söderlund wrote:
> Add a test which exercises the whole FileDescriptor interface.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  test/file_descriptor.cpp | 152 +++++++++++++++++++++++++++++++++++++++
>  test/meson.build         |   1 +
>  2 files changed, 153 insertions(+)
>  create mode 100644 test/file_descriptor.cpp
> 
> diff --git a/test/file_descriptor.cpp b/test/file_descriptor.cpp
> new file mode 100644
> index 0000000000000000..b1d0d0e0b2807b20
> --- /dev/null
> +++ b/test/file_descriptor.cpp
> @@ -0,0 +1,152 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * file_descriptor.cpp - FileDescriptor test
> + */
> +
> +#include <fcntl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <libcamera/file_descriptor.h>
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;

Alphabetically sorted ?

> +
> +class FileDescriptorTest : public Test
> +{
> +protected:
> +	int init()
> +	{
> +		fd_ = open("/tmp", O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR);
> +		if (fd_ < 0)
> +			return TestFail;
> +
> +		return 0;
> +	}
> +
> +	int run()
> +	{
> +		FileDescriptor *desc1, *desc2;
> +
> +		/* Test creating FileDescriptor from numerical file descriptor. */
> +		desc1 = new FileDescriptor(fd_);
> +		if (desc1->fd() == fd_)

You're leaking desc1. There are plenty of leaks below too. It's
important to avoid leaks in tests, as otherwise valgrind will catch them
and we won't be able to tell whether libcamera or the tests are leaky.
You could make them class members and delete them in the destructor.
Don't forget to set the pointers to nullptr in the constructor, as well
as after every manual delete to avoid double-delete.

> +			return TestFail;
> +
> +		if (!validFD(fd_) || !validFD(desc1->fd()))
> +			return TestFail;
> +
> +		delete desc1;
> +
> +		if (!validFD(fd_))
> +			return TestFail;

I would add

		int fd = desc1->fd();

before the delete, and replace the above check with

		if (!validFD(fd_) || validFD(fd))
			return TestFail;

to ensure that deleting the FileDescriptor closes the managed fd.

> +
> +		/* Test creating FileDescriptor from other FileDescriptor. */
> +		desc1 = new FileDescriptor(fd_);
> +		desc2 = new FileDescriptor(*desc1);
> +
> +		if (desc1->fd() == fd_ || desc2->fd() == fd_ || desc1->fd() == desc2->fd())
> +			return TestFail;
> +
> +		if (!validFD(fd_) || !validFD(desc1->fd()) || !validFD(desc2->fd()))
> +			return TestFail;
> +
> +		delete desc1;
> +
> +		if (!validFD(fd_) || !validFD(desc2->fd()))
> +			return TestFail;
> +
> +		delete desc2;
> +
> +		if (!validFD(fd_))
> +			return TestFail;

I think you can drop the three !validFD(fd_) checks above, as well as
all the same checks below, as this is already tested by the first test
case.

> +
> +		/* Test creating FileDescriptor by taking over other FileDescriptor. */
> +		desc1 = new FileDescriptor(fd_);
> +		desc2 = new FileDescriptor(std::move(*desc1));
> +
> +		if (desc1->fd() != -1 || desc2->fd() == fd_ || desc1->fd() == desc2->fd())
> +			return TestFail;

You want here to make sure that constructing desc2 didn't dup desc1.

		desc1 = new FileDescriptor(fd_);
		fd = desc1->fd();
		desc2 = new FileDescriptor(std::move(*desc1));

		if (desc1->fd() != -1 || desc2->fd() != fd)
			return TestFail;

> +
> +		if (!validFD(fd_) || !validFD(desc2->fd()))
> +			return TestFail;
> +
> +		delete desc1;
> +		delete desc2;
> +
> +		if (!validFD(fd_))
> +			return TestFail;
> +
> +		/* Test creating FileDescriptor by copy assigment */

s/assignment/assignment./

> +		desc1 = new FileDescriptor();
> +		desc2 = new FileDescriptor(fd_);
> +
> +		if (desc1->fd() != -1 || desc2->fd() == fd_ || desc1->fd() == desc2->fd())
> +			return TestFail;
> +
> +		if (!validFD(fd_) || !validFD(desc2->fd()))
> +			return TestFail;

I think you can drop all these checks except for desc1->fd() != -1 as
that hasn't been tested before.

> +
> +		*desc1 = *desc2;
> +
> +		if (desc1->fd() == fd_ || desc2->fd() == fd_ || desc1->fd() == desc2->fd())
> +			return TestFail;

Similarly as before,

		fd = desc2->fd();

before the assignment, and the test should be

		if (desc1->fd() == fd || desc2->fd() != fd)
			return TestFail;

to ensure that desc1 did dup and the desc2 wasn't modified.

> +
> +		if (!validFD(fd_) || !validFD(desc1->fd()) || !validFD(desc2->fd()))
> +			return TestFail;
> +
> +		delete desc1;
> +		delete desc2;
> +
> +		if (!validFD(fd_))
> +			return TestFail;
> +
> +		/* Test creating FileDescriptor by move assigment */

s/assignment/assignment./

> +		desc1 = new FileDescriptor();
> +		desc2 = new FileDescriptor(fd_);
> +
> +		if (desc1->fd() != -1 || desc2->fd() == fd_ || desc1->fd() == desc2->fd())
> +			return TestFail;
> +
> +		if (!validFD(fd_) || !validFD(desc2->fd()))
> +			return TestFail;
> +

Those checks can be dropped too.

> +		*desc1 = std::move(*desc2);
> +
> +		if (desc1->fd() == fd_ || desc2->fd() != -1 || desc1->fd() == desc2->fd())
> +			return TestFail;

And same as before,

		if (desc1->fd() != fd || desc2->fd() != -1)
			return TestFail;

with

		fd = desc2->fd();

before the assignment.

> +
> +		if (!validFD(fd_) || !validFD(desc1->fd()))
> +			return TestFail;
> +
> +		delete desc1;
> +		delete desc2;
> +
> +		if (!validFD(fd_))
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +
> +	void cleanup()
> +	{
> +		if (fd_ > 0)

>= 0, or != -1

> +			close(fd_);
> +	}
> +
> +private:
> +	bool validFD(int fd)

This method should be static.

s/validFD/isValidFd/ (or validFd) ?

> +	{
> +		struct stat s;
> +		return fstat(fd, &s) == 0;

What are you trying to catch here, that the fd is still open ? Maybe the
method should then be called isFdOpen() ? If you want to go one step
further you could also compare s.st_ino with a saved value retrieved in
init() after creating the temporary file. Then the method shouldn't be
static, and can be called isValidFd(). A one-line comment explaining
what is checked would be useful in any case.

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

> +	}
> +
> +	int fd_;
> +};
> +
> +TEST_REGISTER(FileDescriptorTest)
> diff --git a/test/meson.build b/test/meson.build
> index 1bb2161dc05a7b1d..9dc392df30efd956 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -25,6 +25,7 @@ internal_tests = [
>      ['event',                           'event.cpp'],
>      ['event-dispatcher',                'event-dispatcher.cpp'],
>      ['event-thread',                    'event-thread.cpp'],
> +    ['file_descriptor',                 'file_descriptor.cpp'],
>      ['message',                         'message.cpp'],
>      ['object',                          'object.cpp'],
>      ['object-invoke',                   'object-invoke.cpp'],

-- 
Regards,

Laurent Pinchart