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
