Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Knut Omang <knut.omang@oracle.com>
Cc: linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kbuild@vger.kernel.org,
	Shuah Khan <shuah@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Michal Marek <michal.lkml@markovi.net>,
	Shreyans Devendra Doshi <0xinfosect0r@gmail.com>,
	Alan Maguire <alan.maguire@oracle.com>,
	Brendan Higgins <brendanhiggins@google.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Hidenori Yamaji <hidenori.yamaji@sony.com>,
	Frank Rowand <frowand.list@gmail.com>,
	Timothy Bird <Tim.Bird@sony.com>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Theodore Ts'o <tytso@mit.edu>, Daniel Vetter <daniel@ffwll.ch>,
	Stephen Boyd <sboyd@kernel.org>
Subject: Re: [RFC 06/19] ktf: A simple debugfs interface to test results
Date: Tue, 13 Aug 2019 10:21:52 +0200
Message-ID: <20190813082152.GA17627@kroah.com> (raw)
In-Reply-To: <ae6c38384e2338aa3cfb8a4e4dd1002833789253.1565676440.git-series.knut.omang@oracle.com>

On Tue, Aug 13, 2019 at 08:09:21AM +0200, Knut Omang wrote:
> From: Alan Maguire <alan.maguire@oracle.com>
> 
> While test results is available via netlink from user space, sometimes
> it may be useful to be able to access the results from the kernel as well,
> for instance due to a crash. Make that possible via debugfs.
> 
> ktf_debugfs.h:   Support for creating a debugfs representation of test
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  tools/testing/selftests/ktf/kernel/ktf_debugfs.c | 356 ++++++++++++++++-
>  tools/testing/selftests/ktf/kernel/ktf_debugfs.h |  34 ++-
>  2 files changed, 390 insertions(+)
>  create mode 100644 tools/testing/selftests/ktf/kernel/ktf_debugfs.c
>  create mode 100644 tools/testing/selftests/ktf/kernel/ktf_debugfs.h
> 
> diff --git a/tools/testing/selftests/ktf/kernel/ktf_debugfs.c b/tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> new file mode 100644
> index 0000000..a20fbd2
> --- /dev/null
> +++ b/tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> @@ -0,0 +1,356 @@
> +/*
> + * Copyright (c) 2009, 2017, Oracle and/or its affiliates. All rights reserved.
> + *    Author: Alan Maguire <alan.maguire@oracle.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0

Has to be the first line of the file, did you run this through
checkpatch?

> +static int ktf_run_test_open(struct inode *inode, struct file *file)
> +{
> +	struct ktf_test *t;
> +
> +	if (!try_module_get(THIS_MODULE))
> +		return -EIO;

This is an anti-pattern, and one guaranteed to not work properly.  NEVER
do this.

> +
> +	t = (struct ktf_test *)inode->i_private;
> +
> +	return single_open(file, ktf_debugfs_run, t);
> +}
> +
> +static int ktf_debugfs_release(struct inode *inode, struct file *file)
> +{
> +	module_put(THIS_MODULE);

Same here, not ok.


> +	return single_release(inode, file);
> +}
> +
> +static const struct file_operations ktf_run_test_fops = {
> +	.open = ktf_run_test_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = ktf_debugfs_release,
> +};
> +
> +static int ktf_results_test_open(struct inode *inode, struct file *file)
> +{
> +	struct ktf_test *t;
> +
> +	if (!try_module_get(THIS_MODULE))
> +		return -EIO;

Nope!

And why -EIO?  That is not an io issue.

> +void ktf_debugfs_create_test(struct ktf_test *t)
> +{
> +	struct ktf_case *testset = ktf_case_find(t->tclass);
> +
> +	if (!testset)
> +		return;
> +
> +	memset(&t->debugfs, 0, sizeof(t->debugfs));
> +
> +	t->debugfs.debugfs_results_test =
> +		debugfs_create_file(t->name, S_IFREG | 0444,
> +				    testset->debugfs.debugfs_results_test,
> +				 t, &ktf_results_test_fops);
> +
> +	if (t->debugfs.debugfs_results_test) {

How can that variable ever be NULL (hint, it can not.)

> +		t->debugfs.debugfs_run_test =
> +			debugfs_create_file(t->name, S_IFREG | 0444,
> +					    testset->debugfs.debugfs_run_test,
> +				 t, &ktf_run_test_fops);
> +		if (!t->debugfs.debugfs_run_test) {
> +			_ktf_debugfs_destroy_test(t);
> +		} else {
> +			/* Take reference for test for debugfs */
> +			ktf_test_get(t);
> +		}
> +	}

