Skip to contentSkip to conversation

quotactl01: replace int comparison with memcmp

PATCH New Jan Stancek 3 participants 4 messages Jan 13, 2017 – Jan 16, 2017

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 quotactl01: replace int comparison with memcmp

Testcase currently uses unsigned long for all returned and expected values. This doesn't work for some cases, for example Q_GETFMT returns 32bit number. So if we pass in unsigned long (8 bytes) on big endian, the comparison breaks:

quotactl01.c:198: FAIL: quotactl got unexpected info 8589934592, expected 2 2 << 32 == 8589934592

This patch converts parameters to void* and adds size value. After syscall it passes all to memcmp.

Patch also removes dummy "data" variable and replaces it with NULL and 0 size parameters.

---
 testcases/kernel/syscalls/quotactl/quotactl01.c | 99 +++++++++++++++----------
 1 file changed, 58 insertions(+), 41 deletions(-)
testcases/kernel/syscalls/quotactl/quotactl01.c +58 -41
index 586272af1d0c..fb74a8a6404f 100644
@@ -79,9 +79,8 @@
 # define MNTPOINT	"mntpoint"
 
 static int mount_flag;
-static unsigned long fmt_id = FMTID;
-static unsigned long test_id;
-static unsigned long data;
+static int32_t fmt_id = FMTID;
+static int test_id;
 static struct dqblk set_dq = {
 	.dqb_bsoftlimit = 100,
 	.dqb_valid = QIF_BLIMITS
@@ -93,53 +92,70 @@ static struct dqinfo set_qf = {
 	.dqi_valid = IIF_BGRACE
 };
 static struct dqinfo res_qf;
-static unsigned long fmt_buf[32];
+static int32_t fmt_buf;
 # endif
 
 static struct tcase {
 	int cmd;
-	unsigned long *id;
+	int *id;
 	void *addr;
-	unsigned long *set_data;
-	unsigned long *res_data;
+	void *set_data;
+	void *res_data;
+	int sz;
 	char *des;
 } tcases[] = {
-	{QCMD(Q_QUOTAON, USRQUOTA), &fmt_id, USRPATH, &data, &data,
-	"turn on quota for user"},
-	{QCMD(Q_SETQUOTA, USRQUOTA), &test_id, &set_dq, &data, &data,
-	"set disk quota limit for user"},
-	{QCMD(Q_GETQUOTA, USRQUOTA), &test_id, &res_dq, &set_dq.dqb_bsoftlimit,
-	&res_dq.dqb_bsoftlimit, "get disk quota limit for user"},
+	{QCMD(Q_QUOTAON, USRQUOTA), &fmt_id, USRPATH,
+	NULL, NULL, 0, "turn on quota for user"},
+
+	{QCMD(Q_SETQUOTA, USRQUOTA), &test_id, &set_dq,
+	NULL, NULL, 0, "set disk quota limit for user"},
+
+	{QCMD(Q_GETQUOTA, USRQUOTA), &test_id, &res_dq,
+	&set_dq.dqb_bsoftlimit, &res_dq.dqb_bsoftlimit,
+	sizeof(res_dq.dqb_bsoftlimit), "get disk quota limit for user"},
 # if defined(HAVE_QUOTAV2)
-	{QCMD(Q_SETINFO, USRQUOTA), &test_id, &set_qf, &data, &data,
-	"set information about quotafile for user"},
-	{QCMD(Q_GETINFO, USRQUOTA), &test_id, &res_qf, &set_qf.dqi_bgrace,
-	&res_qf.dqi_bgrace, "get information about quotafile for user"},
-	{QCMD(Q_GETFMT, USRQUOTA), &test_id, fmt_buf, &fmt_id, fmt_buf,
+	{QCMD(Q_SETINFO, USRQUOTA), &test_id, &set_qf,
+	NULL, NULL, 0, "set information about quotafile for user"},
+
+	{QCMD(Q_GETINFO, USRQUOTA), &test_id, &res_qf,
+	&set_qf.dqi_bgrace, &res_qf.dqi_bgrace, sizeof(res_qf.dqi_bgrace),
+	"get information about quotafile for user"},
+
+	{QCMD(Q_GETFMT, USRQUOTA), &test_id, &fmt_buf,
+	&fmt_id, &fmt_buf, sizeof(fmt_buf),
 	"get quota format for user"},
 # endif
-	{QCMD(Q_SYNC, USRQUOTA), &test_id, &res_dq, &data, &data,
-	"update quota usages for user"},
-	{QCMD(Q_QUOTAOFF, USRQUOTA), &test_id, USRPATH, &data, &data,
-	"turn off quota for user"},
-	{QCMD(Q_QUOTAON, GRPQUOTA), &fmt_id, GRPPATH, &data, &data,
-	"turn on quota for group"},
-	{QCMD(Q_SETQUOTA, GRPQUOTA), &test_id, &set_dq, &data, &data,
-	"set disk quota limit for group"},
+	{QCMD(Q_SYNC, USRQUOTA), &test_id, &res_dq,
+	NULL, NULL, 0, "update quota usages for user"},
+
+	{QCMD(Q_QUOTAOFF, USRQUOTA), &test_id, USRPATH,
+	NULL, NULL, 0, "turn off quota for user"},
+
+	{QCMD(Q_QUOTAON, GRPQUOTA), &fmt_id, GRPPATH,
+	NULL, NULL, 0, "turn on quota for group"},
+
+	{QCMD(Q_SETQUOTA, GRPQUOTA), &test_id, &set_dq,
+	NULL, NULL, 0, "set disk quota limit for group"},
+
 	{QCMD(Q_GETQUOTA, GRPQUOTA), &test_id, &res_dq, &set_dq.dqb_bsoftlimit,
-	&res_dq.dqb_bsoftlimit, "set disk quota limit for group"},
+	&res_dq.dqb_bsoftlimit, sizeof(res_dq.dqb_bsoftlimit),
+	"set disk quota limit for group"},
 # if defined(HAVE_QUOTAV2)
-	{QCMD(Q_SETINFO, GRPQUOTA), &test_id, &set_qf, &data, &data,
-	"set information about quotafile for group"},
+	{QCMD(Q_SETINFO, GRPQUOTA), &test_id, &set_qf,
+	NULL, NULL, 0, "set information about quotafile for group"},
+
 	{QCMD(Q_GETINFO, GRPQUOTA), &test_id, &res_qf, &set_qf.dqi_bgrace,
-	&res_qf.dqi_bgrace, "get information about quotafile for group"},
-	{QCMD(Q_GETFMT, GRPQUOTA), &test_id, fmt_buf, &fmt_id, fmt_buf,
-	"get quota format for group"},
+	&res_qf.dqi_bgrace, sizeof(res_qf.dqi_bgrace),
+	"get information about quotafile for group"},
+
+	{QCMD(Q_GETFMT, GRPQUOTA), &test_id, &fmt_buf,
+	&fmt_id, &fmt_buf, sizeof(fmt_buf), "get quota format for group"},
 # endif
-	{QCMD(Q_SYNC, GRPQUOTA), &test_id, &res_dq, &data, &data,
-	"update quota usages for group"},
-	{QCMD(Q_QUOTAOFF, GRPQUOTA), &test_id, GRPPATH, &data, &data,
-	"turn off quota for group"}
+	{QCMD(Q_SYNC, GRPQUOTA), &test_id, &res_dq,
+	NULL, NULL, 0, "update quota usages for group"},
+
+	{QCMD(Q_QUOTAOFF, GRPQUOTA), &test_id, GRPPATH,
+	NULL, NULL, 0, "turn off quota for group"}
 };
 
 static void setup(void)
