Skip to contentSkip to conversation

syscalls/mallopt: Does not work correctly?

PATCH New TAKAHASHI Tetsuya 2 participants 4 messages Oct 1, 2015 – Oct 22, 2015

lore
Share & tools
lore

TAKAHASHI Tetsuya

author
#0 lore raw

PATCH 1/1 syscalls/mallopt: Does not work correctly?

Hello.

About testcase "syscalls/mallopt", I had 2 questions.
(Are there bugs?)
1. System call mallopt() returns 1 on Success, and 0 on Failure.
But, testcase does not check it's.
2. In case of using "M_MXFAST" param, allowed values are in the range 0 to 80*sizeof(size_t)/4. (since glibc 2.3)
http://linux.die.net/man/3/mallopt
But, testcase sets fixed value "2048".
"mallopt(M_MXFAST, 2048)" always return FAIL.

Therefore, I suggest to adding following functions.
1. Checking mallopt() return values.
2. Setting tests value is 0 and 80*sizeof(size_t)/4.

testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 renamed from testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-08 18:37:03.000000000 +0900 +32 -5
@@ -51,6 +51,7 @@
 
 #define FAILED 0
 #define PASSED 1
+#define MAX_FAST_SIZE (80 * sizeof(size_t) / 4)
 
 int local_flag = PASSED;
 

@@ -71,7 +72,8 @@ int main(int argc, char *argv[])
 
  tst_tmpdir();
 
- buf = SAFE_MALLOC(NULL, 20480);
+ if ((buf = SAFE_MALLOC(NULL, 20480)) == NULL)
+  tst_brkm(TBROK, NULL, "malloc failed");
 
  /*
   * Check space usage.

@@ -86,16 +88,41 @@ int main(int argc, char *argv[])
   printinfo();
   tst_resm(TFAIL, "mallinfo failed: smblks != 0");
  }
+ if (info.uordblks >= 20480 && info.smblks == 0)
+  tst_resm(TPASS, "mallinfo() succeeded");
  free(buf);
 
  /*
   * Test mallopt's M_MXFAST and M_NLBLKS cmds.
   */
- mallopt(M_MXFAST, 2048);
- mallopt(M_NLBLKS, 50);
- buf = SAFE_MALLOC(NULL, 1024);
+ if (mallopt(M_MXFAST, MAX_FAST_SIZE) == 0)
+  tst_resm(TFAIL, "mallopt(M_MXFAST, %d) failed", MAX_FAST_SIZE);
+ else
+  tst_resm(TPASS, "mallopt(M_MXFAST, %d) succeeded", MAX_FAST_SIZE);
+
+ if (mallopt(M_NLBLKS, 50) == 0)
+  tst_resm(TFAIL, "mallopt(M_NLBLKS, 50) failed");
+ else
+  tst_resm(TPASS, "mallopt(M_NLBLKS, 50) succeeded");
+
+ if ((buf = SAFE_MALLOC(NULL, 1024)) == NULL)
+  tst_resm(TFAIL, "malloc(1024) failed");
+ else {
+  tst_resm(TPASS, "malloc(1024) succeeded");
+  free(buf);
+ }
 
- free(buf);
+ if (mallopt(M_MXFAST, 0) == 0)
+  tst_resm(TFAIL, "mallopt(M_MXFAST, 0) failed");
+ else
+  tst_resm(TPASS, "mallopt(M_MXFAST, 0) succeeded");
+
+ if ((buf = SAFE_MALLOC(NULL, 1024)) == NULL)
+  tst_resm(TFAIL, "malloc(1024) failed");
+ else {
+  tst_resm(TPASS, "malloc(1024) succeeded");
+  free(buf);
+ }
 
  unlink("core");
  tst_rmdir();
#1 lore raw

PATCH 1/1 syscalls/mallopt: Does not work correctly?

Hi!

Quoted 3 lines

Therefore, I suggest to adding following functions.
1. Checking mallopt() return values.
2. Setting tests value is 0 and 80*sizeof(size_t)/4.

Sounds good, comments to the patch itself below.

Quoted 11 lines
testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 renamed from testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-08 18:37:03.000000000 +0900 +1 -0
@@ -51,6 +51,7 @@
 
 #define FAILED 0
 #define PASSED 1
+#define MAX_FAST_SIZE (80 * sizeof(size_t) / 4)
 
 int local_flag = PASSED;

testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 int main(int argc, char *argv[]) · line 76