Never test the result of any debugfs call, you do not need to.  Just
call it and move on, your code flow should NEVER be different with, or
without, a successful debugfs call.


> +static int ktf_run_testset_open(struct inode *inode, struct file *file)
> +{
> +	struct ktf_case *testset;
> +
> +	if (!try_module_get(THIS_MODULE))
> +		return -EIO;

Again no.  I hate to know what code you copied this all from, as that
code is very wrong.  Do you have a pointer to that code anywhere so we
can fix that up?

> +
> +	testset = (struct ktf_case *)inode->i_private;
> +
> +	return single_open(file, ktf_debugfs_run_all, testset);
> +}
> +
> +static const struct file_operations ktf_run_testset_fops = {
> +	.open = ktf_run_testset_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = ktf_debugfs_release,

If you really care about module references you should be setting the
owner of the module here.

> +};
> +
> +static void _ktf_debugfs_destroy_testset(struct ktf_case *testset)
> +{
> +	debugfs_remove(testset->debugfs.debugfs_run_testset);
> +	debugfs_remove(testset->debugfs.debugfs_run_test);
> +	debugfs_remove(testset->debugfs.debugfs_results_testset);
> +	debugfs_remove(testset->debugfs.debugfs_results_test);

Why not just recursivly remove the directory?  That way you do not have
to keep track of any individual files.


> +}
> +
> +void ktf_debugfs_create_testset(struct ktf_case *testset)
> +{
> +	char tests_subdir[KTF_DEBUGFS_NAMESZ];
> +	const char *name = ktf_case_name(testset);
> +
> +	memset(&testset->debugfs, 0, sizeof(testset->debugfs));
> +
> +	/* First add /sys/kernel/debug/ktf/[results|run]/<testset> */
> +	testset->debugfs.debugfs_results_testset =
> +		debugfs_create_file(name, S_IFREG | 0444,
> +				    ktf_debugfs_resultsdir,
> +				 testset, &ktf_results_testset_fops);
> +	if (!testset->debugfs.debugfs_results_testset)
> +		goto err;

Again, can never happen, and again, do not do different things depending
on the result of a debugfs call.

> +
> +	testset->debugfs.debugfs_run_testset =
> +		debugfs_create_file(name, S_IFREG | 0444,
> +				    ktf_debugfs_rundir,
> +				    testset, &ktf_run_testset_fops);
> +	if (!testset->debugfs.debugfs_run_testset)
> +		goto err;

Again, nope.

> +
> +	/* Now add parent directories for individual test result/run tests
> +	 * which live in
> +	 * /sys/kernel/debug/ktf/[results|run]/<testset>-tests/<testname>
> +	 */
> +	(void)snprintf(tests_subdir, sizeof(tests_subdir), "%s%s",
> +			name, KTF_DEBUGFS_TESTS_SUFFIX);

why (void)?


> +
> +	testset->debugfs.debugfs_results_test =
> +		debugfs_create_dir(tests_subdir, ktf_debugfs_resultsdir);
> +	if (!testset->debugfs.debugfs_results_test)
> +		goto err;

nope :)

> +
> +	testset->debugfs.debugfs_run_test =
> +		debugfs_create_dir(tests_subdir, ktf_debugfs_rundir);
> +	if (!testset->debugfs.debugfs_run_test)
> +		goto err;

Nope :)