@@ -185,7 +201,7 @@ static void verify_quota(unsigned int n)
 
 	res_dq.dqb_bsoftlimit = 0;
 	res_qf.dqi_igrace = 0;
-	memset(fmt_buf, 0, sizeof(fmt_buf));
+	memset(&fmt_buf, 0, sizeof(fmt_buf));
 
 	TEST(quotactl(tc->cmd, tst_device->dev, *tc->id, tc->addr));
 	if (TEST_RETURN == -1) {
@@ -193,13 +209,14 @@ static void verify_quota(unsigned int n)
 		return;
 	}
 
-	if (*tc->set_data != *tc->res_data) {
-		tst_res(TFAIL, "quotactl got unexpected info %lu, "
-			"expected %lu", *tc->res_data, *tc->set_data);
+	if (memcmp(tc->res_data, tc->set_data, tc->sz)) {
+		tst_res(TFAIL, "quotactl failed to %s", tc->des);
+		tst_res_hexd(TINFO, tc->res_data, tc->sz, "retval:   ");
+		tst_res_hexd(TINFO, tc->set_data, tc->sz, "expected: ");
 		return;
 	}
 
-	tst_res(TPASS, "quotactl secceeded to %s", tc->des);
+	tst_res(TPASS, "quotactl succeeded to %s", tc->des);
 }
 
 static struct tst_test test = {
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH 1/1 quotactl01: replace int comparison with memcmp

Quoted 6 lines · Jan Stancek

Testcase currently uses unsigned long for all returned and expected values. This doesn't work for some cases, for example Q_GETFMT returns 32bit number. So if we pass in unsigned long (8 bytes) on big endian, the comparison breaks:

Quoted 8 lines

quotactl01.c:198: FAIL: quotactl got unexpected info 8589934592, expected 2 2 << 32 == 8589934592

This patch converts parameters to void* and adds size value. After syscall it passes all to memcmp.

Patch also removes dummy "data" variable and replaces it with NULL and 0 size parameters.

Hi Jan

It looks fine to me, thanks for your patch.

Best Regards,
Xiao Yang

Quoted 153 lines
---
 testcases/kernel/syscalls/quotactl/quotactl01.c | 99 +++++++++++++++----------
 1 file changed, 58 insertions(+), 41 deletions(-)
testcases/kernel/syscalls/quotactl/quotactl01.c +58 -41
index 586272af1d0c..fb74a8a6404f 100644
@@ -79,9 +79,8 @@
 # define MNTPOINT	"mntpoint"
 
 static int mount_flag;
-static unsigned long fmt_id = FMTID;
-static unsigned long test_id;
-static unsigned long data;
+static int32_t fmt_id = FMTID;
+static int test_id;
 static struct dqblk set_dq = {
 	.dqb_bsoftlimit = 100,
 	.dqb_valid = QIF_BLIMITS
@@ -93,53 +92,70 @@ static struct dqinfo set_qf = {
 	.dqi_valid = IIF_BGRACE
 };
 static struct dqinfo res_qf;
-static unsigned long fmt_buf[32];
+static int32_t fmt_buf;
 # endif
 
 static struct tcase {
 	int cmd;
-	unsigned long *id;
+	int *id;
 	void *addr;
-	unsigned long *set_data;
-	unsigned long *res_data;
+	void *set_data;
+	void *res_data;
+	int sz;
 	char *des;
 } tcases[] = {
-	{QCMD(Q_QUOTAON, USRQUOTA), &fmt_id, USRPATH, &data, &data,
-	"turn on quota for user"},
-	{QCMD(Q_SETQUOTA, USRQUOTA), &test_id, &set_dq, &data, &data,
-	"set disk quota limit for user"},
-	{QCMD(Q_GETQUOTA, USRQUOTA), &test_id, &res_dq, &set_dq.dqb_bsoftlimit,
-	&res_dq.dqb_bsoftlimit, "get disk quota limit for user"},
+	{QCMD(Q_QUOTAON, USRQUOTA), &fmt_id, USRPATH,
+	NULL, NULL, 0, "turn on quota for user"},
+
+	{QCMD(Q_SETQUOTA, USRQUOTA), &test_id, &set_dq,
+	NULL, NULL, 0, "set disk quota limit for user"},
+
+	{QCMD(Q_GETQUOTA, USRQUOTA), &test_id, &res_dq,
+	&set_dq.dqb_bsoftlimit, &res_dq.dqb_bsoftlimit,
+	sizeof(res_dq.dqb_bsoftlimit), "get disk quota limit for user"},
 # if defined(HAVE_QUOTAV2)
-	{QCMD(Q_SETINFO, USRQUOTA), &test_id, &set_qf, &data, &data,
-	"set information about quotafile for user"},
-	{QCMD(Q_GETINFO, USRQUOTA), &test_id, &res_qf, &set_qf.dqi_bgrace,
-	&res_qf.dqi_bgrace, "get information about quotafile for user"},
-	{QCMD(Q_GETFMT, USRQUOTA), &test_id, fmt_buf, &fmt_id, fmt_buf,
+	{QCMD(Q_SETINFO, USRQUOTA), &test_id, &set_qf,
+	NULL, NULL, 0, "set information about quotafile for user"},
+
+	{QCMD(Q_GETINFO, USRQUOTA), &test_id, &res_qf,
+	&set_qf.dqi_bgrace, &res_qf.dqi_bgrace, sizeof(res_qf.dqi_bgrace),
+	"get information about quotafile for user"},
+
+	{QCMD(Q_GETFMT, USRQUOTA), &test_id, &fmt_buf,
+	&fmt_id, &fmt_buf, sizeof(fmt_buf),
 	"get quota format for user"},
 # endif
-	{QCMD(Q_SYNC, USRQUOTA), &test_id, &res_dq, &data, &data,
-	"update quota usages for user"},
-	{QCMD(Q_QUOTAOFF, USRQUOTA), &test_id, USRPATH, &data, &data,
-	"turn off quota for user"},
-	{QCMD(Q_QUOTAON, GRPQUOTA), &fmt_id, GRPPATH, &data, &data,
-	"turn on quota for group"},
-	{QCMD(Q_SETQUOTA, GRPQUOTA), &test_id, &set_dq, &data, &data,
-	"set disk quota limit for group"},
+	{QCMD(Q_SYNC, USRQUOTA), &test_id, &res_dq,
+	NULL, NULL, 0, "update quota usages for user"},
+
+	{QCMD(Q_QUOTAOFF, USRQUOTA), &test_id, USRPATH,
+	NULL, NULL, 0, "turn off quota for user"},
+
+	{QCMD(Q_QUOTAON, GRPQUOTA), &fmt_id, GRPPATH,
+	NULL, NULL, 0, "turn on quota for group"},
+
+	{QCMD(Q_SETQUOTA, GRPQUOTA), &test_id, &set_dq,
+	NULL, NULL, 0, "set disk quota limit for group"},
+
 	{QCMD(Q_GETQUOTA, GRPQUOTA), &test_id, &res_dq, &set_dq.dqb_bsoftlimit,
-	&res_dq.dqb_bsoftlimit, "set disk quota limit for group"},
+	&res_dq.dqb_bsoftlimit, sizeof(res_dq.dqb_bsoftlimit),
+	"set disk quota limit for group"},
 # if defined(HAVE_QUOTAV2)
-	{QCMD(Q_SETINFO, GRPQUOTA), &test_id, &set_qf, &data, &data,
-	"set information about quotafile for group"},
+	{QCMD(Q_SETINFO, GRPQUOTA), &test_id, &set_qf,
+	NULL, NULL, 0, "set information about quotafile for group"},
+
 	{QCMD(Q_GETINFO, GRPQUOTA), &test_id, &res_qf, &set_qf.dqi_bgrace,
-	&res_qf.dqi_bgrace, "get information about quotafile for group"},
-	{QCMD(Q_GETFMT, GRPQUOTA), &test_id, fmt_buf, &fmt_id, fmt_buf,
-	"get quota format for group"},
+	&res_qf.dqi_bgrace, sizeof(res_qf.dqi_bgrace),
+	"get information about quotafile for group"},
+
+	{QCMD(Q_GETFMT, GRPQUOTA), &test_id, &fmt_buf,
+	&fmt_id, &fmt_buf, sizeof(fmt_buf), "get quota format for group"},
 # endif
-	{QCMD(Q_SYNC, GRPQUOTA), &test_id, &res_dq, &data, &data,
-	"update quota usages for group"},
-	{QCMD(Q_QUOTAOFF, GRPQUOTA), &test_id, GRPPATH, &data, &data,
-	"turn off quota for group"}
+	{QCMD(Q_SYNC, GRPQUOTA), &test_id, &res_dq,
+	NULL, NULL, 0, "update quota usages for group"},
+
+	{QCMD(Q_QUOTAOFF, GRPQUOTA), &test_id, GRPPATH,
+	NULL, NULL, 0, "turn off quota for group"}
 };
 
 static void setup(void)
@@ -185,7 +201,7 @@ static void verify_quota(unsigned int n)
 
 	res_dq.dqb_bsoftlimit = 0;
 	res_qf.dqi_igrace = 0;
-	memset(fmt_buf, 0, sizeof(fmt_buf));
+	memset(&fmt_buf, 0, sizeof(fmt_buf));
 
 	TEST(quotactl(tc->cmd, tst_device->dev, *tc->id, tc->addr));
 	if (TEST_RETURN == -1) {
@@ -193,13 +209,14 @@ static void verify_quota(unsigned int n)
 		return;
 	}
 
-	if (*tc->set_data != *tc->res_data) {
-		tst_res(TFAIL, "quotactl got unexpected info %lu, "
-			"expected %lu", *tc->res_data, *tc->set_data);
+	if (memcmp(tc->res_data, tc->set_data, tc->sz)) {
+		tst_res(TFAIL, "quotactl failed to %s", tc->des);
+		tst_res_hexd(TINFO, tc->res_data, tc->sz, "retval:   ");
+		tst_res_hexd(TINFO, tc->set_data, tc->sz, "expected: ");
 		return;
 	}
 
-	tst_res(TPASS, "quotactl secceeded to %s", tc->des);
+	tst_res(TPASS, "quotactl succeeded to %s", tc->des);
 }
 
 static struct tst_test test = {
#2 lore raw

PATCH 1/1 quotactl01: replace int comparison with memcmp

In reply to Jan Stancek

Hi!

testcases/kernel/syscalls/quotactl/quotactl01.c static void verify_quota(unsigned int n) · line 204

Quoted 2 lines
testcases/kernel/syscalls/quotactl/quotactl01.c +1 -1
-	memset(fmt_buf, 0, sizeof(fmt_buf));
+	memset(&fmt_buf, 0, sizeof(fmt_buf));

Hmm, we can simply do fmt_buf = 0 here now, right?

Otherwise this looks good, acked.

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

PATCH 1/1 quotactl01: replace int comparison with memcmp

----- Original Message -----

Quoted 11 lines

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected], "yangx jy" <[email protected]>
Sent: Monday, 16 January, 2017 12:50:57 PM
Subject: Re: [PATCH] quotactl01: replace int comparison with memcmp

Hi!

testcases/kernel/syscalls/quotactl/quotactl01.c static void verify_quota(unsigned int n) · line 204

Quoted 2 lines
testcases/kernel/syscalls/quotactl/quotactl01.c +1 -1
-	memset(fmt_buf, 0, sizeof(fmt_buf));
+	memset(&fmt_buf, 0, sizeof(fmt_buf));

Hmm, we can simply do fmt_buf = 0 here now, right?

Replaced with simple assignment and pushed.

Regards,
Jan

Quoted 7 lines

Otherwise this looks good, acked.

--
Cyril Hrubis
[email protected]