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

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

[CAI Qian]

Add a new test to mmap/munmap /dev/zero.

Signed-off-by: CAI Qian <caiqian@redhat.com>

--- /dev/null
+++ b/testcases/kernel/syscalls/mmap/mmap10.c
@@ -0,0 +1,131 @@
+/*
+ * mmap/munmap /dev/zero
+ *
+ * Copyright (C) 2010  Red Hat, Inc.
+ * 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.
+ *
+ * Further, this software is distributed without any warranty that it
+ * is free of the rightful claim of any third person regarding
+ * infringement or the like.  Any license provided herein, whether
+ * implied or otherwise, applies only to this software file.  Patent
+ * licenses, if any, provided herein do not apply to combinations of
+ * this program with other software, or any other product whatsoever.
+ *
+ * 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.
+ */
+
+#include "test.h"
+#include "usctest.h"
+#include <errno.h>
+#include <sys/mman.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <sys/wait.h>
+#include <stdio.h>
+
+#define SIZE (5*1024*1024)
+
+char *TCID = "mmap10";
+int TST_TOTAL = 1;
+extern int Tst_count;
+
+/* main setup function of test */
+void setup();
+/* cleanup function for the test */
+void cleanup();
+int mmapzero();
+
+
+int main(int ac, char **av)
+{
+	/* loop counter */
+	int lc;
+	/* message returned from parse_opts */
+	char *msg;
+
+	msg = parse_opts(ac, av, (option_t *) NULL, NULL);
+	if (msg != (char *)NULL) {
+		tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg);
+		tst_exit();
+	}
+
+	/* Perform global setup for test */
+	setup();
+
+	/* Check looping state if -i option given */
+	for (lc = 0; TEST_LOOPING(lc); lc++) {
+		/* Reset Tst_count in case we are looping. */
+		Tst_count = 0;
+
+		TEST(mmapzero());
+
+		if(TEST_RETURN != 0)
+			tst_resm(TFAIL, "mmapzero() failed with %ld.",
+				TEST_RETURN);
+		else
+			tst_resm(TPASS, "mmapzero() completed successfully.");
+	 }
+
+	 cleanup();
+	 return 0;
+ }
+
+ int mmapzero()
+ {
+	 char *x;
+
+	 x = mmap("/dev/zero", SIZE+SIZE-4096, PROT_READ|PROT_WRITE,
+		 MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+	 if (x == MAP_FAILED) {
+		 perror("error");
+		 return 1;
+	 }
+	 x[SIZE] = 0;
+	 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);
+	 while (waitpid(-1, NULL, WNOHANG) > 0);
+	 return 0;
+ }
+
+ void cleanup()
+ {
+	 /*
+	  * remove the tmp directory and exit
+	  */
+	 TEST_CLEANUP;
+	 tst_rmdir();
+	 tst_exit();
+
+ }
+
+
+void setup()
+{
+	/*
+	 * setup a default signal hander and a
+	 * temporary working directory.
+	 */
+	tst_sig(FORK, DEF_HANDLER, cleanup);
+	TEST_PAUSE;
+	tst_tmpdir();
+}

------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 1695 bytes --]

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 "()"

> +int main(int ac, char **av)

use consistent style.  this should be "int argc" and "char *argv[]".

> +	if (msg != (char *)NULL) {

useless cast

> +		if(TEST_RETURN != 0)

need a space before the "("

> +	 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.

> +	 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.

> +	 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.

why are you using _exit() ?

munmap() returns a value you shouldbe checking.  blindly calling funcs doesnt 
accomplish much.

> + void cleanup()
> + {
> +	 /*
> +	  * remove the tmp directory and exit
> +	  */
> +	 TEST_CLEANUP;
> +	 tst_rmdir();
> +	 tst_exit();
> +
> + }

useless newline before the closing brace
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 369 bytes --]

------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[Garrett Cooper]

On Tue, Oct 12, 2010 at 12:15 PM, Mike Frysinger <vapier@gentoo.org> 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();

...

    +1 to what Mike said, but in all actually what is the purpose of
this test? There isn't a description either in the commit message or
the code itself that mentions what this code should functionally do.
We need both in order for the code to be committed (otherwise folks
will play the "WTF does this code do?" game when semantics change a
few years later in the GNU/Linux world).
Thanks,
-Garrett

------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[CAI Qian]

----- "Garrett Cooper" <yanegomi@gmail.com> wrote:

> On Tue, Oct 12, 2010 at 12:15 PM, Mike Frysinger <vapier@gentoo.org>
> 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();
> 
> ...
> 
>     +1 to what Mike said, but in all actually what is the purpose of
> this test? There isn't a description either in the commit message or
> the code itself that mentions what this code should functionally do.
> We need both in order for the code to be committed (otherwise folks
> will play the "WTF does this code do?" game when semantics change a
> few years later in the GNU/Linux world).
> Thanks,
Fixed in v2.
> -Garrett

------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[CAI Qian]