Quoted 6 lines
testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 renamed from testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-08 18:37:03.000000000 +0900 +2 -1
@@ -71,7 +72,8 @@ int main(int argc, char *argv[])
 
  tst_tmpdir();
 
- buf = SAFE_MALLOC(NULL, 20480);
+ if ((buf = SAFE_MALLOC(NULL, 20480)) == NULL)
+  tst_brkm(TBROK, NULL, "malloc failed");

The purpose of SAFE_MALLOC() is to allocate the buffer, or exit the test. It will never return NULL.

testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 int main(int argc, char *argv[]) · line 90

Quoted 6 lines
testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 renamed from testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-08 18:37:03.000000000 +0900 +0 -0
  /*
   * Check space usage.

@@ -86,16 +88,41 @@ int main(int argc, char *argv[])
   printinfo();
   tst_resm(TFAIL, "mallinfo failed: smblks != 0");
  }

We use tabs for indentation. It looks like even the context of the patch set uses spaces instead. One possibility is that your email client converted tabs to spaces. Please figure out how to send it correcty, as it is the patch will not apply.

Quoted 16 lines
testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 renamed from testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-08 18:37:03.000000000 +0900 +8 -3
+ if (info.uordblks >= 20480 && info.smblks == 0)
+  tst_resm(TPASS, "mallinfo() succeeded");
  free(buf);
 
  /*
   * Test mallopt's M_MXFAST and M_NLBLKS cmds.
   */
- mallopt(M_MXFAST, 2048);
- mallopt(M_NLBLKS, 50);
- buf = SAFE_MALLOC(NULL, 1024);
+ if (mallopt(M_MXFAST, MAX_FAST_SIZE) == 0)
+  tst_resm(TFAIL, "mallopt(M_MXFAST, %d) failed", MAX_FAST_SIZE);
+ else
+  tst_resm(TPASS, "mallopt(M_MXFAST, %d) succeeded", MAX_FAST_SIZE);
+
+ if (mallopt(M_NLBLKS, 50) == 0)

testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 int main(int argc, char *argv[]) · line 113

Quoted 10 lines
testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 renamed from testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-08 18:37:03.000000000 +0900 +10 -0
+  tst_resm(TFAIL, "mallopt(M_NLBLKS, 50) failed");
+ else
+  tst_resm(TPASS, "mallopt(M_NLBLKS, 50) succeeded");
+
+ if ((buf = SAFE_MALLOC(NULL, 1024)) == NULL)
+  tst_resm(TFAIL, "malloc(1024) failed");
+ else {
+  tst_resm(TPASS, "malloc(1024) succeeded");
+  free(buf);
+ }

Here as well, the SAFE_MALLOC() will never fail, just use plain old malloc() here instead.

testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 int main(int argc, char *argv[]) · line 125

Quoted 12 lines
testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10 16:20:44.471493667 +0900 renamed from testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-08 18:37:03.000000000 +0900 +11 -1
- free(buf);
+ if (mallopt(M_MXFAST, 0) == 0)
+  tst_resm(TFAIL, "mallopt(M_MXFAST, 0) failed");
+ else
+  tst_resm(TPASS, "mallopt(M_MXFAST, 0) succeeded");
+
+ if ((buf = SAFE_MALLOC(NULL, 1024)) == NULL)
+  tst_resm(TFAIL, "malloc(1024) failed");
+ else {
+  tst_resm(TPASS, "malloc(1024) succeeded");
+  free(buf);
+ }

And here as well.

Other than that you should also update the TST_TOTAL to match the number of PASS/FAIL messages

Signature · 2 lines
Cyril Hrubis
[email protected]
author
#2 lore raw

PATCH 1/1 syscalls/mallopt: Does not work correctly?

Hi!

Thanks for reviewing.

Quoted 2 lines

The purpose of SAFE_MALLOC() is to allocate the buffer, or exit the test. It will never return NULL.

Ok, I deleted unnecessary NULL check.

Quoted 11 lines

Review int main(int argc, char *argv[]) · line 90

Quoted 6 lines

