Re: [LTP] [PATCH] fix lchown tests

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> > Folowing patch fixes/cleanups lchown tests.
> >
> > lchown01 changes:
> >
> > * make a better use of ltp test interface
> > * make error messages consistent
> > * various coding style issues
> >
> > lchown02 changes:
> >
> > * all test setups are now done in setup functions
> > * eliminated need for external script for preparing tempfile
> > * make a better use of ltp test interface
> > * make error messages consistent
> > * various coding style issues
> > * fix lchown02 invocation in runtest files
> 
> ...
> 
> +CFLAGS+=-W -Wall
> +
> 
> gcooper> Please remove.

Okay.

> ...
> 
> +				if (user_id == (uid_t)-1)
> +					user_id = test_cases[i - 1].user_id;
> +				if (group_id == (gid_t)-1)
> +					group_id = test_cases[i - 1].group_id;
> 
> gcooper> These assume that i is >= 1. Could you please add that
> conditional to the overall block so someone doesn't delete the
> subtestcase, break the test and come back to us later and gripe about
> how it's accessing memory out of bounds?
 
Okay.

> +				if ((stat_buf.st_uid != user_id) ||
> +				    (stat_buf.st_gid != group_id)) {
>  					tst_resm(TFAIL,
>  						 "%s: Incorrect ownership set, "
>  						 "Expected %d %d", SFILE,
> 
> gcooper> Technically it's %u.

Okay.

> +	return 0;
> +}
> 
> gcooper> If it gets down here, it should be return 1;
> 

Well, the return 0; is there just to silent compiler as cleanup() calls
tst_exit(); The best solution is IMHO add noreturn attribute to
cleanup() and remove the return 0;

> ...
> 
> +	if (close(fd) == -1)
> +		tst_brkm(TBROK | TERRNO, cleanup, "close(2) %s", TESTFILE);
> 
> gcooper> Wouldn't worry about the close.
 
Checking it doesn't do any harm. ;)

> ...
> 
> -	/* fix permissions on the tmpdir */
> -	if (chmod(".", 0711) != 0) {
> -		tst_brkm(TBROK | TERRNO, cleanup, "chmod() failed");
> 
> gcooper> The directory mode for tst_tmpdir is different:
> 
> #define DIR_MODE        0777  /* mode of tmp dir that will be created */

As far as I understand the tests, changing permissions on tmp directory
is not needed as the test is testing permissions on symlinks created
inside of it (so we only need that the content of the directory is
accesible). I susspect that this is some workaround that wasn't removed
when test was ported for LTP.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list