[{"id":21318,"web_url":"https://patchwork.libcamera.org/comment/21318/","msgid":"","date":"2021-11-29T13:36:17","subject":"Re: [libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart\n wrote:\n>\n> Add a unit test to exercise the API of the UniqueFD class.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> test/meson.build | 1 +\n> test/unique-fd.cpp | 220 +++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 221 insertions(+)\n> create mode 100644 test/unique-fd.cpp\n>\n> diff --git a/test/meson.build b/test/meson.build\n> index d0466f17d7b6..42dfbc1f8ee9 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -53,6 +53,7 @@ internal_tests = [\n> ['threads', 'threads.cpp'],\n> ['timer', 'timer.cpp'],\n> ['timer-thread', 'timer-thread.cpp'],\n> + ['unique-fd', 'unique-fd.cpp'],\n> ['utils', 'utils.cpp'],\n> ]\n>\n> diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp\n> new file mode 100644\n> index 000000000000..92ca3f328811\n> --- /dev/null\n> +++ b/test/unique-fd.cpp\n> @@ -0,0 +1,220 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * unique-fd.cpp - UniqueFD test\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"test.h\"\n> +\n> +using namespace libcamera;\n> +using namespace std;\n> +\n> +class UniqueFDTest : public Test\n> +{\n> +protected:\n> + int init()\ninit() override\n> + {\n> + return createFd();\n> + }\n> +\n> + int createFd()\n> + {\n> + fd_ = open(\"/tmp\", O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR);\n> + if (fd_ < 0)\n> + return TestFail;\n> +\n> + /* Cache inode number of temp file. */\n> + struct stat s;\n> + if (fstat(fd_, &s))\n> + return TestFail;\n> +\n> + inodeNr_ = s.st_ino;\n> +\n> + return 0;\n> + }\n> +\n> + int run()\nrun() override\n\n> + {\n> + /* Test creating empty UniqueFD. */\n> + UniqueFD fd;\n> +\n> + if (fd.isValid() || fd.get() != -1) {\n\nI would not check the initialization value is -1.\nThe point is fd.isValid() is false with a default constructor and the\ndefault value is not matter IMO.\nIf you would like, then I would declare the static constexpr value\nlike kInvalidFDValue and compare with it.\n\n> + std::cout << \"Failed fd check (default constructor)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + /* Test creating UniqueFD from numerical file descriptor. */\n> + UniqueFD fd2(fd_);\n> + if (!fd2.isValid() || fd2.get() != fd_) {\n> + std::cout << \"Failed fd check (fd constructor)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + if (!isValidFd(fd_)) {\n> + std::cout << \"Failed fd validity (fd constructor)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + /* Test move constructor. */\n> + UniqueFD fd3(std::move(fd2));\n> + if (!fd3.isValid() || fd3.get() != fd_) {\n> + std::cout << \"Failed fd check (move constructor)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + if (fd2.isValid() || fd2.get() != -1) {\n\nditto.\n\n> + std::cout << \"Failed moved fd check (move constructor)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + if (!isValidFd(fd_)) {\n> + std::cout << \"Failed fd validity (move constructor)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + /* Test move assignment operator. */\n> + fd = std::move(fd3);\n> + if (!fd.isValid() || fd.get() != fd_) {\n> + std::cout << \"Failed fd check (move assignment)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + if (fd3.isValid() || fd3.get() != -1) {\n> + std::cout << \"Failed moved fd check (move assignment)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n\nditto.\n\n> +\n> + if (!isValidFd(fd_)) {\n> + std::cout << \"Failed fd validity (move assignment)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + /* Test swapping. */\n> + fd2.swap(fd);\n> + if (!fd2.isValid() || fd2.get() != fd_) {\n> + std::cout << \"Failed fd check (swap)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + if (fd.isValid() || fd.get() != -1) {\n> + std::cout << \"Failed swapped fd check (swap)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + if (!isValidFd(fd_)) {\n> + std::cout << \"Failed fd validity (swap)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + /* Test release. */\n> + int numFd = fd2.release();\n> + if (fd2.isValid() || fd2.get() != -1) {\n> + std::cout << \"Failed fd check (release)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + if (numFd != fd_) {\n> + std::cout << \"Failed released fd check (release)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + if (!isValidFd(fd_)) {\n> + std::cout << \"Failed fd validity (release)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + /* Test reset assignment. */\n> + fd.reset(numFd);\n> + if (!fd.isValid() || fd.get() != fd_) {\n> + std::cout << \"Failed fd check (reset assignment)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + if (!isValidFd(fd_)) {\n> + std::cout << \"Failed fd validity (reset assignment)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + /* Test reset destruction. */\n> + fd.reset();\n> + if (fd.isValid() || fd.get() != -1) {\n> + std::cout << \"Failed fd check (reset destruction)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + if (isValidFd(fd_)) {\n> + std::cout << \"Failed fd validity (reset destruction)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + /* Test destruction. */\n> + if (createFd() == TestFail) {\n> + std::cout << \"Failed to recreate test fd\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + {\n> + UniqueFD fd4(fd_);\n> + }\n> +\n> + if (isValidFd(fd_)) {\n> + std::cout << \"Failed fd validity (destruction)\"\n> + << std::endl;\n> + return TestFail;\n> + }\n> +\n> + return TestPass;\n> + }\n> +\n> + void cleanup()\n\ncleanup() override\n\nWith minor nits,\n\nReviewed-by: Hirokazu Honda \n> + {\n> + if (fd_ > 0)\n> + close(fd_);\n> + }\n> +\n> +private:\n> + bool isValidFd(int fd)\n> + {\n> + struct stat s;\n> + if (fstat(fd, &s))\n> + return false;\n> +\n> + /* Check that inode number matches cached temp file. */\n> + return s.st_ino == inodeNr_;\n> + }\n> +\n> + int fd_;\n> + ino_t inodeNr_;\n> +};\n> +\n> +TEST_REGISTER(UniqueFDTest)\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3AAF7BF415\n\tfor ;\n\tMon, 29 Nov 2021 13:36:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58611605A4;\n\tMon, 29 Nov 2021 14:36:31 +0100 (CET)","from mail-ed1-x533.google.com (mail-ed1-x533.google.com\n\t[IPv6:2a00:1450:4864:20::533])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E3C6060592\n\tfor ;\n\tMon, 29 Nov 2021 14:36:29 +0100 (CET)","by mail-ed1-x533.google.com with SMTP id e3so72021619edu.4\n\tfor ;\n\tMon, 29 Nov 2021 05:36:29 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"UZNgul6x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=LQLPTUFKhXqbF44FJwuNyvBKCLJrB9oHMobHqMuqs9A=;\n\tb=UZNgul6xl+hE2k+XI3mB7FhsRlFs/QZhANzV10UjmFC66OT2b7AHTR1tglLlvAOmRs\n\tTp5Z+nyk4M97OqaOECIdmNlirtlvt22AzIgh+7i/NqxplTDpHLvpsJor7uAcIG+CcYiz\n\tvvagJpRrCkf5ga2yXsU6ER6e+BftLXLStuz18=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=LQLPTUFKhXqbF44FJwuNyvBKCLJrB9oHMobHqMuqs9A=;\n\tb=xXi/yfc8Ys6+bVbRFy+3ngzyznTSyU+9JRMD6Rd52RuXiz9vcgo7QoXtQjZ7ijJpCD\n\tjZgEZyce6hrRpJp2TejDoTAEz8qw6u9BZMBhbkWQTPkdiH+ikd0lo9aUp7RKUFmIh6rz\n\tKtoD25k7jP+vHXYpKh/g5qZfS8VsHhKpXkCOdjIebL73XYMmW01fn1t0HaACAoSBRv2p\n\tEGVagvwDOqUEpMUVz9YQw/mSh1120sFt8ncSjKCAIHGcTYfG6IjZHU3RqfgxeaPNErht\n\tlGSiXI0xxAnFoeT3jZSEUWgEraiRUCKwriiKa5R813Del8FGKmSGaKt74r9bm/2446ML\n\tJwvQ==","X-Gm-Message-State":"AOAM531UUVv7no0Pg+HLWAc3IbWS2zGb8J5quvhI0+OUQ+1W2R+2IE26\n\tT06hbwsnEWMCDtvSsEI8ajgtq4nn02tY7t9lPtt7JNh80Xk=","X-Google-Smtp-Source":"ABdhPJy2JA3aNRKCXRRPoCQJQCxjWYHuJMkiL04fxr+F29wuLVSpno8Rol3mhTaJr++8PzrsSvMWwPIXPoQV7PGDB68=","X-Received":"by 2002:a50:ef12:: with SMTP id\n\tm18mr75479544eds.381.1638192989245; \n\tMon, 29 Nov 2021 05:36:29 -0800 (PST)","MIME-Version":"1.0","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-6-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20211128235752.10836-6-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 29 Nov 2021 22:36:17 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21334,"web_url":"https://patchwork.libcamera.org/comment/21334/","msgid":"<20211129150704.egqvovu477pi767f@uno.localdomain>","date":"2021-11-29T15:07:04","subject":"Re: [libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Nov 29, 2021 at 01:57:40AM +0200, Laurent Pinchart wrote:\n> Add a unit test to exercise the API of the UniqueFD class.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> test/meson.build | 1 +\n> test/unique-fd.cpp | 220 +++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 221 insertions(+)\n> create mode 100644 test/unique-fd.cpp\n>\n> diff --git a/test/meson.build b/test/meson.build\n> index d0466f17d7b6..42dfbc1f8ee9 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -53,6 +53,7 @@ internal_tests = [\n> ['threads', 'threads.cpp'],\n> ['timer', 'timer.cpp'],\n> ['timer-thread', 'timer-thread.cpp'],\n> + ['unique-fd', 'unique-fd.cpp'],\n> ['utils', 'utils.cpp'],\n> ]\n>\n> diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp\n> new file mode 100644\n> index 000000000000..92ca3f328811\n> --- /dev/null\n> +++ b/test/unique-fd.cpp\n> @@ -0,0 +1,220 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * unique-fd.cpp - UniqueFD test\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"test.h\"\n> +\n> +using namespace libcamera;\n> +using namespace std;\n> +\n> +class UniqueFDTest : public Test\n> +{\n> +protected:\n> +\tint init()\n\noverride ?\n\n> +\t{\n> +\t\treturn createFd();\n> +\t}\n> +\n> +\tint createFd()\n> +\t{\n> +\t\tfd_ = open(\"/tmp\", O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR);\n> +\t\tif (fd_ < 0)\n> +\t\t\treturn TestFail;\n> +\n> +\t\t/* Cache inode number of temp file. */\n> +\t\tstruct stat s;\n> +\t\tif (fstat(fd_, &s))\n> +\t\t\treturn TestFail;\n> +\n> +\t\tinodeNr_ = s.st_ino;\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint run()\n> +\t{\n> +\t\t/* Test creating empty UniqueFD. */\n> +\t\tUniqueFD fd;\n> +\n> +\t\tif (fd.isValid() || fd.get() != -1) {\n> +\t\t\tstd::cout << \"Failed fd check (default constructor)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Test creating UniqueFD from numerical file descriptor. */\n> +\t\tUniqueFD fd2(fd_);\n> +\t\tif (!fd2.isValid() || fd2.get() != fd_) {\n> +\t\t\tstd::cout << \"Failed fd check (fd constructor)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (!isValidFd(fd_)) {\n\nUps, I missed that in the review of the patch that introduced\nUniqueFD, but shouldn't constructing by && reset the parameter to -1 ?\n(This can also be read as: Shouldn't we have UniqueFD(int &&) ?)\n\n> +\t\t\tstd::cout << \"Failed fd validity (fd constructor)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Test move constructor. */\n> +\t\tUniqueFD fd3(std::move(fd2));\n> +\t\tif (!fd3.isValid() || fd3.get() != fd_) {\n> +\t\t\tstd::cout << \"Failed fd check (move constructor)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (fd2.isValid() || fd2.get() != -1) {\n> +\t\t\tstd::cout << \"Failed moved fd check (move constructor)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (!isValidFd(fd_)) {\n> +\t\t\tstd::cout << \"Failed fd validity (move constructor)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Test move assignment operator. */\n> +\t\tfd = std::move(fd3);\n> +\t\tif (!fd.isValid() || fd.get() != fd_) {\n> +\t\t\tstd::cout << \"Failed fd check (move assignment)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (fd3.isValid() || fd3.get() != -1) {\n> +\t\t\tstd::cout << \"Failed moved fd check (move assignment)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (!isValidFd(fd_)) {\n> +\t\t\tstd::cout << \"Failed fd validity (move assignment)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Test swapping. */\n> +\t\tfd2.swap(fd);\n> +\t\tif (!fd2.isValid() || fd2.get() != fd_) {\n> +\t\t\tstd::cout << \"Failed fd check (swap)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (fd.isValid() || fd.get() != -1) {\n> +\t\t\tstd::cout << \"Failed swapped fd check (swap)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (!isValidFd(fd_)) {\n> +\t\t\tstd::cout << \"Failed fd validity (swap)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Test release. */\n> +\t\tint numFd = fd2.release();\n> +\t\tif (fd2.isValid() || fd2.get() != -1) {\n> +\t\t\tstd::cout << \"Failed fd check (release)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (numFd != fd_) {\n> +\t\t\tstd::cout << \"Failed released fd check (release)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (!isValidFd(fd_)) {\n> +\t\t\tstd::cout << \"Failed fd validity (release)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Test reset assignment. */\n> +\t\tfd.reset(numFd);\n> +\t\tif (!fd.isValid() || fd.get() != fd_) {\n> +\t\t\tstd::cout << \"Failed fd check (reset assignment)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (!isValidFd(fd_)) {\n> +\t\t\tstd::cout << \"Failed fd validity (reset assignment)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Test reset destruction. */\n> +\t\tfd.reset();\n> +\t\tif (fd.isValid() || fd.get() != -1) {\n> +\t\t\tstd::cout << \"Failed fd check (reset destruction)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (isValidFd(fd_)) {\n> +\t\t\tstd::cout << \"Failed fd validity (reset destruction)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Test destruction. */\n> +\t\tif (createFd() == TestFail) {\n> +\t\t\tstd::cout << \"Failed to recreate test fd\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t{\n> +\t\t\tUniqueFD fd4(fd_);\n> +\t\t}\n> +\n> +\t\tif (isValidFd(fd_)) {\n> +\t\t\tstd::cout << \"Failed fd validity (destruction)\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n\nnice!\n\nFor the test\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tvoid cleanup()\n> +\t{\n> +\t\tif (fd_ > 0)\n> +\t\t\tclose(fd_);\n> +\t}\n> +\n> +private:\n> +\tbool isValidFd(int fd)\n> +\t{\n> +\t\tstruct stat s;\n> +\t\tif (fstat(fd, &s))\n> +\t\t\treturn false;\n> +\n> +\t\t/* Check that inode number matches cached temp file. */\n> +\t\treturn s.st_ino == inodeNr_;\n> +\t}\n> +\n> +\tint fd_;\n> +\tino_t inodeNr_;\n> +};\n> +\n> +TEST_REGISTER(UniqueFDTest)\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 62020BF415\n\tfor ;\n\tMon, 29 Nov 2021 15:06:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9CBAC605B4;\n\tMon, 29 Nov 2021 16:06:13 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9442B605A0\n\tfor ;\n\tMon, 29 Nov 2021 16:06:12 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 0EEB840014;\n\tMon, 29 Nov 2021 15:06:11 +0000 (UTC)"],"Date":"Mon, 29 Nov 2021 16:07:04 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211129150704.egqvovu477pi767f@uno.localdomain>","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211128235752.10836-6-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21370,"web_url":"https://patchwork.libcamera.org/comment/21370/","msgid":"","date":"2021-11-29T17:31:44","subject":"Re: [libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Nov 29, 2021 at 10:36:17PM +0900, Hirokazu Honda wrote:\n> On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart wrote:\n> >\n> > Add a unit test to exercise the API of the UniqueFD class.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > test/meson.build | 1 +\n> > test/unique-fd.cpp | 220 +++++++++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 221 insertions(+)\n> > create mode 100644 test/unique-fd.cpp\n> >\n> > diff --git a/test/meson.build b/test/meson.build\n> > index d0466f17d7b6..42dfbc1f8ee9 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -53,6 +53,7 @@ internal_tests = [\n> > ['threads', 'threads.cpp'],\n> > ['timer', 'timer.cpp'],\n> > ['timer-thread', 'timer-thread.cpp'],\n> > + ['unique-fd', 'unique-fd.cpp'],\n> > ['utils', 'utils.cpp'],\n> > ]\n> >\n> > diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp\n> > new file mode 100644\n> > index 000000000000..92ca3f328811\n> > --- /dev/null\n> > +++ b/test/unique-fd.cpp\n> > @@ -0,0 +1,220 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * unique-fd.cpp - UniqueFD test\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace libcamera;\n> > +using namespace std;\n> > +\n> > +class UniqueFDTest : public Test\n> > +{\n> > +protected:\n> > + int init()\n>\n> init() override\n\nOops.\n\n> > + {\n> > + return createFd();\n> > + }\n> > +\n> > + int createFd()\n> > + {\n> > + fd_ = open(\"/tmp\", O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR);\n> > + if (fd_ < 0)\n> > + return TestFail;\n> > +\n> > + /* Cache inode number of temp file. */\n> > + struct stat s;\n> > + if (fstat(fd_, &s))\n> > + return TestFail;\n> > +\n> > + inodeNr_ = s.st_ino;\n> > +\n> > + return 0;\n> > + }\n> > +\n> > + int run()\n>\n> run() override\n\nRe-oops :-)\n\n> > + {\n> > + /* Test creating empty UniqueFD. */\n> > + UniqueFD fd;\n> > +\n> > + if (fd.isValid() || fd.get() != -1) {\n> \n> I would not check the initialization value is -1.\n> The point is fd.isValid() is false with a default constructor and the\n> default value is not matter IMO.\n> If you would like, then I would declare the static constexpr value\n> like kInvalidFDValue and compare with it.\n\nIt depends on the API contract I think. The documentation explicitly\nstates that get() returns -1 if no fd is owned, so that's why I'm\nchecking it. This matches std::unique_ptr<>::get() that returns nullptr\nif no object is owned. The alternative is to make get() an undefined\nbehaviour if no fd is owned, and drop the checks here, but I don't think\nthat would be a good idea. If get() is explicitly defined as returning\n-1 when no fd is owned, then I think that should be tested here.\n\n> > + std::cout << \"Failed fd check (default constructor)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Test creating UniqueFD from numerical file descriptor. */\n> > + UniqueFD fd2(fd_);\n> > + if (!fd2.isValid() || fd2.get() != fd_) {\n> > + std::cout << \"Failed fd check (fd constructor)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (!isValidFd(fd_)) {\n> > + std::cout << \"Failed fd validity (fd constructor)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Test move constructor. */\n> > + UniqueFD fd3(std::move(fd2));\n> > + if (!fd3.isValid() || fd3.get() != fd_) {\n> > + std::cout << \"Failed fd check (move constructor)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (fd2.isValid() || fd2.get() != -1) {\n> \n> ditto.\n> \n> > + std::cout << \"Failed moved fd check (move constructor)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (!isValidFd(fd_)) {\n> > + std::cout << \"Failed fd validity (move constructor)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Test move assignment operator. */\n> > + fd = std::move(fd3);\n> > + if (!fd.isValid() || fd.get() != fd_) {\n> > + std::cout << \"Failed fd check (move assignment)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (fd3.isValid() || fd3.get() != -1) {\n> > + std::cout << \"Failed moved fd check (move assignment)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> \n> ditto.\n> \n> > +\n> > + if (!isValidFd(fd_)) {\n> > + std::cout << \"Failed fd validity (move assignment)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Test swapping. */\n> > + fd2.swap(fd);\n> > + if (!fd2.isValid() || fd2.get() != fd_) {\n> > + std::cout << \"Failed fd check (swap)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (fd.isValid() || fd.get() != -1) {\n> > + std::cout << \"Failed swapped fd check (swap)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (!isValidFd(fd_)) {\n> > + std::cout << \"Failed fd validity (swap)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Test release. */\n> > + int numFd = fd2.release();\n> > + if (fd2.isValid() || fd2.get() != -1) {\n> > + std::cout << \"Failed fd check (release)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (numFd != fd_) {\n> > + std::cout << \"Failed released fd check (release)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (!isValidFd(fd_)) {\n> > + std::cout << \"Failed fd validity (release)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Test reset assignment. */\n> > + fd.reset(numFd);\n> > + if (!fd.isValid() || fd.get() != fd_) {\n> > + std::cout << \"Failed fd check (reset assignment)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (!isValidFd(fd_)) {\n> > + std::cout << \"Failed fd validity (reset assignment)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Test reset destruction. */\n> > + fd.reset();\n> > + if (fd.isValid() || fd.get() != -1) {\n> > + std::cout << \"Failed fd check (reset destruction)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (isValidFd(fd_)) {\n> > + std::cout << \"Failed fd validity (reset destruction)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Test destruction. */\n> > + if (createFd() == TestFail) {\n> > + std::cout << \"Failed to recreate test fd\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + {\n> > + UniqueFD fd4(fd_);\n> > + }\n> > +\n> > + if (isValidFd(fd_)) {\n> > + std::cout << \"Failed fd validity (destruction)\"\n> > + << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + return TestPass;\n> > + }\n> > +\n> > + void cleanup()\n> \n> cleanup() override\n> \n> With minor nits,\n> \n> Reviewed-by: Hirokazu Honda \n>\n> > + {\n> > + if (fd_ > 0)\n> > + close(fd_);\n> > + }\n> > +\n> > +private:\n> > + bool isValidFd(int fd)\n> > + {\n> > + struct stat s;\n> > + if (fstat(fd, &s))\n> > + return false;\n> > +\n> > + /* Check that inode number matches cached temp file. */\n> > + return s.st_ino == inodeNr_;\n> > + }\n> > +\n> > + int fd_;\n> > + ino_t inodeNr_;\n> > +};\n> > +\n> > +TEST_REGISTER(UniqueFDTest)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E2D76BF415\n\tfor ;\n\tMon, 29 Nov 2021 17:32:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D12A605A4;\n\tMon, 29 Nov 2021 18:32:10 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB7DB60592\n\tfor ;\n\tMon, 29 Nov 2021 18:32:08 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 39D0C2A5;\n\tMon, 29 Nov 2021 18:32:08 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RBZ98Duc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638207128;\n\tbh=GiaDiIVJCRMc9mbXFB94D/jOoobbaAWQoGYTeLJzyc4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RBZ98DucHFVEYhUYCKM/hLz97jUPhMlfgXwNyGiq5NcpCwLFIEi3IBXL6SGG+0Yic\n\t6BR3YXzqZtz3fAYmRH/m40EcEnV1uhRjtwWowPj6dCINmahd1eexw0RrvQ5tUF8Jp8\n\tkAzzJoHxDT/pER0bZQQeS/e1fnubvHT8oHsUJc+c=","Date":"Mon, 29 Nov 2021 19:31:44 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-6-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21371,"web_url":"https://patchwork.libcamera.org/comment/21371/","msgid":"","date":"2021-11-29T17:41:36","subject":"Re: [libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Nov 29, 2021 at 04:07:04PM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 29, 2021 at 01:57:40AM +0200, Laurent Pinchart wrote:\n> > Add a unit test to exercise the API of the UniqueFD class.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > test/meson.build | 1 +\n> > test/unique-fd.cpp | 220 +++++++++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 221 insertions(+)\n> > create mode 100644 test/unique-fd.cpp\n> >\n> > diff --git a/test/meson.build b/test/meson.build\n> > index d0466f17d7b6..42dfbc1f8ee9 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -53,6 +53,7 @@ internal_tests = [\n> > ['threads', 'threads.cpp'],\n> > ['timer', 'timer.cpp'],\n> > ['timer-thread', 'timer-thread.cpp'],\n> > + ['unique-fd', 'unique-fd.cpp'],\n> > ['utils', 'utils.cpp'],\n> > ]\n> >\n> > diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp\n> > new file mode 100644\n> > index 000000000000..92ca3f328811\n> > --- /dev/null\n> > +++ b/test/unique-fd.cpp\n> > @@ -0,0 +1,220 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * unique-fd.cpp - UniqueFD test\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace libcamera;\n> > +using namespace std;\n> > +\n> > +class UniqueFDTest : public Test\n> > +{\n> > +protected:\n> > +\tint init()\n> \n> override ?\n\nAbsolutely.\n\n> > +\t{\n> > +\t\treturn createFd();\n> > +\t}\n> > +\n> > +\tint createFd()\n> > +\t{\n> > +\t\tfd_ = open(\"/tmp\", O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR);\n> > +\t\tif (fd_ < 0)\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\t/* Cache inode number of temp file. */\n> > +\t\tstruct stat s;\n> > +\t\tif (fstat(fd_, &s))\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\tinodeNr_ = s.st_ino;\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tint run()\n> > +\t{\n> > +\t\t/* Test creating empty UniqueFD. */\n> > +\t\tUniqueFD fd;\n> > +\n> > +\t\tif (fd.isValid() || fd.get() != -1) {\n> > +\t\t\tstd::cout << \"Failed fd check (default constructor)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Test creating UniqueFD from numerical file descriptor. */\n> > +\t\tUniqueFD fd2(fd_);\n> > +\t\tif (!fd2.isValid() || fd2.get() != fd_) {\n> > +\t\t\tstd::cout << \"Failed fd check (fd constructor)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (!isValidFd(fd_)) {\n> \n> Ups, I missed that in the review of the patch that introduced\n> UniqueFD, but shouldn't constructing by && reset the parameter to -1 ?\n> (This can also be read as: Shouldn't we have UniqueFD(int &&) ?)\n\nLet's discuss it in patch 04/17.\n\n> > +\t\t\tstd::cout << \"Failed fd validity (fd constructor)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Test move constructor. */\n> > +\t\tUniqueFD fd3(std::move(fd2));\n> > +\t\tif (!fd3.isValid() || fd3.get() != fd_) {\n> > +\t\t\tstd::cout << \"Failed fd check (move constructor)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (fd2.isValid() || fd2.get() != -1) {\n> > +\t\t\tstd::cout << \"Failed moved fd check (move constructor)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (!isValidFd(fd_)) {\n> > +\t\t\tstd::cout << \"Failed fd validity (move constructor)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Test move assignment operator. */\n> > +\t\tfd = std::move(fd3);\n> > +\t\tif (!fd.isValid() || fd.get() != fd_) {\n> > +\t\t\tstd::cout << \"Failed fd check (move assignment)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (fd3.isValid() || fd3.get() != -1) {\n> > +\t\t\tstd::cout << \"Failed moved fd check (move assignment)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (!isValidFd(fd_)) {\n> > +\t\t\tstd::cout << \"Failed fd validity (move assignment)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Test swapping. */\n> > +\t\tfd2.swap(fd);\n> > +\t\tif (!fd2.isValid() || fd2.get() != fd_) {\n> > +\t\t\tstd::cout << \"Failed fd check (swap)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (fd.isValid() || fd.get() != -1) {\n> > +\t\t\tstd::cout << \"Failed swapped fd check (swap)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (!isValidFd(fd_)) {\n> > +\t\t\tstd::cout << \"Failed fd validity (swap)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Test release. */\n> > +\t\tint numFd = fd2.release();\n> > +\t\tif (fd2.isValid() || fd2.get() != -1) {\n> > +\t\t\tstd::cout << \"Failed fd check (release)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (numFd != fd_) {\n> > +\t\t\tstd::cout << \"Failed released fd check (release)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (!isValidFd(fd_)) {\n> > +\t\t\tstd::cout << \"Failed fd validity (release)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Test reset assignment. */\n> > +\t\tfd.reset(numFd);\n> > +\t\tif (!fd.isValid() || fd.get() != fd_) {\n> > +\t\t\tstd::cout << \"Failed fd check (reset assignment)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (!isValidFd(fd_)) {\n> > +\t\t\tstd::cout << \"Failed fd validity (reset assignment)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Test reset destruction. */\n> > +\t\tfd.reset();\n> > +\t\tif (fd.isValid() || fd.get() != -1) {\n> > +\t\t\tstd::cout << \"Failed fd check (reset destruction)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (isValidFd(fd_)) {\n> > +\t\t\tstd::cout << \"Failed fd validity (reset destruction)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Test destruction. */\n> > +\t\tif (createFd() == TestFail) {\n> > +\t\t\tstd::cout << \"Failed to recreate test fd\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t{\n> > +\t\t\tUniqueFD fd4(fd_);\n> > +\t\t}\n> > +\n> > +\t\tif (isValidFd(fd_)) {\n> > +\t\t\tstd::cout << \"Failed fd validity (destruction)\"\n> > +\t\t\t\t << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> \n> nice!\n> \n> For the test\n> Reviewed-by: Jacopo Mondi \n> \n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tvoid cleanup()\n> > +\t{\n> > +\t\tif (fd_ > 0)\n> > +\t\t\tclose(fd_);\n> > +\t}\n> > +\n> > +private:\n> > +\tbool isValidFd(int fd)\n> > +\t{\n> > +\t\tstruct stat s;\n> > +\t\tif (fstat(fd, &s))\n> > +\t\t\treturn false;\n> > +\n> > +\t\t/* Check that inode number matches cached temp file. */\n> > +\t\treturn s.st_ino == inodeNr_;\n> > +\t}\n> > +\n> > +\tint fd_;\n> > +\tino_t inodeNr_;\n> > +};\n> > +\n> > +TEST_REGISTER(UniqueFDTest)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 227BABDB13\n\tfor ;\n\tMon, 29 Nov 2021 17:42:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 20B3D605A4;\n\tMon, 29 Nov 2021 18:42:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1975260592\n\tfor ;\n\tMon, 29 Nov 2021 18:42:01 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7A1762A5;\n\tMon, 29 Nov 2021 18:42:00 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"k8OgsRc2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638207720;\n\tbh=Z64ZZNnUwcpqFTN6WJcbLb9fTVNcMuCj68XTV5HjrPY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k8OgsRc2FhDl+3ySPXp7DhbJMxz3xIR8ZvMv4t2pCeBJ1RX4lLH33eZlX1AFtEinx\n\tRH78ETsfDCptWUtC+Nbxc6akfPYfV6F7IP7pTIaAp/yLdEAQmhaCt0UIElq1K3sPHY\n\tYdfPt1qKHW1XleKvxGJWqOOF6esEOyrxE/AMTWKI=","Date":"Mon, 29 Nov 2021 19:41:36 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-6-laurent.pinchart@ideasonboard.com>\n\t<20211129150704.egqvovu477pi767f@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211129150704.egqvovu477pi767f@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21382,"web_url":"https://patchwork.libcamera.org/comment/21382/","msgid":"","date":"2021-11-30T01:13:20","subject":"Re: [libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Nov 30, 2021 at 2:32 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Nov 29, 2021 at 10:36:17PM +0900, Hirokazu Honda wrote:\n> > On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart wrote:\n> > >\n> > > Add a unit test to exercise the API of the UniqueFD class.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > test/meson.build | 1 +\n> > > test/unique-fd.cpp | 220 +++++++++++++++++++++++++++++++++++++++++++++\n> > > 2 files changed, 221 insertions(+)\n> > > create mode 100644 test/unique-fd.cpp\n> > >\n> > > diff --git a/test/meson.build b/test/meson.build\n> > > index d0466f17d7b6..42dfbc1f8ee9 100644\n> > > --- a/test/meson.build\n> > > +++ b/test/meson.build\n> > > @@ -53,6 +53,7 @@ internal_tests = [\n> > > ['threads', 'threads.cpp'],\n> > > ['timer', 'timer.cpp'],\n> > > ['timer-thread', 'timer-thread.cpp'],\n> > > + ['unique-fd', 'unique-fd.cpp'],\n> > > ['utils', 'utils.cpp'],\n> > > ]\n> > >\n> > > diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp\n> > > new file mode 100644\n> > > index 000000000000..92ca3f328811\n> > > --- /dev/null\n> > > +++ b/test/unique-fd.cpp\n> > > @@ -0,0 +1,220 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * unique-fd.cpp - UniqueFD test\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"test.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +using namespace std;\n> > > +\n> > > +class UniqueFDTest : public Test\n> > > +{\n> > > +protected:\n> > > + int init()\n> >\n> > init() override\n>\n> Oops.\n>\n> > > + {\n> > > + return createFd();\n> > > + }\n> > > +\n> > > + int createFd()\n> > > + {\n> > > + fd_ = open(\"/tmp\", O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR);\n> > > + if (fd_ < 0)\n> > > + return TestFail;\n> > > +\n> > > + /* Cache inode number of temp file. */\n> > > + struct stat s;\n> > > + if (fstat(fd_, &s))\n> > > + return TestFail;\n> > > +\n> > > + inodeNr_ = s.st_ino;\n> > > +\n> > > + return 0;\n> > > + }\n> > > +\n> > > + int run()\n> >\n> > run() override\n>\n> Re-oops :-)\n>\n> > > + {\n> > > + /* Test creating empty UniqueFD. */\n> > > + UniqueFD fd;\n> > > +\n> > > + if (fd.isValid() || fd.get() != -1) {\n> >\n> > I would not check the initialization value is -1.\n> > The point is fd.isValid() is false with a default constructor and the\n> > default value is not matter IMO.\n> > If you would like, then I would declare the static constexpr value\n> > like kInvalidFDValue and compare with it.\n>\n> It depends on the API contract I think. The documentation explicitly\n> states that get() returns -1 if no fd is owned, so that's why I'm\n> checking it. This matches std::unique_ptr<>::get() that returns nullptr\n> if no object is owned. The alternative is to make get() an undefined\n> behaviour if no fd is owned, and drop the checks here, but I don't think\n> that would be a good idea. If get() is explicitly defined as returning\n> -1 when no fd is owned, then I think that should be tested here.\n>\n\nAck. I am fine to keep the code as-is.\n\n-Hiro\n> > > + std::cout << \"Failed fd check (default constructor)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Test creating UniqueFD from numerical file descriptor. */\n> > > + UniqueFD fd2(fd_);\n> > > + if (!fd2.isValid() || fd2.get() != fd_) {\n> > > + std::cout << \"Failed fd check (fd constructor)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (!isValidFd(fd_)) {\n> > > + std::cout << \"Failed fd validity (fd constructor)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Test move constructor. */\n> > > + UniqueFD fd3(std::move(fd2));\n> > > + if (!fd3.isValid() || fd3.get() != fd_) {\n> > > + std::cout << \"Failed fd check (move constructor)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (fd2.isValid() || fd2.get() != -1) {\n> >\n> > ditto.\n> >\n> > > + std::cout << \"Failed moved fd check (move constructor)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (!isValidFd(fd_)) {\n> > > + std::cout << \"Failed fd validity (move constructor)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Test move assignment operator. */\n> > > + fd = std::move(fd3);\n> > > + if (!fd.isValid() || fd.get() != fd_) {\n> > > + std::cout << \"Failed fd check (move assignment)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (fd3.isValid() || fd3.get() != -1) {\n> > > + std::cout << \"Failed moved fd check (move assignment)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> >\n> > ditto.\n> >\n> > > +\n> > > + if (!isValidFd(fd_)) {\n> > > + std::cout << \"Failed fd validity (move assignment)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Test swapping. */\n> > > + fd2.swap(fd);\n> > > + if (!fd2.isValid() || fd2.get() != fd_) {\n> > > + std::cout << \"Failed fd check (swap)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (fd.isValid() || fd.get() != -1) {\n> > > + std::cout << \"Failed swapped fd check (swap)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (!isValidFd(fd_)) {\n> > > + std::cout << \"Failed fd validity (swap)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Test release. */\n> > > + int numFd = fd2.release();\n> > > + if (fd2.isValid() || fd2.get() != -1) {\n> > > + std::cout << \"Failed fd check (release)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (numFd != fd_) {\n> > > + std::cout << \"Failed released fd check (release)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (!isValidFd(fd_)) {\n> > > + std::cout << \"Failed fd validity (release)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Test reset assignment. */\n> > > + fd.reset(numFd);\n> > > + if (!fd.isValid() || fd.get() != fd_) {\n> > > + std::cout << \"Failed fd check (reset assignment)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (!isValidFd(fd_)) {\n> > > + std::cout << \"Failed fd validity (reset assignment)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Test reset destruction. */\n> > > + fd.reset();\n> > > + if (fd.isValid() || fd.get() != -1) {\n> > > + std::cout << \"Failed fd check (reset destruction)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (isValidFd(fd_)) {\n> > > + std::cout << \"Failed fd validity (reset destruction)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /* Test destruction. */\n> > > + if (createFd() == TestFail) {\n> > > + std::cout << \"Failed to recreate test fd\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + {\n> > > + UniqueFD fd4(fd_);\n> > > + }\n> > > +\n> > > + if (isValidFd(fd_)) {\n> > > + std::cout << \"Failed fd validity (destruction)\"\n> > > + << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + return TestPass;\n> > > + }\n> > > +\n> > > + void cleanup()\n> >\n> > cleanup() override\n> >\n> > With minor nits,\n> >\n> > Reviewed-by: Hirokazu Honda \n> >\n> > > + {\n> > > + if (fd_ > 0)\n> > > + close(fd_);\n> > > + }\n> > > +\n> > > +private:\n> > > + bool isValidFd(int fd)\n> > > + {\n> > > + struct stat s;\n> > > + if (fstat(fd, &s))\n> > > + return false;\n> > > +\n> > > + /* Check that inode number matches cached temp file. */\n> > > + return s.st_ino == inodeNr_;\n> > > + }\n> > > +\n> > > + int fd_;\n> > > + ino_t inodeNr_;\n> > > +};\n> > > +\n> > > +TEST_REGISTER(UniqueFDTest)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 61135BF415\n\tfor ;\n\tTue, 30 Nov 2021 01:13:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3B5E605A4;\n\tTue, 30 Nov 2021 02:13:34 +0100 (CET)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 44443604FC\n\tfor ;\n\tTue, 30 Nov 2021 02:13:32 +0100 (CET)","by mail-ed1-x534.google.com with SMTP id g14so79612929edb.8\n\tfor ;\n\tMon, 29 Nov 2021 17:13:32 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"i+gbriBv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=LWO+GanprJJrJjsHckFmn3Aah4CTC1tPuoSO1IFr1vo=;\n\tb=i+gbriBvlvuZ7Eid5/3VDX3c8wTR2VssIGSwDHaDXE/l6ckexb7cV9HrXHr+uzb9Cl\n\tX55H5Bx9aKIgb41iKncJPON4iKiULa4KGLCwr8Fo8FIJ8QQS/hNwoCjvzbUJStS8DXxv\n\trz+4OhdAFFM4xuzW6qhKUiWLdeb6uZ/nD6esY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=LWO+GanprJJrJjsHckFmn3Aah4CTC1tPuoSO1IFr1vo=;\n\tb=AgwE1FAG2MLD1DvX8JaQE8wfwmFuQODvAn7v92iMDfzCjFDPh9yWEYLUYW0iiAb8qC\n\t7+VY9lSxJyfXwWVhQijHdIKjFx5kBDM9jQsG2eJ02N4Dyu1oz4cI9lNHzEJksxRdibJ5\n\tWa7ToiHBcjXJm/4p02vqM1i7BLEMrrdzK++yHif9+fw7b28kEqALc78Cw83km6seYtnx\n\tPRIE7+px0vSnxDvRVxLX2Blv87yegz8WS+EEVU0Y5Ym0BlcQy/U2lO2c313yve3hGOXF\n\tnQL8ACL8Iw7Hjafv3NZV3D4BvbMEjHdjDsI4oJ3+4P+u+AN7N2mr5lWpJyAxwP/HaZbV\n\t88VA==","X-Gm-Message-State":"AOAM533p/y1jA3u60zcuvh/Tsr1abYTQsyKHI5LSKsGZmJfgKCKIRc4C\n\t09by5J6El/pFbu9eQIGG/8KXzpmEAjv7lMOg3vYumUBl4xc=","X-Google-Smtp-Source":"ABdhPJwx/u6a2nzvJr+/ioZ9oHszDcXG2o1pk+JvZ0IZun0h9nzz8nROPk00aqgBbVTOE+aRBbqdzs6XF9C5MntOQV8=","X-Received":"by 2002:a05:6402:2552:: with SMTP id\n\tl18mr79386686edb.368.1638234811853; \n\tMon, 29 Nov 2021 17:13:31 -0800 (PST)","MIME-Version":"1.0","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-6-laurent.pinchart@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 30 Nov 2021 10:13:20 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] test: Add UniqueFD test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]