/*
* Check space usage.

(quoted diff) +0 -0
@@ -86,16 +88,41 @@ int main(int argc, char *argv[])
   printinfo();
   tst_resm(TFAIL, "mallinfo failed: smblks != 0");
  }

We use tabs for indentation. It looks like even the context of the patch set uses spaces instead. One possibility is that your email client converted tabs to spaces. Please figure out how to send it correcty, as it is the patch will not apply.

Sorry, It was converted into the space by mistake. I fixed patch.

Quoted 2 lines

Here as well, the SAFE_MALLOC() will never fail, just use plain old malloc() here instead.

I replace SAFE_MALLOC after malopts with "malloc".

Quoted 2 lines

Other than that you should also update the TST_TOTAL to match the number of PASS/FAIL messages

Ok, I updated TST_TOTAL to match the number of PASS/FAIL messages

---

testcases/kernel/syscalls/mallopt/mallopt01.c +32 -5
diff -urpN a/testcases/kernel/syscalls/mallopt/mallopt01.c b/testcases/kernel/syscalls/mallopt/mallopt01.c
@@ -51,13 +51,14 @@
 
 #define FAILED 0
 #define PASSED 1
+#define MAX_FAST_SIZE	(80 * sizeof(size_t) / 4)
 
 int local_flag = PASSED;
 
 char *TCID = "mallopt01";
 int block_number;
 FILE *temp;
-int TST_TOTAL = 1;
+int TST_TOTAL = 6;
 extern int tst_COUNT;		/* Test Case counter for tst_routines */
 
 void printinfo();

@@ -86,16 +87,42 @@ int main(int argc, char *argv[])
 		printinfo();
 		tst_resm(TFAIL, "mallinfo failed: smblks != 0");
 	}
+	if (info.uordblks >= 20480 && info.smblks == 0)
+		tst_resm(TPASS, "mallinfo() succeeded");
 	free(buf);
 
 	/*
 	 * Test mallopt's M_MXFAST and M_NLBLKS cmds.
 	 */
-	mallopt(M_MXFAST, 2048);
-	mallopt(M_NLBLKS, 50);
-	buf = SAFE_MALLOC(NULL, 1024);
 
-	free(buf);
+	if (mallopt(M_MXFAST, MAX_FAST_SIZE) == 0)
+		tst_resm(TFAIL, "mallopt(M_MXFAST, %d) failed", MAX_FAST_SIZE);
+	else
+		tst_resm(TPASS, "mallopt(M_MXFAST, %d) succeeded", MAX_FAST_SIZE);
+
+	if (mallopt(M_NLBLKS, 50) == 0)
+		tst_resm(TFAIL, "mallopt(M_NLBLKS, 50) failed");
+	else
+		tst_resm(TPASS, "mallopt(M_NLBLKS, 50) succeeded");
+
+	if ((buf = malloc(1024)) == NULL)
+		tst_resm(TFAIL, "malloc(1024) failed");
+	else {
+		tst_resm(TPASS, "malloc(1024) succeeded");
+		free(buf);
+	}
+
+	if (mallopt(M_MXFAST, 0) == 0)
+		tst_resm(TFAIL, "mallopt(M_MXFAST, 0) failed");
+	else
+		tst_resm(TPASS, "mallopt(M_MXFAST, 0) succeeded");
+
+	if ((buf = malloc(1024)) == NULL)
+		tst_resm(TFAIL, "malloc(1024) failed");
+	else {
+		tst_resm(TPASS, "malloc(1024) succeeded");
+		free(buf);
+	}
 
 	unlink("core");
 	tst_rmdir();

---

Best regards.

Quoted 100 lines

-----Original Message-----
From: Cyril Hrubis [mailto:[email protected]]
Sent: Thursday, October 01, 2015 8:30 PM
To: TAKAHASHI Tetsuya
Cc: [email protected]; TAKEISHI Kazuhiro
Subject: Re: [LTP] [PATCH] syscalls/mallopt: Does not work correctly?

Hi!

Quoted 3 lines

Therefore, I suggest to adding following functions.
1. Checking mallopt() return values.
2. Setting tests value is 0 and 80*sizeof(size_t)/4.

Sounds good, comments to the patch itself below.

Quoted 1 line

--- a/testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-08

18:37:03.000000000 +0900

Quoted 1 line

+++ b/testcases/kernel/syscalls/mallopt/mallopt01.c 2015-09-10

16:20:44.471493667 +0900

Quoted 9 lines
(quoted diff) +1 -0
@@ -51,6 +51,7 @@
 
 #define FAILED 0
 #define PASSED 1
+#define MAX_FAST_SIZE (80 * sizeof(size_t) / 4)
 
 int local_flag = PASSED;

Review int main(int argc, char *argv[]) · line 76

Quoted 6 lines
(quoted diff) +2 -1
@@ -71,7 +72,8 @@ int main(int argc, char *argv[])
 
  tst_tmpdir();
 
- buf = SAFE_MALLOC(NULL, 20480);
+ if ((buf = SAFE_MALLOC(NULL, 20480)) == NULL)
+  tst_brkm(TBROK, NULL, "malloc failed");

