| Message ID | 20240306085254.534940-1-dbrendel@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] munlockall: add test case that verifies memory has been unlocked | expand |
Hi Dennis,
Reviewed-by: Petr Vorel <pvorel@suse.cz>
LGTM, thanks!
I would just explicitly note the test was rewritten from scratch
(can be added before merge).
Kind regards,
Petr
Hi Dennis,
Good to see your patch soon, nice work!
Next time remember to commit the patch with Signed-off-by.
`git commit -sm xxxx`
Reviewed-by: Li Wang <liwang@redhat.com>
Hi Dennis, Li, > Hi Dennis, > Good to see your patch soon, nice work! > Next time remember to commit the patch with Signed-off-by. > `git commit -sm xxxx` +1 (planning to add that before merge). Also these comments "Changes compared to v2:" should be below "---", that way we can read them but they will not endup in the git commit message when we apply the patch. Kind regards, Petr
Hi Dennis, > Hi Petr and Li, > Thank you very much for your valuable feedback! > What's left to do for me? Re-submit as v4 with sign-off and > proper commit message? v3 is ready to be merged, I'll add your SBT and cleanup the commit message, while I'll be adding my and Li's RBT. I'm just waiting a little bit because Cyril looked into this patchset, he might have look (I merge tonight). Kind regards, Petr > Thanks, > Dennis > On 3/6/24 11:41, Petr Vorel wrote: > > Hi Dennis, Li, > >> Hi Dennis, > >> Good to see your patch soon, nice work! > >> Next time remember to commit the patch with Signed-off-by. > >> `git commit -sm xxxx` > > +1 (planning to add that before merge). > > Also these comments "Changes compared to v2:" should be below "---", > > that way we can read them but they will not endup in the git commit message when > > we apply the patch. > > Kind regards, > > Petr
Hi! > +/*\ > + * [Description] > + * > + * Verify that munlockall(2) unlocks all previously locked memory > + */ > > -char *TCID = "munlockall01"; > -int TST_TOTAL = 1; > +#include <sys/mman.h> > > -#if !defined(UCLINUX) > +#include "tst_test.h" > > -int main(int ac, char **av) > +static void verify_munlockall(void) > { > - int lc; > - > - tst_parse_opts(ac, av, NULL, NULL); > + unsigned long size = 0; > > - setup(); > + SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size); > > - /* check looping state */ > - for (lc = 0; TEST_LOOPING(lc); lc++) { > + if (size != 0UL) > + tst_brk(TBROK, "Locked memory after init should be 0 but is %ld", size); > > - tst_count = 0; > + if (mlockall(MCL_CURRENT | MCL_FUTURE) != 0) > + tst_brk(TBROK | TERRNO, "Could not lock memory using mlockall()"); > > - TEST(munlockall()); > + SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size); > > - /* check return code */ > - if (TEST_RETURN == -1) { > - tst_resm(TFAIL | TTERRNO, "munlockall() Failed with" > - " return=%ld", TEST_RETURN); > - } else { > - tst_resm(TPASS, "munlockall() passed with" > - " return=%ld ", TEST_RETURN); > + if (size == 0UL) > + tst_brk(TBROK, "Locked memory after mlockall() should be greater than 0, " > + "but is %ld", size); This line can be shorter: tst_brk(TBROK, "After mlockall() locked memory should be >0"); We already checked that size is 0 so no need to print it. > - } > - } > + if (munlockall() != 0) > + tst_brk(TBROK | TERRNO, "Could not unlock memory using munlockall()"); We are testing the munlockall() syscall here so it would be better to use the TST_EXP_PASS() macro. > - /* cleanup and exit */ > - cleanup(); > - tst_exit(); > + SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size); > > + if (size != 0UL) > + tst_res(TFAIL, "Locked memory after munlockall() should be 0 but is %ld", size); > + > + else > + tst_res(TPASS, "Memory successfully locked and unlocked"); > } > > -#else > - > -int main(void) > -{ > - tst_resm(TINFO, "test is not available on uClinux"); > - tst_exit(); > -} > - > -#endif /* if !defined(UCLINUX) */ > - > -/* setup() - performs all ONE TIME setup for this test. */ > -void setup(void) > -{ > - tst_require_root(); > - > - tst_sig(NOFORK, DEF_HANDLER, cleanup); > - > - TEST_PAUSE; > -} > - > -/* > - * cleanup() - performs all ONE TIME cleanup for this test at > - * completion or premature exit. > - */ > -void cleanup(void) > -{ > -} > +static struct tst_test test = { > + .test_all = verify_munlockall, > +}; > -- > 2.44.0 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi Petr and Dennis, As this is the first patch Dennis(thanks!!) writes down for LTP. I would suggest sending a patch V4 (including SBT, Cyril's new reviews) to make you familiar with the workflow of the community:). On Wed, Mar 6, 2024 at 8:44 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Dennis, > > > Hi Petr and Li, > > > Thank you very much for your valuable feedback! > > > What's left to do for me? Re-submit as v4 with sign-off and > > proper commit message? > > v3 is ready to be merged, I'll add your SBT and cleanup the commit message, > while I'll be adding my and Li's RBT. > > I'm just waiting a little bit because Cyril looked into this patchset, > he might have look (I merge tonight). > > Kind regards, > Petr > > > Thanks, > > Dennis > > > On 3/6/24 11:41, Petr Vorel wrote: > > > Hi Dennis, Li, > > > >> Hi Dennis, > > > >> Good to see your patch soon, nice work! > > > >> Next time remember to commit the patch with Signed-off-by. > > >> `git commit -sm xxxx` > > > > +1 (planning to add that before merge). > > > > Also these comments "Changes compared to v2:" should be below "---", > > > that way we can read them but they will not endup in the git commit message when > > > we apply the patch. > > > > Kind regards, > > > Petr > > > > > >
Hi Cyril, On 3/6/24 16:26, Cyril Hrubis wrote: > Hi! >> +/*\ >> + * [Description] >> + * >> + * Verify that munlockall(2) unlocks all previously locked memory >> + */ >> >> -char *TCID = "munlockall01"; >> -int TST_TOTAL = 1; >> +#include <sys/mman.h> >> >> -#if !defined(UCLINUX) >> +#include "tst_test.h" >> >> -int main(int ac, char **av) >> +static void verify_munlockall(void) >> { >> - int lc; >> - >> - tst_parse_opts(ac, av, NULL, NULL); >> + unsigned long size = 0; >> >> - setup(); >> + SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size); >> >> - /* check looping state */ >> - for (lc = 0; TEST_LOOPING(lc); lc++) { >> + if (size != 0UL) >> + tst_brk(TBROK, "Locked memory after init should be 0 but is %ld", size); >> >> - tst_count = 0; >> + if (mlockall(MCL_CURRENT | MCL_FUTURE) != 0) >> + tst_brk(TBROK | TERRNO, "Could not lock memory using mlockall()"); >> >> - TEST(munlockall()); >> + SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size); >> >> - /* check return code */ >> - if (TEST_RETURN == -1) { >> - tst_resm(TFAIL | TTERRNO, "munlockall() Failed with" >> - " return=%ld", TEST_RETURN); >> - } else { >> - tst_resm(TPASS, "munlockall() passed with" >> - " return=%ld ", TEST_RETURN); >> + if (size == 0UL) >> + tst_brk(TBROK, "Locked memory after mlockall() should be greater than 0, " >> + "but is %ld", size); > > This line can be shorter: > > tst_brk(TBROK, "After mlockall() locked memory should be >0"); > > We already checked that size is 0 so no need to print it. Agreed :-) >> - } >> - } >> + if (munlockall() != 0) >> + tst_brk(TBROK | TERRNO, "Could not unlock memory using munlockall()"); > > We are testing the munlockall() syscall here so it would be better to > use the TST_EXP_PASS() macro. The actual purpose of the test was not checking the return value of the syscall (wrapper), but the behavior as reported by the kernel through /proc - but fair enough, it does not do any harm :-) >> - /* cleanup and exit */ >> - cleanup(); >> - tst_exit(); >> + SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size); >> >> + if (size != 0UL) >> + tst_res(TFAIL, "Locked memory after munlockall() should be 0 but is %ld", size); >> + >> + else >> + tst_res(TPASS, "Memory successfully locked and unlocked"); >> } >> >> -#else >> - >> -int main(void) >> -{ >> - tst_resm(TINFO, "test is not available on uClinux"); >> - tst_exit(); >> -} >> - >> -#endif /* if !defined(UCLINUX) */ >> - >> -/* setup() - performs all ONE TIME setup for this test. */ >> -void setup(void) >> -{ >> - tst_require_root(); >> - >> - tst_sig(NOFORK, DEF_HANDLER, cleanup); >> - >> - TEST_PAUSE; >> -} >> - >> -/* >> - * cleanup() - performs all ONE TIME cleanup for this test at >> - * completion or premature exit. >> - */ >> -void cleanup(void) >> -{ >> -} >> +static struct tst_test test = { >> + .test_all = verify_munlockall, >> +}; >> -- >> 2.44.0 >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp >
diff --git a/testcases/kernel/syscalls/munlockall/munlockall01.c b/testcases/kernel/syscalls/munlockall/munlockall01.c index 51f731b65..2745a1aee 100644 --- a/testcases/kernel/syscalls/munlockall/munlockall01.c +++ b/testcases/kernel/syscalls/munlockall/munlockall01.c @@ -1,134 +1,48 @@ +// SPDX-License-Identifier: GPL-2.0-or-later /* - * Copyright (c) Wipro Technologies Ltd, 2002. All Rights Reserved. - * - * This program is free software; you can redistribute it and/or modify it - * under the terms of version 2 of the GNU General Public License as - * published by the Free Software Foundation. - * - * This program is distributed in the hope that it would be useful, but - * WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. - * - * You should have received a copy of the GNU General Public License along - * with this program; if not, write the Free Software Foundation, Inc., - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. - * + * Copyright Red Hat + * Author: Dennis Brendel <dbrendel@redhat.com> */ -/************************************************************************** - * - * TEST IDENTIFIER : munlockall01 - * - * EXECUTED BY : root / superuser - * - * TEST TITLE : Basic test for munlockall(2) - * - * TEST CASE TOTAL : 1 - * - * AUTHOR : sowmya adiga<sowmya.adiga@wipro.com> - * - * SIGNALS - * Uses SIGUSR1 to pause before test if option set. - * (See the parse_opts(3) man page). - * - * DESCRIPTION - * This is a phase I test for the munlockall(2) system call. - * It is intended to provide a limited exposure of the system call. - * - * Setup: - * Setup signal handling. - * Pause for SIGUSR1 if option specified. - * - * Test: - * Execute system call - * Check return code, if system call failed (return=-1) - * Log the errno and Issue a FAIL message. - * Otherwise, Issue a PASS message. - * - * Cleanup: - * Print errno log and/or timing stats if options given - * - * USAGE: <for command-line> - * munlockall01 [-c n] [-e] [-i n] [-I x] [-p x] [-t] - * where, -c n : Run n copies concurrently - * -e : Turn on errno logging. - * -h : Show this help screen - * -i n : Execute test n times. - * -I x : Execute test for x seconds. - * -p : Pause for SIGUSR1 before starting - * -P x : Pause for x seconds between iterations. - * t : Turn on syscall timing. - * - * RESTRICTIONS - * Must be root/superuser to run it. - *****************************************************************************/ -#include <errno.h> -#include <sys/mman.h> -#include "test.h" -void setup(); -void cleanup(); +/*\ + * [Description] + * + * Verify that munlockall(2) unlocks all previously locked memory + */ -char *TCID = "munlockall01"; -int TST_TOTAL = 1; +#include <sys/mman.h> -#if !defined(UCLINUX) +#include "tst_test.h" -int main(int ac, char **av) +static void verify_munlockall(void) { - int lc; - - tst_parse_opts(ac, av, NULL, NULL); + unsigned long size = 0; - setup(); + SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size); - /* check looping state */ - for (lc = 0; TEST_LOOPING(lc); lc++) { + if (size != 0UL) + tst_brk(TBROK, "Locked memory after init should be 0 but is %ld", size); - tst_count = 0; + if (mlockall(MCL_CURRENT | MCL_FUTURE) != 0) + tst_brk(TBROK | TERRNO, "Could not lock memory using mlockall()"); - TEST(munlockall()); + SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size); - /* check return code */ - if (TEST_RETURN == -1) { - tst_resm(TFAIL | TTERRNO, "munlockall() Failed with" - " return=%ld", TEST_RETURN); - } else { - tst_resm(TPASS, "munlockall() passed with" - " return=%ld ", TEST_RETURN); + if (size == 0UL) + tst_brk(TBROK, "Locked memory after mlockall() should be greater than 0, " + "but is %ld", size); - } - } + if (munlockall() != 0) + tst_brk(TBROK | TERRNO, "Could not unlock memory using munlockall()"); - /* cleanup and exit */ - cleanup(); - tst_exit(); + SAFE_FILE_LINES_SCANF("/proc/self/status", "VmLck: %ld", &size); + if (size != 0UL) + tst_res(TFAIL, "Locked memory after munlockall() should be 0 but is %ld", size); + else + tst_res(TPASS, "Memory successfully locked and unlocked"); } -#else - -int main(void) -{ - tst_resm(TINFO, "test is not available on uClinux"); - tst_exit(); -} - -#endif /* if !defined(UCLINUX) */ - -/* setup() - performs all ONE TIME setup for this test. */ -void setup(void) -{ - tst_require_root(); - - tst_sig(NOFORK, DEF_HANDLER, cleanup); - - TEST_PAUSE; -} - -/* - * cleanup() - performs all ONE TIME cleanup for this test at - * completion or premature exit. - */ -void cleanup(void) -{ -} +static struct tst_test test = { + .test_all = verify_munlockall, +};