From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 86E57D30006 for ; Fri, 18 Oct 2024 13:04:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0FAF16B007B; Fri, 18 Oct 2024 09:04:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 084746B0083; Fri, 18 Oct 2024 09:04:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E67696B0089; Fri, 18 Oct 2024 09:04:13 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id C0BFB6B007B for ; Fri, 18 Oct 2024 09:04:13 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id CE2471C6350 for ; Fri, 18 Oct 2024 13:03:59 +0000 (UTC) X-FDA: 82686740808.26.B1609DE Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf13.hostedemail.com (Postfix) with ESMTP id 1ADD320008 for ; Fri, 18 Oct 2024 13:03:59 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=sy70iDxd; spf=pass (imf13.hostedemail.com: domain of broonie@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=broonie@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729256506; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=hxd0hnRzmfuk79PxVe1bJj11dWecVzX5FmhiWKbt71I=; b=XTWpZih+h2awoiT2044oiv09gkDFirq568syjHmkMIz1i5hLSx/TV8fASRLGcAC0RwoVAl JXUkpFq2vNR0S94TrHL6w/P5RaQQyIeUfNs0N5yLoMONRtMA7weFmL3W91qzj4tQWIuWlm /k/34CxhjLVFX+zkq05yjyodlfjPtY0= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729256506; a=rsa-sha256; cv=none; b=24Wz/uCt8bbcdZY6EnobM9kjxfeqE+n/Hgz1zV9lWoIaVBOnZNrNbQUaQXQK2vkzy10rUj AbIeRzbCwLhbT6VXBMJCgJquaDWi1RKwoC3rMvtZfNbk1sktsEVNOPlM+Wsr37Il2s0GpH j9zAtd+Rfp17NhsRJI1N8elZmCeuW6s= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=sy70iDxd; spf=pass (imf13.hostedemail.com: domain of broonie@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=broonie@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 382CD5C59D9; Fri, 18 Oct 2024 13:04:06 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8B843C4CEC3; Fri, 18 Oct 2024 13:04:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1729256650; bh=g2CJxGpTFCOsw8YxyBZm5iz4MWqWWqjHA92Ubpkewfk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=sy70iDxdvegIxSpbXgc5l5sDisr1CZ4sC7k1lbpyMO3ps7QBlrYogssUkxA3itTq3 AQEoQL9PclRAphMwOGvXczd3YP5474UnFMyGtofrV7wewmUhWgb36+D/mxX0TSNS9T f/lVPozmAReEZcHeTg1Kr5WVXMFXZYL56yvSNJocnM2MQT14uIf4KFaoaWuUmH2N9u j6TTPE83B54c0FIDFMWZnWB6v9ey8dIJiiRCeaLi84cRr3FGSDOSn6r3l8GdREAatz wjnSiM+Q3gG6VspsFgyIlOd22BZENkjey1+D1sWRaKBTI22bIf8/K8mo1mVq3sihH0 aeBFQqweIx1hw== Date: Fri, 18 Oct 2024 14:04:04 +0100 From: Mark Brown To: Jeff Xu Cc: Lorenzo Stoakes , Muhammad Usama Anjum , akpm@linux-foundation.org, linux-kselftest@vger.kernel.org, linux-mm@kvack.org, linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org, pedro.falcato@gmail.com, willy@infradead.org, vbabka@suse.cz, Liam.Howlett@oracle.com, rientjes@google.com, keescook@chromium.org Subject: Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap Message-ID: <1f8eff74-005b-4fa9-9446-47f4cdbf3e8d@sirena.org.uk> References: <4944ce41-9fe1-4e22-8967-f6bd7eafae3f@lucifer.local> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="YLYxAeMluy/4woDv" Content-Disposition: inline In-Reply-To: X-Cookie: What is the sound of one hand clapping? X-Rspamd-Queue-Id: 1ADD320008 X-Stat-Signature: xpe1iyxgjxqeir4prenog3xwbhnjp8wd X-Rspamd-Server: rspam09 X-Rspam-User: X-HE-Tag: 1729256639-642977 X-HE-Meta: U2FsdGVkX19PlmHvviweCEFPBwHDxGGX8HM5YBDZfGnI4bcAoXZ3/Gu4iAqu7ZYHE6qno5VeUkvNWjLqX3j8mIld06lJxKtP1BTYo3eKtcERA5X6Pe0HGXCPxiVJUrvL2EPFkUoQBRjOP39z44YfbdexJVCCouJhYGl/rt2bfbvSXyzxpGG4RgbAUuuDF11KPBETaG+tSIaSkKJPs/IdBd1xa6PqFLAQxu52VdcryQKpkX6tDPT7BIm6mnLBt+rBei6nSSpKcbCvU7LKo+nEoYu3i/FqHHJUdUWEyiuFzq3us005N5P654SHZO8pVFsH7Eq8S/4VH+pEZcVJUVZyKCTZuf8ZYilwkXjUyh8vKsk8vcoWP4K2PptGLJiq87Gbn5sLvG2+gIpMIyjnZgP0COLkss6gC3EXiygTX2PJfXZzHUtlMsjn7g4hHrjwWyNRnX8LnU6DiMFp1+OF8bj/j5eniTsGL/vllnMM1NQZJQBc2O4teNqjeqIPyaKMibA+KUXz6+QeRHVoQV5bI1/av/dE0IMICPAQ1Y/LA370OO8p+9fKx7IgBA7zaoevY8oQAQvd9WVX0aLyioh43kOPgmrfUlDHTXG2dbFWSpw1LShbDxJTcJ9oiz9ZT2U4kx4RLUBzrfcuwJVdzXiN7gXHhnSkZ0oH+yEAg511iP8bZI5c25lcV7r8ZdG7e5qCInHA99cMVKgy9Ao1v72cmihsoD0iYmMZipt705xbjf5krKUn3lJRVnZzE9/8peS+rj3S0HSKZJxd3SCsLD6zy+88fuxknJmOWY20sk8jklDu03CA4mGy+Lxa6WCiqQ+LE+c7LfZ0D2zyeQ7LiciDQgDFoA9qohksBqMRbSUOJPYHJIHwXtO25p2sHDN+HwgI+SsclfXcZc5Jkr0V22BH1rFMILtn8qEZdj7fSJIRf1wHU2M9JttZw87kQyYI34Pf5UYHg3nz7UoNBpBRKTRC+7W 4ILJvRPx 0czy6nBDibWi0IDe40CMQMwWaNeoI+hxBMw01O6LJyxrBlnklFxTiMw6tWW+y2PVBWOh+gfLcsAZeMs7ewtQHpXIl3IICJrAz0s9tRdgvXli6Lr9O40sGr+1sOMsiXTYrNJNgUWWZkJ80QJHR6LOgcB83t3VEEv2PlKIaroUOARFUbIC/F5FKcUyet/s9HZe/wkN0VbzrnGiJ3Msobq5XyvQ7n5Ra5aswrqwryYFs7aH4xqW64+loZmVZDVWGZ4o79cZcT3wHHfA4G2FF9XVkhwMl+8CFjD6M693wDwB+fvB0GFzdGAZwP7e2kvv6uxlbUWPQtnevN0RVAgCYjSlkjwYcqf95un4ODtNbq6eXD3MjaA2duN/3FkmQyENHkGcpPNlB X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: --YLYxAeMluy/4woDv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote: > So it is not a problem with the MACRO, but where is it used ? > ret = sys_mseal(ptr, size); > FAIL_TEST_IF_FALSE(!ret); > Take this example, it would be > assert(!ret) The problem is that the macro name is confusing and not terribly consistent with how the rest of the selftests work. The standard kselftest result reporting is ksft_test_result(bool result, char *name_format, ...); so the result of the test is a boolean flag which is passed in. This macro on the other hand sounds like a double negative so you have to stop and think what the logic is, and it's not seen anywhere else so nobody is going to be familiar with it. The main thing this is doing is burying a return statement in there, that's a bit surprising too. I'll also note that these macros are resulting in broken kselftest output, the name for a test has to be stable for automated systems to be able to associate test results between runs but these print ksft_test_result_fail("%s: line:%d\n", \ __func__, __LINE__); \ return; \ which includes the line number of the test in the name which is an obvious problem, automated systems won't be able to tell that any two failures are related to each other never mind the passing test. We should report why things failed but it's better to do that with a ksft_print_msg(), ideally one that's directly readable rather than requiring someone to go into the source code and look it up. A more standard way to write what you've got here would be to have the tests return a bool then have a runner loop which iterates over the tests: struct { char *name; bool (*func)(void); } tests[]; ... for (i = 0; i < ARRAY_SIZE(tests); i++) ksft_test_result(tests[i].test(), tests[i].name); then the tests can just have explicit return statements and don't need to worry about logging anything other than diagnostics. Depending on how much you need to share between tests you might also be able to use kselftest_harness.h which fork()s each test into a separate child and allows you to just fault to fail if that's easier. > > We are writing unit tests in a test framework, let's use very well > > established industry practices please. Plus also the fact that we have a framework here... > > Also note that you don't even need to reinvent the wheel, there is a > > fully-featured test harness available in > > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and > > EXPECT_xxx() helpers. > The EXPECT_xxx() doesn't take care of reporting though, or maybe it I rather think people would've noticed if the test harness was so broken that it was unable to report failures. If it is that broken we should fix it rather than open coding something else. --YLYxAeMluy/4woDv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmcSXMMACgkQJNaLcl1U h9AmKAf/Tu4kW1hhmlXQ1Agr7BitWMDgVw9arn8ymLeyxIk/tu+VEoRY7AhaMvzd 3J00TJc8rCiabRtgebGDooegImXGlwOPo+OozAKDSoqY+XL5NQxnakmMrk0vSUfQ jJzLnl2Bxo/43T6/xPQKzUdxMz8NY4uHDF3X0FPWWnvF4F/5XMZ44uDRMmFrL+aI FsP7ZEB4W1A7wn5bpj83e58whRSO5G2Y/3Z5kl5oh49cTIXBM4nNIVtOcOWt5976 FWtpGPSQEHqtBj840j7dVwFCRAJ9aobcbBWt4lOhOVftuOf1fE0QeLgGaE9/tOHB gYLIyF/x8ZauAuQattbuUL3nOp6iqw== =r5Sq -----END PGP SIGNATURE----- --YLYxAeMluy/4woDv--