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.
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.
/* 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);
> 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");
> 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.
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ł
> igt_subtest("flink-lifetime")
> test_flink_lifetime(fd);
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
