Hi Cyril, Thanks for your review. I will update codes according to your suggestion. Yuan
On 2015/8/18 20:07, Cyril Hrubis wrote: > Hi! >> +/******************************************************************************/ >> +/* >> */ >> +/* Copyright (c) International Business Machines Corp., 2008 >> */ >> +/* >> */ >> +/* This program is free software; you can redistribute it and/or modify >> */ >> +/* it under the terms of the GNU General Public License as published by >> */ >> +/* the Free Software Foundation; either version 2 of the License, or >> */ >> +/* (at your option) any later version. >> */ >> +/* >> */ >> +/* This program is distributed in the hope that it will be useful, >> */ >> +/* but WITHOUT ANY WARRANTY; without even the implied warranty of >> */ >> +/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See >> */ >> +/* the GNU General Public License for more details. >> */ >> +/* >> */ >> +/* You should have received a copy of the GNU General Public License >> */ >> +/* along with this program. >> */ >> +/* >> */ >> +/******************************************************************************/ >> + >> +#include <stdio.h> >> +#include "config.h" >> +#include "test.h" >> +#if HAVE_SYS_CAPABILITY_H >> +#include <linux/types.h> >> +#include <sys/capability.h> >> +#endif >> + >> +char *TCID = "filecaps01"; > ^ > Since TCID is set to filecaps01 it would make more > sense to rename the file to filecaps01.c so that the > filename and TCID match. > >> +int TST_TOTAL = 1; >> + >> +static void cleanup(void) >> +{ >> + tst_rmdir(); >> +} >> + >> +static void setup(void) >> +{ >> + tst_tmpdir(); >> +} > Looking at this test in particular it doesn't need temporary directory, > so we can omit this. > >> +int main(int argc, char *argv[]) >> +{ >> +#ifdef HAVE_LIBCAP >> + int lc; >> + cap_t caps, caps2; >> + int ret; >> +#endif >> + tst_parse_opts(argc, argv, NULL, NULL); >> + setup(); >> + >> +#ifdef HAVE_LIBCAP >> + for (lc = 0; TEST_LOOPING(lc); lc++) { >> + caps = cap_from_text("cap_setpcap+ep"); >> + caps2 = cap_from_text("cap_setpcap+ep"); >> + ret = cap_set_proc(caps); >> + if (ret != 0) >> + tst_resm(TFAIL, "cap_set_proc failure"); > ^ > This should be tst_brkm(TBROK, ...); so that > we exit the test if the preparation failed. > >> + ret = cap_compare(caps, caps2); > This seems really strange, we compile two identical capabilities from > string and then compare them for equality. Shouldn't the code rather: > > * prepare capability from string > * set the capability > * get the capability back > * compare the compiled value with the one we get back > > Otherwise this test does not make much sense to me. > >> + if (ret != 0) >> + tst_resm(TFAIL, "Caps were not the same."); >> + >> + cap_free(caps); >> + cap_free(caps2); >> + tst_resm(TPASS, "Caps were the same."); > ^ > You must not print TPASS if the test > >> + } >> +#else >> + tst_resm(TFAIL, "System doesn't support full POSIX capabilities."); > ^ > This should be tst_brkm(TCONF, ...); > > Usually testcases tend to have separate main() > function for the case that some functionality was > missing upon compilation rather than spreading > #ifdefs around the code. I.e. > > #ifdef HAVE_FOO > int main(int argc, char *argv[]) > { > /* do the test here */ > ... > } > #else > int main(void) > { > tst_brkm(TCONF, "..."); > } > #endif /* HAVE_FOO */ >> +#endif >> + cleanup(); >> + tst_exit(); >> +} ------------------------------------------------------------------------------ _______________________________________________ Ltp-list mailing list Ltp-list@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ltp-list