Re: [LTP] [PATCH] mmap/munmap /dev/zero

[CAI Q. <ca...@re...>]

----- "Mike Frysinger" <va...@ge...> wrote:

> On Tuesday, October 12, 2010 10:07:16 CAI Qian wrote:
> > +/* main setup function of test */
> > +void setup();
> > +/* cleanup function for the test */
> > +void cleanup();
> > +int mmapzero();
> 
> functins that take no args should be "(void)" and not "()"
Fixed in v2.
> 
> > +int main(int ac, char **av)
> 
> use consistent style.  this should be "int argc" and "char *argv[]".
Fixed.
> 
> > +	if (msg != (char *)NULL) {
> 
> useless cast
Fixed.
> 
> > +		if(TEST_RETURN != 0)
> 
> need a space before the "("
Fixed
> 
> > +	 x = mmap("/dev/zero", SIZE+SIZE-4096, PROT_READ|PROT_WRITE,
> > +		 MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> 
> uhh, have you even checked this test ?  this mmap() makes absolutely
> no sense 
> on so many levels.
Can you elaborate?
> 
> > +	 if (x == MAP_FAILED) {
> > +		 perror("error");
> 
> make this perror() slightly less useless by replacing "error" with
> "mmap".  
> although, i wonder why you cant use any of the ltp helper functions
> for error 
> reporting ... tests rarely (if ever) should be writing to
> stdout/stderr 
> themselves.
Fixed.
> 
> > +	 if (!fork()) {
> > +		 munmap(x + SIZE+4096, SIZE-4096*2);
> > +		 _exit(0);
> > +	 }
> > +	 if (!fork()) {
> > +		 if (!fork()) {
> > +			 munmap(x + SIZE+4096, SIZE-4096*2);
> > +			 _exit(0);
> > +		 }
> > +		 munmap(x + SIZE+4096, SIZE-4096*2);
> > +		 _exit(0);
> > +	 }
> > +	 munmap(x, SIZE+SIZE-4096);
> 
> fork() returns -1 on error which means this code misbehaves.
Fixed.
> 
> why are you using _exit() ?
It is used to terminate a child.
> 
> munmap() returns a value you shouldbe checking.  blindly calling funcs
> doesnt 
> accomplish much.
Fixed.
> 
> > + void cleanup()
> > + {
> > +	 /*
> > +	  * remove the tmp directory and exit
> > +	  */
> > +	 TEST_CLEANUP;
> > +	 tst_rmdir();
> > +	 tst_exit();
> > +
> > + }
> 
> useless newline before the closing brace
Fixed.
> -mike