> +
> +	/* Take reference count for testset.  One will do as we will always
> +	 * free testset debugfs resources together.
> +	 */
> +	ktf_case_get(testset);
> +	return;
> +err:
> +	_ktf_debugfs_destroy_testset(testset);
> +}
> +
> +void ktf_debugfs_destroy_testset(struct ktf_case *testset)
> +{
> +	tlog(T_DEBUG, "Destroying debugfs testset %s", ktf_case_name(testset));
> +	_ktf_debugfs_destroy_testset(testset);
> +	/* Remove our debugfs reference cout to testset */
> +	ktf_case_put(testset);
> +}
> +
> +/* /sys/kernel/debug/ktf/coverage shows coverage statistics. */
> +static int ktf_debugfs_cov(struct seq_file *seq, void *v)
> +{
> +	ktf_cov_seq_print(seq);
> +
> +	return 0;
> +}
> +
> +static int ktf_cov_open(struct inode *inode, struct file *file)
> +{
> +	if (!try_module_get(THIS_MODULE))
> +		return -EIO;

{sigh}  I'll stop reviewing now :)

thanks,

greg k-h

  reply index

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-13  6:09 [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree Knut Omang
2019-08-13  6:09 ` [RFC 01/19] kbuild: Fixes to rules for host-cshlib and host-cxxshlib Knut Omang
2019-08-13 14:01   ` Masahiro Yamada
2019-08-13 16:19     ` Knut Omang
2019-08-14  2:02       ` Masahiro Yamada
2019-08-14  5:50         ` Knut Omang
2019-08-14  5:52         ` Knut Omang
2019-08-14 12:52           ` Knut Omang
2019-08-21  1:47             ` Masahiro Yamada
2019-08-21  4:03               ` Knut Omang
2019-08-13  6:09 ` [RFC 02/19] ktf: Introduce the main part of the kernel side of ktf Knut Omang
2019-09-09  1:23   ` Brendan Higgins
2019-09-10  6:15     ` Knut Omang
2019-08-13  6:09 ` [RFC 03/19] ktf: Introduce a generic netlink protocol for test result communication Knut Omang
2019-09-09  1:28   ` Brendan Higgins
2019-09-10  6:30     ` Knut Omang
2019-08-13  6:09 ` [RFC 04/19] ktf: An implementation of a generic associative array container Knut Omang
2019-08-13  6:09 ` [RFC 05/19] ktf: Implementation of ktf support for overriding function entry and return Knut Omang
2019-08-13  6:09 ` [RFC 06/19] ktf: A simple debugfs interface to test results Knut Omang
2019-08-13  8:21   ` Greg Kroah-Hartman [this message]
2019-08-14 17:17     ` Knut Omang
2019-08-15  8:49       ` Greg Kroah-Hartman
2019-08-15 10:35         ` Knut Omang
2019-08-15 10:52           ` Greg Kroah-Hartman
2019-08-13  6:09 ` [RFC 07/19] ktf: Simple coverage support Knut Omang
2019-08-13  6:09 ` [RFC 08/19] ktf: Configurable context support for network info setup Knut Omang
2019-08-13  6:09 ` [RFC 09/19] ktf: resolve: A helper utility to aid in exposing private kernel symbols to KTF tests Knut Omang
2019-08-13  6:09 ` [RFC 10/19] ktf: Add documentation for Kernel Test Framework (KTF) Knut Omang
2019-08-13  6:09 ` [RFC 11/19] ktf: Add a small test suite with a few tests to test KTF itself Knut Omang
2019-08-13  6:09 ` [RFC 12/19] ktf: Main part of user land library for executing tests Knut Omang
2019-08-13  6:09 ` [RFC 13/19] ktf: Integration logic for running ktf tests from googletest Knut Omang
2019-08-13  6:09 ` [RFC 14/19] ktf: Internal debugging facilities Knut Omang
2019-08-13  6:09 ` [RFC 15/19] ktf: Some simple examples Knut Omang
2019-08-13  6:09 ` [RFC 16/19] ktf: Some user applications to run tests Knut Omang
2019-08-13  6:09 ` [RFC 17/19] ktf: Toplevel ktf Makefile/makefile includes and scripts to run from kselftest Knut Omang
2019-08-13  6:09 ` [RFC 18/19] kselftests: Enable building ktf Knut Omang
2019-08-13  6:09 ` [RFC 19/19] Documentation/dev-tools: Add index entry for KTF documentation Knut Omang
2019-08-13  8:10 ` [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree Brendan Higgins
2019-08-13  8:17 ` Brendan Higgins
2019-08-13 11:29   ` Knut Omang
2019-08-13 17:50     ` Brendan Higgins
2019-08-13  8:23 ` Greg Kroah-Hartman
2019-08-13  9:51   ` Knut Omang
2019-08-13 17:02     ` Brendan Higgins

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190813082152.GA17627@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=0xinfosect0r@gmail.com \
    --cc=Tim.Bird@sony.com \
    --cc=alan.maguire@oracle.com \
    --cc=brendanhiggins@google.com \
    --cc=corbet@lwn.net \
    --cc=daniel@ffwll.ch \
    --cc=frowand.list@gmail.com \
    --cc=hidenori.yamaji@sony.com \
    --cc=khilman@baylibre.com \
    --cc=knut.omang@oracle.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=michal.lkml@markovi.net \
    --cc=sboyd@kernel.org \
    --cc=shuah@kernel.org \
    --cc=tytso@mit.edu \
    --cc=yamada.masahiro@socionext.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org linux-kselftest@archiver.kernel.org
	public-inbox-index linux-kselftest


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/ public-inbox