LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Shuah Khan <skhan@linuxfoundation.org>
To: Bart Van Assche <bvanassche@acm.org>, Christoph Hellwig <hch@lst.de>
Cc: Joel Becker <jlbec@evilplan.org>,
	linux-kernel@vger.kernel.org,
	Bodo Stroesser <bostroesser@gmail.com>,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	Brendan Higgins <brendanhiggins@google.com>,
	Yanko Kaneti <yaneti@declera.com>,
	Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH v4 3/3] configfs: Add unit tests
Date: Tue, 10 Aug 2021 14:50:50 -0600	[thread overview]
Message-ID: <e329b0a1-ffe4-9bfa-2bea-33e17da70f58@linuxfoundation.org> (raw)
In-Reply-To: <9b1e5c35-1d11-0afa-d382-6f5dc0b14a23@acm.org>

On 8/10/21 12:45 PM, Bart Van Assche wrote:
> On 8/10/21 9:50 AM, Christoph Hellwig wrote:
>> On Mon, Aug 09, 2021 at 11:31:23AM -0700, Bart Van Assche wrote:
>>>>> +config CONFIGFS_KUNIT_TEST
>>>>> +    bool "Configfs Kunit test" if !KUNIT_ALL_TESTS
>>>>> +    depends on CONFIGFS_FS && KUNIT=y
>>>>> +    default KUNIT_ALL_TESTS
>>>>
>>>> Why does it depend on KUNIT=y?  What is the issue with a modular KUNIT
>>>> build?
>>>
>>> The unit tests calls do_mount(). do_mount() has not been exported and
>>> hence is not available to kernel modules. Hence the exclusion of KUNIT=m.
>>
>> You should probably document that.  But then again this is another
>> big red flag that this code should live in userspace.
>>
>>>> To me this sounds like userspace would be a better place for these
>>>> kinds of tests.
>>>
>>> Splitting the code that can only be run from inside the kernel (creation
>>> of configfs attributes) and the code that can be run from user space and
>>> making sure that the two run in a coordinated fashion would involve a
>>> significant amount of work. I prefer to keep the current approach.
>>
>> But userspace is the right place to do this kind of pathname
>> based file system I/O.
> 
> Shuah, as selftest maintainer, can you recommend an approach? How about splitting patch 3/3 from this series into a kernel module (the code that creates the configfs test attributes) and user space code (the code that reads and writes the configfs attributes) and adding the user space code in a subdirectory of tools/testing/selftests/?
> 

I am missing a lot of context here. I don't see this series in my inbox
except patch 2/3 which says:

"A common feature of unit testing frameworks is support for sharing a test
configuration across multiple unit tests. Add this functionality to the
KUnit framework. This functionality will be used in the next patch in this
series."

That doesn't tell me much other than what happens that it is a common unit
testing framework without explaining why it should be done this way.

Taking a quick look at the original message on lore - I agree with Christoph
that this code belongs in userspace. I would like to see the division of
kernel userspace.

Why do the unit tests need to call do_mount() - can whatever the unit tests
are currently doing can be done from userspace.

If part of the test code must live in kernel space then kernel test module
approach can be used.

thanks,
-- Shuah





  reply	other threads:[~2021-08-10 20:50 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-05  4:35 [PATCH v4 0/3] Restore the kernel v5.13 text attribute write behavior Bart Van Assche
2021-08-05  4:35 ` [PATCH v4 1/3] configfs: " Bart Van Assche
2021-08-05  4:35 ` [PATCH v4 2/3] kunit: Add support for suite initialization and cleanup Bart Van Assche
2021-08-05  4:35 ` [PATCH v4 3/3] configfs: Add unit tests Bart Van Assche
2021-08-09 14:59   ` Christoph Hellwig
2021-08-09 18:31     ` Bart Van Assche
2021-08-10 16:50       ` Christoph Hellwig
2021-08-10 18:45         ` Bart Van Assche
2021-08-10 20:50           ` Shuah Khan [this message]
2021-08-11  9:00             ` Brendan Higgins
2022-03-01 20:03               ` Brendan Higgins
2021-08-10 22:00     ` Brendan Higgins
2021-08-11  3:13       ` Bart Van Assche
2021-08-09 14:56 ` [PATCH v4 0/3] Restore the kernel v5.13 text attribute write behavior Christoph Hellwig

Reply instructions:

You may reply publicly 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=e329b0a1-ffe4-9bfa-2bea-33e17da70f58@linuxfoundation.org \
    --to=skhan@linuxfoundation.org \
    --cc=bostroesser@gmail.com \
    --cc=brendanhiggins@google.com \
    --cc=bvanassche@acm.org \
    --cc=hch@lst.de \
    --cc=jlbec@evilplan.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=yaneti@declera.com \
    --subject='Re: [PATCH v4 3/3] configfs: Add unit tests' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).