----- "Mike Frysinger" <vapier@gentoo.org> 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

------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 2551 bytes --]

On Tuesday, October 12, 2010 22:06:29 CAI Qian wrote:
> ----- "Mike Frysinger" wrote:
> > On Tuesday, October 12, 2010 10:07:16 CAI Qian wrote:
> > > +	 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?

so you havent actually looked at the syscalls made by the application and the 
memory maps created nor used mmap() in detail before ?  well, let's start with 
the mmap() man page:

	void *mmap(void *addr, size_t length, int prot, int flags,
		int fd, off_t offset);

"addr" doesnt sound like "file name" to me, and the description agrees:

	If addr is not NULL, then the kernel takes it as a hint about where to
	place the mapping; on Linux, the mapping will be created at a nearby page
	boundary.  The address of the new mapping is returned as the result of
	the call.

so basically you told the kernel to create a mapping in/near your .rodata 
section (since that is the address of the constant string "/dev/zero").  the 
contents of that pointer (which is not of type "char *") have absolutely no 
meaning to the kernel.

then there are the flags you're using:

	MAP_ANONYMOUS
		The mapping is not backed by any file; its contents are initialized
		to zero.  The fd and offset arguments are ignored; however, some
		implementations require fd to be -1 if MAP_ANONYMOUS (or MAP_ANON)
		is specified, and portable applications should ensure this.

so not only are you not getting a mapping from /dev/zero, you're just 
allocating a random piece of memory that the kernel has guaranteed will be 
zeroed out for you.  so any attempts to verify the contents are zero *because 
the data came from /dev/zero* fail.

finally, there's the fd which you're passing as "-1":

	The contents of a file mapping (as opposed to an anonymous mapping; see
	MAP_ANONYMOUS below), are initialized using length bytes starting at
	offset "offset" in the file (or other object) referred to by the file
	descriptor "fd".

"-1" is not a valid fd, so there's no way it could possibly be from /dev/zero.

so we're back where we started: this test makes no sense as written and you 
apparently havent verified it "works" beyond "is the exit status 0 when it 
finished".

> > why are you using _exit() ?
> 
> It is used to terminate a child.

i meant why arent you using exit() ?  why do you need _exit() semantics ?
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 369 bytes --]

------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[CAI Qian]

----- "Mike Frysinger" <vapier@gentoo.org> wrote:

> On Tuesday, October 12, 2010 22:06:29 CAI Qian wrote:
> > ----- "Mike Frysinger" wrote:
> > > On Tuesday, October 12, 2010 10:07:16 CAI Qian wrote:
> > > > +	 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?
> 
> so you havent actually looked at the syscalls made by the application
> and the 
> memory maps created nor used mmap() in detail before ?  well, let's
> start with 
> the mmap() man page:
> 
> 	void *mmap(void *addr, size_t length, int prot, int flags,
> 		int fd, off_t offset);
> 
> "addr" doesnt sound like "file name" to me, and the description
> agrees:
> 
> 	If addr is not NULL, then the kernel takes it as a hint about where
> to
> 	place the mapping; on Linux, the mapping will be created at a nearby
> page
> 	boundary.  The address of the new mapping is returned as the result
> of
> 	the call.
> 
> so basically you told the kernel to create a mapping in/near your
> .rodata 
> section (since that is the address of the constant string
> "/dev/zero").  the 
> contents of that pointer (which is not of type "char *") have
> absolutely no 
> meaning to the kernel.
> 
> then there are the flags you're using:
> 
> 	MAP_ANONYMOUS
> 		The mapping is not backed by any file; its contents are initialized
> 		to zero.  The fd and offset arguments are ignored; however, some
> 		implementations require fd to be -1 if MAP_ANONYMOUS (or MAP_ANON)
> 		is specified, and portable applications should ensure this.
> 
> so not only are you not getting a mapping from /dev/zero, you're just
> 
> allocating a random piece of memory that the kernel has guaranteed
> will be 
> zeroed out for you.  so any attempts to verify the contents are zero
> *because 
> the data came from /dev/zero* fail.
> 
> finally, there's the fd which you're passing as "-1":
> 
> 	The contents of a file mapping (as opposed to an anonymous mapping;
> see
> 	MAP_ANONYMOUS below), are initialized using length bytes starting at
> 	offset "offset" in the file (or other object) referred to by the
> file
> 	descriptor "fd".
> 
> "-1" is not a valid fd, so there's no way it could possibly be from
> /dev/zero.
> 
> so we're back where we started: this test makes no sense as written
> and you 
> apparently havent verified it "works" beyond "is the exit status 0
> when it 
> finished".
I see. MAP_ANONYMOUS is not needed. It certainly want to use int fd. v3 followed.
> 
> > > why are you using _exit() ?
> > 
> > It is used to terminate a child.
> 
> i meant why arent you using exit() ?  why do you need _exit()
> semantics ?
I don't think that too much. I think it can use exit() too.
> -mike

------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list