On 8/30/2017 4:12 AM, Michał Winiarski wrote:
On Tue, Aug 29, 2017 at 02:25:19PM -0700, Vinay Belgaumkar wrote:
Added the missing IGT_TEST_DESCRIPTION and some subtest
descriptions.
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
---
tests/gem_flink_basic.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/tests/gem_flink_basic.c b/tests/gem_flink_basic.c
index 26ae7d6..8761e0d 100644
--- a/tests/gem_flink_basic.c
+++ b/tests/gem_flink_basic.c
@@ -36,6 +36,8 @@
#include <sys/ioctl.h>
#include "drm.h"
+IGT_TEST_DESCRIPTION("Tests for flink - a way to export a gem object by name");
+
static void
test_flink(int fd)
{
@@ -155,14 +157,48 @@ igt_main
igt_fixture
fd = drm_open_driver(DRIVER_INTEL);
+ /* basic:
+ This subtest creates a gem object, and then creates
+ a flink. It tests that we can gain access to the gem
+ object using the flink name.
+
+ Test fails if flink creation/open fails.
+ **/
Please use kernel coding style.
This is not the format we're using for multiline comments.
/*
*
*/
^^^ This is the format we're using.
Agreed. Will change it to match that style. The multi-line comments in /lib
directory actually use this-
/**
* <name of function>
*/
And on the documentation itself, let's take a quote from the kernel coding
style:
"Comments are good, but there is also a danger of over-commenting. NEVER
try to explain HOW your code works in a comment: it's much better to
write the code so that the **working** is obvious, and it's a waste of
time to explain badly written code."
Now, let's try to match the tests with the comments:
/* This subtest creates a gem object */
ret = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
igt_assert_eq(ret, 0);
/* and then creates a flink */
flink.handle = create.handle;
ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink);
igt_assert_eq(ret, 0);
/* It tests that we can gain access to the gem object using the flink
* name
*/
Well... not really, we're not accessing the object in any way.
Yes, but we are trying to open the flink in this line of the test-
open_struct.name = flink.name;
ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, &open_struct);
igt_assert_eq(ret, 0);
igt_assert(open_struct.handle != 0);
I will change it to "open the flink" instead of "access the gem object".
/* Test fails if flink creation/open fails. */
open_struct.name = flink.name;
ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, &open_struct);
igt_assert_eq(ret, 0);
igt_assert(open_struct.handle != 0);
igt_subtest("basic")
test_flink(fd);
+
+ /* double-flink:
+ This test checks if it is possible to create 2 flinks
+ for the same gem object.
+
+ Test fails if 2 flink objects cannot be created.
+ **/
/* This test checks if it is possible to create 2 flinks for the same
* gem object
*/
flink.handle = create.handle;
ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink);
igt_assert_eq(ret, 0);
flink2.handle = create.handle;
ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink2);
igt_assert_eq(ret, 0);
/* Test fails if 2 flink objects cannot be created. */
Well - this is handled by the asserts above.
You ignored this assumption in your description for some reason though:
igt_assert(flink2.name == flink.name);
Agreed. Also need to add that comment saying the name remains the same
across the two
applications opening the same gem object.
igt_subtest("double-flink")
test_double_flink(fd);
+
+ /* bad-flink:
+ Use an invalid flink handle.
+
+ DRM_IOCTL_GEM_FLINK ioctl call should return failure.
+ **/
ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink);
igt_assert(ret == -1 && errno == ENOENT);
There is also an igt_info message:
igt_info("Testing error return on bad flink ioctl.\n");
True, there is some duplication in the comments at this point.
The documentation that I am adding before the subtest call will be rolled up
by gtkdoc/Sphinx/doxygen, it likely
will not look at the text documentation in the actual code. When we look at
the rolled up documentation, it
is good to have an idea of when a particular test will pass/fail without
having to dig into code.
So, yes, there will be some duplication for existing tests. But if we start
following this method for new tests,
we can have one place to describe what the test does/when does it fail, and
then expand on anything that is
not very clear in the code itself.
igt_subtest("bad-flink")
test_bad_flink(fd);
+
+ /* bad-open:
+ Try to use an invalid flink name.
+
+ DRM_IOCTL_GEM_FLINK ioctl call should return failure.
+ **/
open_struct.name = 0x10101010;
ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, &open_struct);
igt_assert(ret == -1 && errno == ENOENT);
Same as for bad flink:
igt_info("Testing error return on bad open ioctl.\n");
igt_subtest("bad-open")
test_bad_open(fd);
+
+ /* flink-lifetime:
+ Check if a flink name can be used even after the drm
+ fd used to create it is closed.
+
+ Flink name should remain valid until the gem object
+ it points to has not been freed.
+ **/
That's better, however...
Why wasn't the object freed when we closed the drm fd (fd2) used to create it?
(hint, it wasn't freed because we're doing OPEN using a different fd before
closing fd2, and that changes the lifetime of an object since we're bumping the
refcount this way, which perhaps could use a comment, not in the description
but in the testcase itself).
As for a one-line description, perhaps something more general would work better?
Check if a flink name is valid for the whole duration of underlying gem object
lifetime.
Agreed. In this case, it makes more sense to have this clarification in the
actual test code itself.
Overall - do you believe, that 1:1 from C to English translation is not a
perfect example of "over-commenting"? Do we really need to take an approach
where we're documenting even the simple ABI checks (e.g. invalid usage - error)?
What value does such documentation have and for whom? I would expect developers
to be able to consume C, are we trying to explain things for non-developers?
-Michał
I am not suggesting we do a C to English translation. The point of this
patch is to encourage
documenting of subtests so that anyone who is starting out with kernel/test
development
can get a better idea of what the test is doing and why, before digging into
the actual code.
