[libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test
Hirokazu Honda
hiroh at chromium.org
Tue Nov 30 02:13:20 CET 2021
Hi Laurent,
On Tue, Nov 30, 2021 at 2:32 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Mon, Nov 29, 2021 at 10:36:17PM +0900, Hirokazu Honda wrote:
> > On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart wrote:
> > >
> > > Add a unit test to exercise the API of the UniqueFD class.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > test/meson.build | 1 +
> > > test/unique-fd.cpp | 220 +++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 221 insertions(+)
> > > create mode 100644 test/unique-fd.cpp
> > >
> > > diff --git a/test/meson.build b/test/meson.build
> > > index d0466f17d7b6..42dfbc1f8ee9 100644
> > > --- a/test/meson.build
> > > +++ b/test/meson.build
> > > @@ -53,6 +53,7 @@ internal_tests = [
> > > ['threads', 'threads.cpp'],
> > > ['timer', 'timer.cpp'],
> > > ['timer-thread', 'timer-thread.cpp'],
> > > + ['unique-fd', 'unique-fd.cpp'],
> > > ['utils', 'utils.cpp'],
> > > ]
> > >
> > > diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp
> > > new file mode 100644
> > > index 000000000000..92ca3f328811
> > > --- /dev/null
> > > +++ b/test/unique-fd.cpp
> > > @@ -0,0 +1,220 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * unique-fd.cpp - UniqueFD test
> > > + */
> > > +
> > > +#include <fcntl.h>
> > > +#include <iostream>
> > > +#include <sys/stat.h>
> > > +#include <sys/types.h>
> > > +#include <unistd.h>
> > > +
> > > +#include <libcamera/base/unique_fd.h>
> > > +#include <libcamera/base/utils.h>
> > > +
> > > +#include "test.h"
> > > +
> > > +using namespace libcamera;
> > > +using namespace std;
> > > +
> > > +class UniqueFDTest : public Test
> > > +{
> > > +protected:
> > > + int init()
> >
> > init() override
>
> Oops.
>
> > > + {
> > > + return createFd();
> > > + }
> > > +
> > > + int createFd()
> > > + {
> > > + fd_ = open("/tmp", O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR);
> > > + if (fd_ < 0)
> > > + return TestFail;
> > > +
> > > + /* Cache inode number of temp file. */
> > > + struct stat s;
> > > + if (fstat(fd_, &s))
> > > + return TestFail;
> > > +
> > > + inodeNr_ = s.st_ino;
> > > +
> > > + return 0;
> > > + }
> > > +
> > > + int run()
> >
> > run() override
>
> Re-oops :-)
>
> > > + {
> > > + /* Test creating empty UniqueFD. */
> > > + UniqueFD fd;
> > > +
> > > + if (fd.isValid() || fd.get() != -1) {
> >
> > I would not check the initialization value is -1.
> > The point is fd.isValid() is false with a default constructor and the
> > default value is not matter IMO.
> > If you would like, then I would declare the static constexpr value
> > like kInvalidFDValue and compare with it.
>
> It depends on the API contract I think. The documentation explicitly
> states that get() returns -1 if no fd is owned, so that's why I'm
> checking it. This matches std::unique_ptr<>::get() that returns nullptr
> if no object is owned. The alternative is to make get() an undefined
> behaviour if no fd is owned, and drop the checks here, but I don't think
> that would be a good idea. If get() is explicitly defined as returning
> -1 when no fd is owned, then I think that should be tested here.
>
Ack. I am fine to keep the code as-is.
-Hiro
> > > + std::cout << "Failed fd check (default constructor)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + /* Test creating UniqueFD from numerical file descriptor. */
> > > + UniqueFD fd2(fd_);
> > > + if (!fd2.isValid() || fd2.get() != fd_) {
> > > + std::cout << "Failed fd check (fd constructor)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (!isValidFd(fd_)) {
> > > + std::cout << "Failed fd validity (fd constructor)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + /* Test move constructor. */
> > > + UniqueFD fd3(std::move(fd2));
> > > + if (!fd3.isValid() || fd3.get() != fd_) {
> > > + std::cout << "Failed fd check (move constructor)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (fd2.isValid() || fd2.get() != -1) {
> >
> > ditto.
> >
> > > + std::cout << "Failed moved fd check (move constructor)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (!isValidFd(fd_)) {
> > > + std::cout << "Failed fd validity (move constructor)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + /* Test move assignment operator. */
> > > + fd = std::move(fd3);
> > > + if (!fd.isValid() || fd.get() != fd_) {
> > > + std::cout << "Failed fd check (move assignment)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (fd3.isValid() || fd3.get() != -1) {
> > > + std::cout << "Failed moved fd check (move assignment)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> >
> > ditto.
> >
> > > +
> > > + if (!isValidFd(fd_)) {
> > > + std::cout << "Failed fd validity (move assignment)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + /* Test swapping. */
> > > + fd2.swap(fd);
> > > + if (!fd2.isValid() || fd2.get() != fd_) {
> > > + std::cout << "Failed fd check (swap)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (fd.isValid() || fd.get() != -1) {
> > > + std::cout << "Failed swapped fd check (swap)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (!isValidFd(fd_)) {
> > > + std::cout << "Failed fd validity (swap)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + /* Test release. */
> > > + int numFd = fd2.release();
> > > + if (fd2.isValid() || fd2.get() != -1) {
> > > + std::cout << "Failed fd check (release)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (numFd != fd_) {
> > > + std::cout << "Failed released fd check (release)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (!isValidFd(fd_)) {
> > > + std::cout << "Failed fd validity (release)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + /* Test reset assignment. */
> > > + fd.reset(numFd);
> > > + if (!fd.isValid() || fd.get() != fd_) {
> > > + std::cout << "Failed fd check (reset assignment)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (!isValidFd(fd_)) {
> > > + std::cout << "Failed fd validity (reset assignment)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + /* Test reset destruction. */
> > > + fd.reset();
> > > + if (fd.isValid() || fd.get() != -1) {
> > > + std::cout << "Failed fd check (reset destruction)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (isValidFd(fd_)) {
> > > + std::cout << "Failed fd validity (reset destruction)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + /* Test destruction. */
> > > + if (createFd() == TestFail) {
> > > + std::cout << "Failed to recreate test fd"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + {
> > > + UniqueFD fd4(fd_);
> > > + }
> > > +
> > > + if (isValidFd(fd_)) {
> > > + std::cout << "Failed fd validity (destruction)"
> > > + << std::endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + return TestPass;
> > > + }
> > > +
> > > + void cleanup()
> >
> > cleanup() override
> >
> > With minor nits,
> >
> > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> >
> > > + {
> > > + if (fd_ > 0)
> > > + close(fd_);
> > > + }
> > > +
> > > +private:
> > > + bool isValidFd(int fd)
> > > + {
> > > + struct stat s;
> > > + if (fstat(fd, &s))
> > > + return false;
> > > +
> > > + /* Check that inode number matches cached temp file. */
> > > + return s.st_ino == inodeNr_;
> > > + }
> > > +
> > > + int fd_;
> > > + ino_t inodeNr_;
> > > +};
> > > +
> > > +TEST_REGISTER(UniqueFDTest)
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list