[{"id":3267,"web_url":"https://patchwork.libcamera.org/comment/3267/","msgid":"<20191216173725.GD4856@pendragon.ideasonboard.com>","date":"2019-12-16T17:37:25","subject":"Re: [libcamera-devel] [PATCH 3/3] test: file_descriptor: Add test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Mon, Dec 16, 2019 at 01:10:29PM +0100, Niklas Söderlund wrote:\n> Add a test which exercises the whole FileDescriptor interface.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  test/file_descriptor.cpp | 152 +++++++++++++++++++++++++++++++++++++++\n>  test/meson.build         |   1 +\n>  2 files changed, 153 insertions(+)\n>  create mode 100644 test/file_descriptor.cpp\n> \n> diff --git a/test/file_descriptor.cpp b/test/file_descriptor.cpp\n> new file mode 100644\n> index 0000000000000000..b1d0d0e0b2807b20\n> --- /dev/null\n> +++ b/test/file_descriptor.cpp\n> @@ -0,0 +1,152 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * file_descriptor.cpp - FileDescriptor test\n> + */\n> +\n> +#include <fcntl.h>\n> +#include <sys/stat.h>\n> +#include <sys/types.h>\n> +#include <unistd.h>\n> +\n> +#include <libcamera/file_descriptor.h>\n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n\nAlphabetically sorted ?\n\n> +\n> +class FileDescriptorTest : public Test\n> +{\n> +protected:\n> +\tint init()\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\treturn 0;\n> +\t}\n> +\n> +\tint run()\n> +\t{\n> +\t\tFileDescriptor *desc1, *desc2;\n> +\n> +\t\t/* Test creating FileDescriptor from numerical file descriptor. */\n> +\t\tdesc1 = new FileDescriptor(fd_);\n> +\t\tif (desc1->fd() == fd_)\n\nYou're leaking desc1. There are plenty of leaks below too. It's\nimportant to avoid leaks in tests, as otherwise valgrind will catch them\nand we won't be able to tell whether libcamera or the tests are leaky.\nYou could make them class members and delete them in the destructor.\nDon't forget to set the pointers to nullptr in the constructor, as well\nas after every manual delete to avoid double-delete.\n\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (!validFD(fd_) || !validFD(desc1->fd()))\n> +\t\t\treturn TestFail;\n> +\n> +\t\tdelete desc1;\n> +\n> +\t\tif (!validFD(fd_))\n> +\t\t\treturn TestFail;\n\nI would add\n\n\t\tint fd = desc1->fd();\n\nbefore the delete, and replace the above check with\n\n\t\tif (!validFD(fd_) || validFD(fd))\n\t\t\treturn TestFail;\n\nto ensure that deleting the FileDescriptor closes the managed fd.\n\n> +\n> +\t\t/* Test creating FileDescriptor from other FileDescriptor. */\n> +\t\tdesc1 = new FileDescriptor(fd_);\n> +\t\tdesc2 = new FileDescriptor(*desc1);\n> +\n> +\t\tif (desc1->fd() == fd_ || desc2->fd() == fd_ || desc1->fd() == desc2->fd())\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (!validFD(fd_) || !validFD(desc1->fd()) || !validFD(desc2->fd()))\n> +\t\t\treturn TestFail;\n> +\n> +\t\tdelete desc1;\n> +\n> +\t\tif (!validFD(fd_) || !validFD(desc2->fd()))\n> +\t\t\treturn TestFail;\n> +\n> +\t\tdelete desc2;\n> +\n> +\t\tif (!validFD(fd_))\n> +\t\t\treturn TestFail;\n\nI think you can drop the three !validFD(fd_) checks above, as well as\nall the same checks below, as this is already tested by the first test\ncase.\n\n> +\n> +\t\t/* Test creating FileDescriptor by taking over other FileDescriptor. */\n> +\t\tdesc1 = new FileDescriptor(fd_);\n> +\t\tdesc2 = new FileDescriptor(std::move(*desc1));\n> +\n> +\t\tif (desc1->fd() != -1 || desc2->fd() == fd_ || desc1->fd() == desc2->fd())\n> +\t\t\treturn TestFail;\n\nYou want here to make sure that constructing desc2 didn't dup desc1.\n\n\t\tdesc1 = new FileDescriptor(fd_);\n\t\tfd = desc1->fd();\n\t\tdesc2 = new FileDescriptor(std::move(*desc1));\n\n\t\tif (desc1->fd() != -1 || desc2->fd() != fd)\n\t\t\treturn TestFail;\n\n> +\n> +\t\tif (!validFD(fd_) || !validFD(desc2->fd()))\n> +\t\t\treturn TestFail;\n> +\n> +\t\tdelete desc1;\n> +\t\tdelete desc2;\n> +\n> +\t\tif (!validFD(fd_))\n> +\t\t\treturn TestFail;\n> +\n> +\t\t/* Test creating FileDescriptor by copy assigment */\n\ns/assignment/assignment./\n\n> +\t\tdesc1 = new FileDescriptor();\n> +\t\tdesc2 = new FileDescriptor(fd_);\n> +\n> +\t\tif (desc1->fd() != -1 || desc2->fd() == fd_ || desc1->fd() == desc2->fd())\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (!validFD(fd_) || !validFD(desc2->fd()))\n> +\t\t\treturn TestFail;\n\nI think you can drop all these checks except for desc1->fd() != -1 as\nthat hasn't been tested before.\n\n> +\n> +\t\t*desc1 = *desc2;\n> +\n> +\t\tif (desc1->fd() == fd_ || desc2->fd() == fd_ || desc1->fd() == desc2->fd())\n> +\t\t\treturn TestFail;\n\nSimilarly as before,\n\n\t\tfd = desc2->fd();\n\nbefore the assignment, and the test should be\n\n\t\tif (desc1->fd() == fd || desc2->fd() != fd)\n\t\t\treturn TestFail;\n\nto ensure that desc1 did dup and the desc2 wasn't modified.\n\n> +\n> +\t\tif (!validFD(fd_) || !validFD(desc1->fd()) || !validFD(desc2->fd()))\n> +\t\t\treturn TestFail;\n> +\n> +\t\tdelete desc1;\n> +\t\tdelete desc2;\n> +\n> +\t\tif (!validFD(fd_))\n> +\t\t\treturn TestFail;\n> +\n> +\t\t/* Test creating FileDescriptor by move assigment */\n\ns/assignment/assignment./\n\n> +\t\tdesc1 = new FileDescriptor();\n> +\t\tdesc2 = new FileDescriptor(fd_);\n> +\n> +\t\tif (desc1->fd() != -1 || desc2->fd() == fd_ || desc1->fd() == desc2->fd())\n> +\t\t\treturn TestFail;\n> +\n> +\t\tif (!validFD(fd_) || !validFD(desc2->fd()))\n> +\t\t\treturn TestFail;\n> +\n\nThose checks can be dropped too.\n\n> +\t\t*desc1 = std::move(*desc2);\n> +\n> +\t\tif (desc1->fd() == fd_ || desc2->fd() != -1 || desc1->fd() == desc2->fd())\n> +\t\t\treturn TestFail;\n\nAnd same as before,\n\n\t\tif (desc1->fd() != fd || desc2->fd() != -1)\n\t\t\treturn TestFail;\n\nwith\n\n\t\tfd = desc2->fd();\n\nbefore the assignment.\n\n> +\n> +\t\tif (!validFD(fd_) || !validFD(desc1->fd()))\n> +\t\t\treturn TestFail;\n> +\n> +\t\tdelete desc1;\n> +\t\tdelete desc2;\n> +\n> +\t\tif (!validFD(fd_))\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tvoid cleanup()\n> +\t{\n> +\t\tif (fd_ > 0)\n\n>= 0, or != -1\n\n> +\t\t\tclose(fd_);\n> +\t}\n> +\n> +private:\n> +\tbool validFD(int fd)\n\nThis method should be static.\n\ns/validFD/isValidFd/ (or validFd) ?\n\n> +\t{\n> +\t\tstruct stat s;\n> +\t\treturn fstat(fd, &s) == 0;\n\nWhat are you trying to catch here, that the fd is still open ? Maybe the\nmethod should then be called isFdOpen() ? If you want to go one step\nfurther you could also compare s.st_ino with a saved value retrieved in\ninit() after creating the temporary file. Then the method shouldn't be\nstatic, and can be called isValidFd(). A one-line comment explaining\nwhat is checked would be useful in any case.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +\t}\n> +\n> +\tint fd_;\n> +};\n> +\n> +TEST_REGISTER(FileDescriptorTest)\n> diff --git a/test/meson.build b/test/meson.build\n> index 1bb2161dc05a7b1d..9dc392df30efd956 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -25,6 +25,7 @@ internal_tests = [\n>      ['event',                           'event.cpp'],\n>      ['event-dispatcher',                'event-dispatcher.cpp'],\n>      ['event-thread',                    'event-thread.cpp'],\n> +    ['file_descriptor',                 'file_descriptor.cpp'],\n>      ['message',                         'message.cpp'],\n>      ['object',                          'object.cpp'],\n>      ['object-invoke',                   'object-invoke.cpp'],","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["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 C2432601E4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Dec 2019 18:37:35 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2E1C4DD;\n\tMon, 16 Dec 2019 18:37:35 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576517855;\n\tbh=R/sSOHqERXQkwCgW8FD36aYAheznf98Vwi0+c+WH8+E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M9NJoS8f9R57VsODXmyIXYQf0wFDEWMEsRqT1inIMQy1/s2YM/mVE5ANFzeXNRffc\n\tJ5W6JS7zAhGW2uEy4N7vQ7FTwoO2TKLgzI8hu7g/ccAjFCTObie6skHSxxC2u3B6VT\n\tS7NUqmtKmaAHO3io1CrzPnT9VJdPC8iM5uuxdP/g=","Date":"Mon, 16 Dec 2019 19:37:25 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191216173725.GD4856@pendragon.ideasonboard.com>","References":"<20191216121029.1048242-1-niklas.soderlund@ragnatech.se>\n\t<20191216121029.1048242-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191216121029.1048242-4-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/3] test: file_descriptor: Add test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 16 Dec 2019 17:37:36 -0000"}}]