The purpose of SAFE_MALLOC() is to allocate the buffer, or exit the test. It will never return NULL.

Review int main(int argc, char *argv[]) · line 90

Quoted 6 lines
(quoted diff) +0 -0
  /*
   * Check space usage.

@@ -86,16 +88,41 @@ int main(int argc, char *argv[])
   printinfo();
   tst_resm(TFAIL, "mallinfo failed: smblks != 0");
  }

We use tabs for indentation. It looks like even the context of the patch set uses spaces instead. One possibility is that your email client converted tabs to spaces. Please figure out how to send it correcty, as it is the patch will not apply.

Quoted 16 lines
(quoted diff) +8 -3
+ if (info.uordblks >= 20480 && info.smblks == 0)
+  tst_resm(TPASS, "mallinfo() succeeded");
  free(buf);
 
  /*
   * Test mallopt's M_MXFAST and M_NLBLKS cmds.
   */
- mallopt(M_MXFAST, 2048);
- mallopt(M_NLBLKS, 50);
- buf = SAFE_MALLOC(NULL, 1024);
+ if (mallopt(M_MXFAST, MAX_FAST_SIZE) == 0)
+  tst_resm(TFAIL, "mallopt(M_MXFAST, %d) failed", MAX_FAST_SIZE);
+ else
+  tst_resm(TPASS, "mallopt(M_MXFAST, %d) succeeded", MAX_FAST_SIZE);
+
+ if (mallopt(M_NLBLKS, 50) == 0)

Review int main(int argc, char *argv[]) · line 113

Quoted 10 lines
(quoted diff) +10 -0
+  tst_resm(TFAIL, "mallopt(M_NLBLKS, 50) failed");
+ else
+  tst_resm(TPASS, "mallopt(M_NLBLKS, 50) succeeded");
+
+ if ((buf = SAFE_MALLOC(NULL, 1024)) == NULL)
+  tst_resm(TFAIL, "malloc(1024) failed");
+ else {
+  tst_resm(TPASS, "malloc(1024) succeeded");
+  free(buf);
+ }

Here as well, the SAFE_MALLOC() will never fail, just use plain old malloc() here instead.

Review int main(int argc, char *argv[]) · line 124

Quoted 12 lines
(quoted diff) +11 -1
- free(buf);
+ if (mallopt(M_MXFAST, 0) == 0)
+  tst_resm(TFAIL, "mallopt(M_MXFAST, 0) failed");
+ else
+  tst_resm(TPASS, "mallopt(M_MXFAST, 0) succeeded");
+
+ if ((buf = SAFE_MALLOC(NULL, 1024)) == NULL)
+  tst_resm(TFAIL, "malloc(1024) failed");
+ else {
+  tst_resm(TPASS, "malloc(1024) succeeded");
+  free(buf);
+ }

And here as well.

Other than that you should also update the TST_TOTAL to match the number of PASS/FAIL messages

--
Cyril Hrubis
[email protected]

#3 lore raw

PATCH 1/1 syscalls/mallopt: Does not work correctly?

Hi!
I've wrote commit description for the change, add your signed-of-by, did small changes (see below) and pushed, thanks.

testcases/kernel/syscalls/mallopt/mallopt01.c int main(int argc, char *argv[]) · line 101

Quoted 5 lines
testcases/kernel/syscalls/mallopt/mallopt01.c +4 -1
-	free(buf);
+	if (mallopt(M_MXFAST, MAX_FAST_SIZE) == 0)
+		tst_resm(TFAIL, "mallopt(M_MXFAST, %d) failed", MAX_FAST_SIZE);
+	else
+		tst_resm(TPASS, "mallopt(M_MXFAST, %d) succeeded", MAX_FAST_SIZE);

I've added cast to (int) for the MAX_FAST_SIZE constant here to supress warnings.

testcases/kernel/syscalls/mallopt/mallopt01.c int main(int argc, char *argv[]) · line 113

Quoted 6 lines
testcases/kernel/syscalls/mallopt/mallopt01.c +6 -0
+	if ((buf = malloc(1024)) == NULL)
+		tst_resm(TFAIL, "malloc(1024) failed");
+	else {
+		tst_resm(TPASS, "malloc(1024) succeeded");
+		free(buf);
+	}

LKML coding style preffers curly braces around both blocks if they are needed around one of them. So I've added them before I pushed the patch.

FYI, you can use checkpatch.pl (distributed with Linux kernel sources) to check patches before submission.

Signature · 2 lines
Cyril Hrubis
[email protected]