Re: [i-g-t PATCH v3 3/3] Convert shell script tests to C version

[Abdiel Janulgue <abdiel.janulgue@linux.intel.com>]

On 12.06.2017 14:14, Arkadiusz Hiler wrote:
> On Tue, Jun 06, 2017 at 11:54:14AM +0300, Abdiel Janulgue wrote:
>> v3: Drop redundant test covered by drv_hangman/basic. Descend thru
>>     debugfs path when reading sysfs entries (Chris).
>>
>> v2: Use internal igt_debugfs functions instead of cat and document
>>     debugfs tests.
>>     Convert sysfs_l3_parity properly.
>>     Rename redundant names in tests.
>>
>> Converted:
>>  - check_drm_clients (ensures no other clients are running.
>>    functionality provided by drm_open_driver_master).
>>  - debugfs_emon_crash
>>  - debugfs_wedged
>>  - drv_debugfs_reader
>>  - sysfs_l3_parity
>>  - test_rte_check  (same as check_drm_clients)
>>  - tools_test
>>  - ZZ_check_dmesg
> 
> Hey,
> 
> Doing each tool in a separate commit would help with readability.

Will do.

> 
>>
>> Cc: Petri Latvala <petri.latvala@intel.com>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
>> ---
>>  tests/Makefile.sources   |   9 +---
>>  tests/ZZ_check_dmesg     |  11 -----
>>  tests/check_drm_clients  |   6 ---
>>  tests/debugfs.c          |  96 +++++++++++++++++++++++++++++++++++++
>>  tests/debugfs_emon_crash |  16 -------
>>  tests/debugfs_wedged     |  10 ----
>>  tests/drv_debugfs_reader |   9 ----
>>  tests/sysfs_l3_parity    |  22 ---------
>>  tests/test_rte_check     |   6 ---
>>  tests/tools.c            | 122 +++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/tools_test         |  16 -------
>>  11 files changed, 220 insertions(+), 103 deletions(-)
>>  delete mode 100755 tests/ZZ_check_dmesg
>>  delete mode 100755 tests/check_drm_clients
>>  create mode 100644 tests/debugfs.c
>>  delete mode 100755 tests/debugfs_emon_crash
>>  delete mode 100755 tests/debugfs_wedged
>>  delete mode 100755 tests/drv_debugfs_reader
>>  delete mode 100755 tests/sysfs_l3_parity
>>  delete mode 100755 tests/test_rte_check
>>  create mode 100644 tests/tools.c
>>  delete mode 100755 tests/tools_test
>>
>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>> index 9553e4d..c4a78a9 100644
>> --- a/tests/Makefile.sources
>> +++ b/tests/Makefile.sources
>> @@ -243,6 +243,8 @@ TESTS_progs = \
>>  	drv_module_reload \
>>  	kms_sysfs_edid_timing \
>>  	perf \
>> +	debugfs \
> 
> The name clashes on AOSP build:
> build/core/base_rules.mk:238: error: external/igt/tests: MODULE.TARGET.EXECUTABLES.debugfs already defined by external/e2fsprogs/debugfs.
> 
> debugfs is a name of a binary that debugs ext{2,3,4} family of FSes.
> 
> I have to look into the build system and our Android.mks more deeply as
> IGTs output binaries should be stored elsewhere and the clash is rather
> enigmatic.
> 
> Also are we changing names of the test? I thought it was only a rewrite.

It's just a rewrite. Now that we combine those debugfs_* scripts into
one executable, maybe just the prefix would suffice? How about
debugfs_tests.c?

> 
>> +	tools \
>>  	$(NULL)
>>  
>>  # IMPORTANT: The ZZ_ tests need to be run last!
>> @@ -251,11 +253,6 @@ TESTS_scripts_M = \
>>  	$(NULL)
>>  
>>  TESTS_scripts = \
>> -	debugfs_emon_crash \
>> -	drv_debugfs_reader \
>> -	sysfs_l3_parity \
>> -	test_rte_check \
>> -	tools_test \
>>  	$(NULL)
> 
> TESTS_scripts now is only $(NULL), so you may as well just remove it
> from the `kernel_tests` variable and get rid of it completely.
> 
>>  
>>  # This target contains testcases which support automagic subtest enumeration
>> @@ -317,9 +314,7 @@ HANG = \
>>  	$(NULL)
>>  
>>  scripts = \
>> -	check_drm_clients \
>>  	ddx_intel_after_fbdev \
>> -	debugfs_wedged \
>>  	drm_lib.sh \
>>  	drm_getopt.sh \
>>  	$(NULL)
> 
> <SNIP>
> 
>> diff --git a/tests/debugfs.c b/tests/debugfs.c
>> new file mode 100644
>> index 0000000..b9ae86c
>> --- /dev/null
>> +++ b/tests/debugfs.c
>> @@ -0,0 +1,96 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +#ifdef HAVE_CONFIG_H
>> +#include "config.h"
>> +#endif
>> +#include "igt.h"
>> +#include "igt_sysfs.h"
>> +#include <fcntl.h>
>> +#include <sys/types.h>
>> +#include <dirent.h>
>> +
>> +static void get_sysfs_entry(int path_fd)
> 
> I was a little bit confused by the function - we are not getting
> anything from it. Just reading and discarding.
> 
> Maybe read_and_discard_sysfs_entry? A comment would also do.

read_and_discard_sysfs_entry() sounds better.

> 
>> +{
>> +	struct dirent *dirent;
>> +	DIR *dir;
>> +
>> +	dir = fdopendir(path_fd);
>> +	if (!dir)
>> +		return;
>> +
>> +	while ((dirent = readdir(dir))) {
>> +		if (!strcmp(dirent->d_name, ".") ||
>> +		    !strcmp(dirent->d_name, ".."))
>> +			continue;
>> +		if (dirent->d_type == DT_DIR) {
>> +			int sub_fd = -1;
>> +			igt_assert((sub_fd =
>> +				    openat(path_fd, dirent->d_name, O_RDONLY |
>> +					   O_DIRECTORY)) > 0);
>> +			get_sysfs_entry(sub_fd);
>> +			close(sub_fd);
>> +		} else {
>> +			char *buf = igt_sysfs_get(path_fd, dirent->d_name);
> 
> igt_sysfs_get may get us NULL.
> Shouldn't we assert on that? It's an error-worthy.

Yep.

> <SNIP>

>> +		igt_assert(asprintf(&cmd, "dmesg | grep "
>> +				    "-- '------\\[ cut here \\]----'")
>> +			   != -1);
>> +		igt_assert(igt_system_quiet(cmd) != IGT_EXIT_SUCCESS);
>> +		free(cmd);
> 
> Are we testing the dmesg here?
> 
> If not they why systeming out "dmesg | grep" instead of going through
> /dev/kmsg?

Yes. This could be improved. Thanks for the review!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx