diff mbox series

[v2,2/2] msgget03: don't depend on existed shared resources

Message ID 20210712075223.10682-2-aleksei.kodanev@bell-sw.com
State Superseded
Headers show
Series [v2,1/2] shmget03: don't depend on existed shared resources | expand

Commit Message

Alexey Kodanev July 12, 2021, 7:52 a.m. UTC
It's unlikely, but still possible that some of them could be
created/released during the test as well, so the patch only
checks errno.

Signed-off-by: Alexey Kodanev <aleksei.kodanev@bell-sw.com>
---
v2: * Move the loop to the test run function and try to get
      ENOSPC errno there.

 .../kernel/syscalls/ipc/msgget/msgget03.c     | 31 ++++++++++---------
 1 file changed, 16 insertions(+), 15 deletions(-)

Comments

Petr Vorel July 22, 2021, 7:55 a.m. UTC | #1
Hi Alexey, Li,

> It's unlikely, but still possible that some of them could be
> created/released during the test as well, so the patch only
> checks errno.

> Signed-off-by: Alexey Kodanev <aleksei.kodanev@bell-sw.com>
> ---
> v2: * Move the loop to the test run function and try to get
>       ENOSPC errno there.

>  .../kernel/syscalls/ipc/msgget/msgget03.c     | 31 ++++++++++---------
>  1 file changed, 16 insertions(+), 15 deletions(-)

> diff --git a/testcases/kernel/syscalls/ipc/msgget/msgget03.c b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
> index 76cf82cd3..1ade8f942 100644
> --- a/testcases/kernel/syscalls/ipc/msgget/msgget03.c
> +++ b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
> @@ -26,29 +26,30 @@ static key_t msgkey;

>  static void verify_msgget(void)
>  {
> -	TST_EXP_FAIL2(msgget(msgkey + maxmsgs, IPC_CREAT | IPC_EXCL), ENOSPC,
> -		"msgget(%i, %i)", msgkey + maxmsgs, IPC_CREAT | IPC_EXCL);
> +	int res = 0, num;
> +
> +	errno = 0;
> +	for (num = 0; num <= maxmsgs; ++num) {
In different patch [1] (I forget you already send patches to fix this) I counted
items in /proc/sysvipc/shm. Not sure what is safer: <= looks a bit drastic
(how about bug which reports ENOSPC much earlier than it should be?), but
obviously new mapping from other program created in the middle of testing.

Kind regards,
Petr

[1] https://patchwork.ozlabs.org/project/ltp/patch/20210722073523.5099-1-pvorel@suse.cz/
> +		res = msgget(msgkey + num, IPC_CREAT | IPC_EXCL);
> +		if (res == -1)
> +			break;
> +		queues[queue_cnt++] = res;
> +	}
> +
> +	if (res != -1 || errno != ENOSPC)
> +		tst_brk(TFAIL | TERRNO, "Failed to trigger ENOSPC error");
> +
> +	tst_res(TPASS, "Maximum number of queues reached (%d), used by test %d",
> +		maxmsgs, queue_cnt);
>  }
...
Cyril Hrubis July 22, 2021, 12:14 p.m. UTC | #2
Hi!
> In different patch [1] (I forget you already send patches to fix this) I counted
> items in /proc/sysvipc/shm. Not sure what is safer: <= looks a bit drastic
> (how about bug which reports ENOSPC much earlier than it should be?), but
> obviously new mapping from other program created in the middle of testing.

I think that we allready discussed this in another thread:

https://lists.linux.it/pipermail/ltp/2021-July/023831.html
Petr Vorel July 22, 2021, 1:01 p.m. UTC | #3
> Hi!
> > In different patch [1] (I forget you already send patches to fix this) I counted
> > items in /proc/sysvipc/shm. Not sure what is safer: <= looks a bit drastic
> > (how about bug which reports ENOSPC much earlier than it should be?), but
> > obviously new mapping from other program created in the middle of testing.

> I think that we allready discussed this in another thread:

> https://lists.linux.it/pipermail/ltp/2021-July/023831.html

Thanks, I forgot this. In that case my approach (not using <=, but count
segments in /proc/sysvipc/shm before testing) might be more precise.
But no strong feeling about that, both solutions fix the test, let's chose one
and merge.

Kind regards,
Petr
Cyril Hrubis July 22, 2021, 1:02 p.m. UTC | #4
Hi!
> > I think that we allready discussed this in another thread:
> 
> > https://lists.linux.it/pipermail/ltp/2021-July/023831.html
> 
> Thanks, I forgot this. In that case my approach (not using <=, but count
> segments in /proc/sysvipc/shm before testing) might be more precise.
> But no strong feeling about that, both solutions fix the test, let's chose one
> and merge.

As I said previously, there are many SysV IPC tests that do expect that
nobody will add/remove IPC shm/queue/semaphores during the testrun and
some of the testcases cannot even be implemented without this
expectation.

Hence I wouldn't complicate the test here and just count how many
segments are there at the start and be done with it.
xuyang2018.jy@fujitsu.com July 23, 2021, 8:46 a.m. UTC | #5
Hi Cyril, Petr
> Hi!
>>> I think that we allready discussed this in another thread:
>>
>>> https://lists.linux.it/pipermail/ltp/2021-July/023831.html
>>
>> Thanks, I forgot this. In that case my approach (not using<=, but count
>> segments in /proc/sysvipc/shm before testing) might be more precise.
>> But no strong feeling about that, both solutions fix the test, let's chose one
>> and merge.
>
> As I said previously, there are many SysV IPC tests that do expect that
> nobody will add/remove IPC shm/queue/semaphores during the testrun and
> some of the testcases cannot even be implemented without this
> expectation.
>
> Hence I wouldn't complicate the test here and just count how many
> segments are there at the start and be done with it.
Agree.

A possible solution(alter get_used_queues api in new ipc lib and add 
file parametrers, so we can use this api for msgget03) I have mentioned 
in the previous email, the url as below:
https://lists.linux.it/pipermail/ltp/2021-July/023653.html
>
Petr Vorel July 23, 2021, 12:11 p.m. UTC | #6
Hi Cyril, all,

> Hi!
> > > I think that we allready discussed this in another thread:

> > > https://lists.linux.it/pipermail/ltp/2021-July/023831.html

> > Thanks, I forgot this. In that case my approach (not using <=, but count
> > segments in /proc/sysvipc/shm before testing) might be more precise.
> > But no strong feeling about that, both solutions fix the test, let's chose one
> > and merge.

> As I said previously, there are many SysV IPC tests that do expect that
> nobody will add/remove IPC shm/queue/semaphores during the testrun and
> some of the testcases cannot even be implemented without this
> expectation.

> Hence I wouldn't complicate the test here and just count how many
> segments are there at the start and be done with it.
Yes, that's what's done in "my approach" [1].

Kind regards,
Petr

[1] https://patchwork.ozlabs.org/project/ltp/patch/20210722073523.5099-1-pvorel@suse.cz/
Petr Vorel July 23, 2021, 12:24 p.m. UTC | #7
Hi all,

> Hi Cyril, Petr
> > Hi!
> >>> I think that we allready discussed this in another thread:

> >>> https://lists.linux.it/pipermail/ltp/2021-July/023831.html

> >> Thanks, I forgot this. In that case my approach (not using<=, but count
> >> segments in /proc/sysvipc/shm before testing) might be more precise.
> >> But no strong feeling about that, both solutions fix the test, let's chose one
> >> and merge.

> > As I said previously, there are many SysV IPC tests that do expect that
> > nobody will add/remove IPC shm/queue/semaphores during the testrun and
> > some of the testcases cannot even be implemented without this
> > expectation.

> > Hence I wouldn't complicate the test here and just count how many
> > segments are there at the start and be done with it.
> Agree.

> A possible solution(alter get_used_queues api in new ipc lib and add 
> file parametrers, so we can use this api for msgget03) I have mentioned 
> in the previous email, the url as below:
> https://lists.linux.it/pipermail/ltp/2021-July/023653.html
LGTM. Or use /proc/sysvipc/shm instead of /proc/sysvipc/msg in get_used_queues()
as you noted get_used_queues() has not been used yet.

BTW searching where get_used_queues() appeared, I see [LTP] [PATCH v3 1/4]
syscalls/ipc: add newipc library for new API [1], but if I'm not mistaken
get_used_queues() was not used even there, maybe it was in some previous
versions.

Kind regards,
Petr

[1] https://lists.linux.it/pipermail/ltp/2016-December/003239.html
xuyang2018.jy@fujitsu.com July 27, 2021, 5:51 a.m. UTC | #8
Hi Petr
> Hi all,
>
>> Hi Cyril, Petr
>>> Hi!
>>>>> I think that we allready discussed this in another thread:
>
>>>>> https://lists.linux.it/pipermail/ltp/2021-July/023831.html
>
>>>> Thanks, I forgot this. In that case my approach (not using<=, but count
>>>> segments in /proc/sysvipc/shm before testing) might be more precise.
>>>> But no strong feeling about that, both solutions fix the test, let's chose one
>>>> and merge.
>
>>> As I said previously, there are many SysV IPC tests that do expect that
>>> nobody will add/remove IPC shm/queue/semaphores during the testrun and
>>> some of the testcases cannot even be implemented without this
>>> expectation.
>
>>> Hence I wouldn't complicate the test here and just count how many
>>> segments are there at the start and be done with it.
>> Agree.
>
>> A possible solution(alter get_used_queues api in new ipc lib and add
>> file parametrers, so we can use this api for msgget03) I have mentioned
>> in the previous email, the url as below:
>> https://lists.linux.it/pipermail/ltp/2021-July/023653.html
> LGTM. Or use /proc/sysvipc/shm instead of /proc/sysvipc/msg in get_used_queues()
> as you noted get_used_queues() has not been used yet.
I rename get_used_queues to get_used_sysvipc_cnt. see attached patch.
>
> BTW searching where get_used_queues() appeared, I see [LTP] [PATCH v3 1/4]
> syscalls/ipc: add newipc library for new API [1], but if I'm not mistaken
> get_used_queues() was not used even there, maybe it was in some previous
> versions.
Yes, no new api case use GET_USED_QUEUES api.
>
> Kind regards,
> Petr
>
> [1] https://lists.linux.it/pipermail/ltp/2016-December/003239.html
From 2772f8f0bbc1526389cb2090895dded41e2c43dc Mon Sep 17 00:00:00 2001
From: Yang Xu <xuyang2018.jy@fujitsu.com>
Date: Tue, 27 Jul 2021 16:22:42 +0800
Subject: [PATCH] libs/libnewipc:Rename get_used_queues as get_used_sysvipc_cnt

Rename get_used_queues as get_used_sysvipc_cnt, so we can use GET_USED_QUEQUES()
and GET_USED_SEGMENTS() to get the corresponding used sysvipc resource total.

Then we can use them in shmget03/msgget03, so we can trigger the ENOSPC error correctly
even current environment has consume some sysvipc resource.

I don't use this api in verify function since we don't support run cases in parallel and
we should assume this situation that this case is the only case to use(free or alloc) sysv
ipc resource at that time.

Fixes: #842
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 include/libnewipc.h                             |  6 ++++--
 libs/libltpnewipc/libnewipc.c                   | 16 ++++++++--------
 testcases/kernel/syscalls/ipc/msgget/msgget03.c | 10 +++++++---
 testcases/kernel/syscalls/ipc/shmget/shmget03.c | 10 ++++++----
 4 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/include/libnewipc.h b/include/libnewipc.h
index 075364f85..b0448841a 100644
--- a/include/libnewipc.h
+++ b/include/libnewipc.h
@@ -49,9 +49,11 @@ key_t getipckey(const char *file, const int lineno);
 #define GETIPCKEY() \
 	getipckey(__FILE__, __LINE__)
 
-int get_used_queues(const char *file, const int lineno);
+int get_used_sysvipc_cnt(const char *file, const int lineno, const char *sysvipc_file);
 #define GET_USED_QUEUES() \
-	get_used_queues(__FILE__, __LINE__)
+	get_used_sysvipc_cnt(__FILE__, __LINE__, "/proc/sysvipc/msg")
+#define GET_USED_SEGMENTS() \
+	get_used_sysvipc_cnt(__FILE__, __LINE__, "/proc/sysvipc/shm")
 
 void *probe_free_addr(const char *file, const int lineno);
 #define PROBE_FREE_ADDR() \
diff --git a/libs/libltpnewipc/libnewipc.c b/libs/libltpnewipc/libnewipc.c
index d0974bbe0..687a907e7 100644
--- a/libs/libltpnewipc/libnewipc.c
+++ b/libs/libltpnewipc/libnewipc.c
@@ -48,25 +48,25 @@ key_t getipckey(const char *file, const int lineno)
 	return key;
 }
 
-int get_used_queues(const char *file, const int lineno)
+int get_used_sysvipc_cnt(const char *file, const int lineno, const char *sysvipc_file)
 {
 	FILE *fp;
-	int used_queues = -1;
+	int used_cnt = -1;
 	char buf[BUFSIZE];
 
-	fp = safe_fopen(file, lineno, NULL, "/proc/sysvipc/msg", "r");
+	fp = safe_fopen(file, lineno, NULL, sysvipc_file, "r");
 
 	while (fgets(buf, BUFSIZE, fp) != NULL)
-		used_queues++;
+		used_cnt++;
 
 	fclose(fp);
 
-	if (used_queues < 0) {
-		tst_brk(TBROK, "can't read /proc/sysvipc/msg to get "
-			"used message queues at %s:%d", file, lineno);
+	if (used_cnt < 0) {
+		tst_brk(TBROK, "can't read %s to get used message queues "
+			"at %s:%d", sysvipc_file, file, lineno);
 	}
 
-	return used_queues;
+	return used_cnt;
 }
 
 void *probe_free_addr(const char *file, const int lineno)
diff --git a/testcases/kernel/syscalls/ipc/msgget/msgget03.c b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
index ab5714cdc..8ccffc547 100644
--- a/testcases/kernel/syscalls/ipc/msgget/msgget03.c
+++ b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
@@ -21,7 +21,7 @@
 #include "tst_safe_sysv_ipc.h"
 #include "libnewipc.h"
 
-static int maxmsgs, queue_cnt;
+static int maxmsgs, queue_cnt, existed_cnt;
 static int *queues;
 static key_t msgkey;
 
@@ -37,11 +37,15 @@ static void setup(void)
 
 	msgkey = GETIPCKEY();
 
+	existed_cnt = GET_USED_QUEUES();
+	tst_res(TINFO, "Current environment %d message queues are already in use",
+		existed_cnt);
+
 	SAFE_FILE_SCANF("/proc/sys/kernel/msgmni", "%i", &maxmsgs);
 
-	queues = SAFE_MALLOC(maxmsgs * sizeof(int));
+	queues = SAFE_MALLOC((maxmsgs - existed_cnt) * sizeof(int));
 
-	for (num = 0; num < maxmsgs; num++) {
+	for (num = 0; num < maxmsgs - existed_cnt; num++) {
 		res = msgget(msgkey + num, IPC_CREAT | IPC_EXCL);
 		if (res == -1)
 			tst_brk(TBROK | TERRNO, "msgget failed unexpectedly");
diff --git a/testcases/kernel/syscalls/ipc/shmget/shmget03.c b/testcases/kernel/syscalls/ipc/shmget/shmget03.c
index efbc465e1..acd352796 100644
--- a/testcases/kernel/syscalls/ipc/shmget/shmget03.c
+++ b/testcases/kernel/syscalls/ipc/shmget/shmget03.c
@@ -22,7 +22,7 @@
 #include "libnewipc.h"
 
 static int *queues;
-static int maxshms, queue_cnt;
+static int maxshms, queue_cnt, existed_cnt;
 static key_t shmkey;
 
 static void verify_shmget(void)
@@ -36,11 +36,13 @@ static void setup(void)
 	int res, num;
 
 	shmkey = GETIPCKEY();
-
+	existed_cnt = GET_USED_SEGMENTS();
+	tst_res(TINFO, "Current environment %d shared memory segments are already in use",
+		existed_cnt);
 	SAFE_FILE_SCANF("/proc/sys/kernel/shmmni", "%i", &maxshms);
 
-	queues = SAFE_MALLOC(maxshms * sizeof(int));
-	for (num = 0; num < maxshms; num++) {
+	queues = SAFE_MALLOC((maxshms - existed_cnt) * sizeof(int));
+	for (num = 0; num < maxshms - existed_cnt; num++) {
 		res = shmget(shmkey + num, SHM_SIZE, IPC_CREAT | IPC_EXCL | SHM_RW);
 		if (res == -1)
 			tst_brk(TBROK | TERRNO, "shmget failed unexpectedly");
xuyang2018.jy@fujitsu.com Aug. 4, 2021, 1:45 a.m. UTC | #9
Hi Cyril, Petr
> Hi Petr
>> Hi all,
>>
>>> Hi Cyril, Petr
>>>> Hi!
>>>>>> I think that we allready discussed this in another thread:
>>
>>>>>> https://lists.linux.it/pipermail/ltp/2021-July/023831.html
>>
>>>>> Thanks, I forgot this. In that case my approach (not using<=, but
>>>>> count
>>>>> segments in /proc/sysvipc/shm before testing) might be more precise.
>>>>> But no strong feeling about that, both solutions fix the test,
>>>>> let's chose one
>>>>> and merge.
>>
>>>> As I said previously, there are many SysV IPC tests that do expect that
>>>> nobody will add/remove IPC shm/queue/semaphores during the testrun and
>>>> some of the testcases cannot even be implemented without this
>>>> expectation.
>>
>>>> Hence I wouldn't complicate the test here and just count how many
>>>> segments are there at the start and be done with it.
>>> Agree.
>>
>>> A possible solution(alter get_used_queues api in new ipc lib and add
>>> file parametrers, so we can use this api for msgget03) I have mentioned
>>> in the previous email, the url as below:
>>> https://lists.linux.it/pipermail/ltp/2021-July/023653.html
>> LGTM. Or use /proc/sysvipc/shm instead of /proc/sysvipc/msg in
>> get_used_queues()
>> as you noted get_used_queues() has not been used yet.
> I rename get_used_queues to get_used_sysvipc_cnt. see attached patch.
Any idea about the attached patch(it was in previous eamil)?

ps:more and more people sent patch to fix this problme, I don't want to 
see many patches for this problem. Let's choose a solution to fix this 
problem.

Best Regards
Yang Xu
>>
>> BTW searching where get_used_queues() appeared, I see [LTP] [PATCH v3
>> 1/4]
>> syscalls/ipc: add newipc library for new API [1], but if I'm not mistaken
>> get_used_queues() was not used even there, maybe it was in some previous
>> versions.
> Yes, no new api case use GET_USED_QUEUES api.
>>
>> Kind regards,
>> Petr
>>
>> [1] https://lists.linux.it/pipermail/ltp/2016-December/003239.html
>
Cyril Hrubis Aug. 4, 2021, 2:48 p.m. UTC | #10
Hi!
> From 2772f8f0bbc1526389cb2090895dded41e2c43dc Mon Sep 17 00:00:00 2001
> From: Yang Xu <xuyang2018.jy@fujitsu.com>
> Date: Tue, 27 Jul 2021 16:22:42 +0800
> Subject: [PATCH] libs/libnewipc:Rename get_used_queues as get_used_sysvipc_cnt
> 
> Rename get_used_queues as get_used_sysvipc_cnt, so we can use GET_USED_QUEQUES()
> and GET_USED_SEGMENTS() to get the corresponding used sysvipc resource total.
> 
> Then we can use them in shmget03/msgget03, so we can trigger the ENOSPC error correctly
> even current environment has consume some sysvipc resource.
> 
> I don't use this api in verify function since we don't support run cases in parallel and
> we should assume this situation that this case is the only case to use(free or alloc) sysv
> ipc resource at that time.
> 
> Fixes: #842
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  include/libnewipc.h                             |  6 ++++--
>  libs/libltpnewipc/libnewipc.c                   | 16 ++++++++--------
>  testcases/kernel/syscalls/ipc/msgget/msgget03.c | 10 +++++++---
>  testcases/kernel/syscalls/ipc/shmget/shmget03.c | 10 ++++++----
>  4 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/include/libnewipc.h b/include/libnewipc.h
> index 075364f85..b0448841a 100644
> --- a/include/libnewipc.h
> +++ b/include/libnewipc.h
> @@ -49,9 +49,11 @@ key_t getipckey(const char *file, const int lineno);
>  #define GETIPCKEY() \
>  	getipckey(__FILE__, __LINE__)
>  
> -int get_used_queues(const char *file, const int lineno);
> +int get_used_sysvipc_cnt(const char *file, const int lineno, const char *sysvipc_file);
>  #define GET_USED_QUEUES() \
> -	get_used_queues(__FILE__, __LINE__)
> +	get_used_sysvipc_cnt(__FILE__, __LINE__, "/proc/sysvipc/msg")
> +#define GET_USED_SEGMENTS() \
> +	get_used_sysvipc_cnt(__FILE__, __LINE__, "/proc/sysvipc/shm")

I would just call it get_used_sysvipc()

>  void *probe_free_addr(const char *file, const int lineno);
>  #define PROBE_FREE_ADDR() \
> diff --git a/libs/libltpnewipc/libnewipc.c b/libs/libltpnewipc/libnewipc.c
> index d0974bbe0..687a907e7 100644
> --- a/libs/libltpnewipc/libnewipc.c
> +++ b/libs/libltpnewipc/libnewipc.c
> @@ -48,25 +48,25 @@ key_t getipckey(const char *file, const int lineno)
>  	return key;
>  }
>  
> -int get_used_queues(const char *file, const int lineno)
> +int get_used_sysvipc_cnt(const char *file, const int lineno, const char *sysvipc_file)
>  {
>  	FILE *fp;
> -	int used_queues = -1;
> +	int used_cnt = -1;

And here as well the _cnt is not adding any value over I would say.

>  	char buf[BUFSIZE];
>  
> -	fp = safe_fopen(file, lineno, NULL, "/proc/sysvipc/msg", "r");
> +	fp = safe_fopen(file, lineno, NULL, sysvipc_file, "r");
>  
>  	while (fgets(buf, BUFSIZE, fp) != NULL)
> -		used_queues++;
> +		used_cnt++;
>  
>  	fclose(fp);
>  
> -	if (used_queues < 0) {
> -		tst_brk(TBROK, "can't read /proc/sysvipc/msg to get "
> -			"used message queues at %s:%d", file, lineno);
> +	if (used_cnt < 0) {
> +		tst_brk(TBROK, "can't read %s to get used message queues "
> +			"at %s:%d", sysvipc_file, file, lineno);
>  	}
>  
> -	return used_queues;
> +	return used_cnt;
>  }
>  
>  void *probe_free_addr(const char *file, const int lineno)
> diff --git a/testcases/kernel/syscalls/ipc/msgget/msgget03.c b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
> index ab5714cdc..8ccffc547 100644
> --- a/testcases/kernel/syscalls/ipc/msgget/msgget03.c
> +++ b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
> @@ -21,7 +21,7 @@
>  #include "tst_safe_sysv_ipc.h"
>  #include "libnewipc.h"
>  
> -static int maxmsgs, queue_cnt;
> +static int maxmsgs, queue_cnt, existed_cnt;
                                  ^
				  Why not 'used_cnt' ?
>  static int *queues;
>  static key_t msgkey;
>  
> @@ -37,11 +37,15 @@ static void setup(void)
>  
>  	msgkey = GETIPCKEY();
>  
> +	existed_cnt = GET_USED_QUEUES();
> +	tst_res(TINFO, "Current environment %d message queues are already in use",
> +		existed_cnt);
> +
>  	SAFE_FILE_SCANF("/proc/sys/kernel/msgmni", "%i", &maxmsgs);
>  
> -	queues = SAFE_MALLOC(maxmsgs * sizeof(int));
> +	queues = SAFE_MALLOC((maxmsgs - existed_cnt) * sizeof(int));
>  
> -	for (num = 0; num < maxmsgs; num++) {
> +	for (num = 0; num < maxmsgs - existed_cnt; num++) {
>  		res = msgget(msgkey + num, IPC_CREAT | IPC_EXCL);
>  		if (res == -1)
>  			tst_brk(TBROK | TERRNO, "msgget failed unexpectedly");
> diff --git a/testcases/kernel/syscalls/ipc/shmget/shmget03.c b/testcases/kernel/syscalls/ipc/shmget/shmget03.c
> index efbc465e1..acd352796 100644
> --- a/testcases/kernel/syscalls/ipc/shmget/shmget03.c
> +++ b/testcases/kernel/syscalls/ipc/shmget/shmget03.c
> @@ -22,7 +22,7 @@
>  #include "libnewipc.h"
>  
>  static int *queues;
> -static int maxshms, queue_cnt;
> +static int maxshms, queue_cnt, existed_cnt;
                                   ^
				   Here as well.
>  static key_t shmkey;
>  
>  static void verify_shmget(void)
> @@ -36,11 +36,13 @@ static void setup(void)
>  	int res, num;
>  
>  	shmkey = GETIPCKEY();
> -
> +	existed_cnt = GET_USED_SEGMENTS();
> +	tst_res(TINFO, "Current environment %d shared memory segments are already in use",
> +		existed_cnt);
>  	SAFE_FILE_SCANF("/proc/sys/kernel/shmmni", "%i", &maxshms);
>  
> -	queues = SAFE_MALLOC(maxshms * sizeof(int));
> -	for (num = 0; num < maxshms; num++) {
> +	queues = SAFE_MALLOC((maxshms - existed_cnt) * sizeof(int));
> +	for (num = 0; num < maxshms - existed_cnt; num++) {
>  		res = shmget(shmkey + num, SHM_SIZE, IPC_CREAT | IPC_EXCL | SHM_RW);
>  		if (res == -1)
>  			tst_brk(TBROK | TERRNO, "shmget failed unexpectedly");

Other than the very minor differencies I would do in naming the
variables and function this looks good to me.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Petr Vorel Aug. 4, 2021, 3:45 p.m. UTC | #11
Hi Xu, Cyril,

> Other than the very minor differencies I would do in naming the
> variables and function this looks good to me.

Agree with all Cyril's suggestions for rename.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

I suppose you just merge it fixed version, right?
Because now we discuss under Alexey's "[v2,2/2] msgget03: don't depend on
existed shared resources" patch.

Kind regards,
Petr
xuyang2018.jy@fujitsu.com Aug. 5, 2021, 3:43 a.m. UTC | #12
Hi Cyril, Petr
> Hi!
>>  From 2772f8f0bbc1526389cb2090895dded41e2c43dc Mon Sep 17 00:00:00 2001
>> From: Yang Xu<xuyang2018.jy@fujitsu.com>
>> Date: Tue, 27 Jul 2021 16:22:42 +0800
>> Subject: [PATCH] libs/libnewipc:Rename get_used_queues as get_used_sysvipc_cnt
>>
>> Rename get_used_queues as get_used_sysvipc_cnt, so we can use GET_USED_QUEQUES()
>> and GET_USED_SEGMENTS() to get the corresponding used sysvipc resource total.
>>
>> Then we can use them in shmget03/msgget03, so we can trigger the ENOSPC error correctly
>> even current environment has consume some sysvipc resource.
>>
>> I don't use this api in verify function since we don't support run cases in parallel and
>> we should assume this situation that this case is the only case to use(free or alloc) sysv
>> ipc resource at that time.
>>
>> Fixes: #842
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   include/libnewipc.h                             |  6 ++++--
>>   libs/libltpnewipc/libnewipc.c                   | 16 ++++++++--------
>>   testcases/kernel/syscalls/ipc/msgget/msgget03.c | 10 +++++++---
>>   testcases/kernel/syscalls/ipc/shmget/shmget03.c | 10 ++++++----
>>   4 files changed, 25 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/libnewipc.h b/include/libnewipc.h
>> index 075364f85..b0448841a 100644
>> --- a/include/libnewipc.h
>> +++ b/include/libnewipc.h
>> @@ -49,9 +49,11 @@ key_t getipckey(const char *file, const int lineno);
>>   #define GETIPCKEY() \
>>   	getipckey(__FILE__, __LINE__)
>>
>> -int get_used_queues(const char *file, const int lineno);
>> +int get_used_sysvipc_cnt(const char *file, const int lineno, const char *sysvipc_file);
>>   #define GET_USED_QUEUES() \
>> -	get_used_queues(__FILE__, __LINE__)
>> +	get_used_sysvipc_cnt(__FILE__, __LINE__, "/proc/sysvipc/msg")
>> +#define GET_USED_SEGMENTS() \
>> +	get_used_sysvipc_cnt(__FILE__, __LINE__, "/proc/sysvipc/shm")
>
> I would just call it get_used_sysvipc()
OK.
>
>>   void *probe_free_addr(const char *file, const int lineno);
>>   #define PROBE_FREE_ADDR() \
>> diff --git a/libs/libltpnewipc/libnewipc.c b/libs/libltpnewipc/libnewipc.c
>> index d0974bbe0..687a907e7 100644
>> --- a/libs/libltpnewipc/libnewipc.c
>> +++ b/libs/libltpnewipc/libnewipc.c
>> @@ -48,25 +48,25 @@ key_t getipckey(const char *file, const int lineno)
>>   	return key;
>>   }
>>
>> -int get_used_queues(const char *file, const int lineno)
>> +int get_used_sysvipc_cnt(const char *file, const int lineno, const char *sysvipc_file)
>>   {
>>   	FILE *fp;
>> -	int used_queues = -1;
>> +	int used_cnt = -1;
>
> And here as well the _cnt is not adding any value over I would say.
OK.
>
>>   	char buf[BUFSIZE];
>>
>> -	fp = safe_fopen(file, lineno, NULL, "/proc/sysvipc/msg", "r");
>> +	fp = safe_fopen(file, lineno, NULL, sysvipc_file, "r");
>>
>>   	while (fgets(buf, BUFSIZE, fp) != NULL)
>> -		used_queues++;
>> +		used_cnt++;
>>
>>   	fclose(fp);
>>
>> -	if (used_queues<  0) {
>> -		tst_brk(TBROK, "can't read /proc/sysvipc/msg to get "
>> -			"used message queues at %s:%d", file, lineno);
>> +	if (used_cnt<  0) {
>> +		tst_brk(TBROK, "can't read %s to get used message queues "
>> +			"at %s:%d", sysvipc_file, file, lineno);
>>   	}
I also modify this info.
message queues => sysvipc resource total
>>
>> -	return used_queues;
>> +	return used_cnt;
>>   }
>>
>>   void *probe_free_addr(const char *file, const int lineno)
>> diff --git a/testcases/kernel/syscalls/ipc/msgget/msgget03.c b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
>> index ab5714cdc..8ccffc547 100644
>> --- a/testcases/kernel/syscalls/ipc/msgget/msgget03.c
>> +++ b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
>> @@ -21,7 +21,7 @@
>>   #include "tst_safe_sysv_ipc.h"
>>   #include "libnewipc.h"
>>
>> -static int maxmsgs, queue_cnt;
>> +static int maxmsgs, queue_cnt, existed_cnt;
>                                    ^
> 				  Why not 'used_cnt' ?
Yes.
>>   static int *queues;
>>   static key_t msgkey;
>>
>> @@ -37,11 +37,15 @@ static void setup(void)
>>
>>   	msgkey = GETIPCKEY();
>>
>> +	existed_cnt = GET_USED_QUEUES();
>> +	tst_res(TINFO, "Current environment %d message queues are already in use",
>> +		existed_cnt);
>> +
>>   	SAFE_FILE_SCANF("/proc/sys/kernel/msgmni", "%i",&maxmsgs);
>>
>> -	queues = SAFE_MALLOC(maxmsgs * sizeof(int));
>> +	queues = SAFE_MALLOC((maxmsgs - existed_cnt) * sizeof(int));
>>
>> -	for (num = 0; num<  maxmsgs; num++) {
>> +	for (num = 0; num<  maxmsgs - existed_cnt; num++) {
>>   		res = msgget(msgkey + num, IPC_CREAT | IPC_EXCL);
>>   		if (res == -1)
>>   			tst_brk(TBROK | TERRNO, "msgget failed unexpectedly");
>> diff --git a/testcases/kernel/syscalls/ipc/shmget/shmget03.c b/testcases/kernel/syscalls/ipc/shmget/shmget03.c
>> index efbc465e1..acd352796 100644
>> --- a/testcases/kernel/syscalls/ipc/shmget/shmget03.c
>> +++ b/testcases/kernel/syscalls/ipc/shmget/shmget03.c
>> @@ -22,7 +22,7 @@
>>   #include "libnewipc.h"
>>
>>   static int *queues;
>> -static int maxshms, queue_cnt;
>> +static int maxshms, queue_cnt, existed_cnt;
>                                     ^
> 				   Here as well.
OK.
>>   static key_t shmkey;
>>
>>   static void verify_shmget(void)
>> @@ -36,11 +36,13 @@ static void setup(void)
>>   	int res, num;
>>
>>   	shmkey = GETIPCKEY();
>> -
>> +	existed_cnt = GET_USED_SEGMENTS();
>> +	tst_res(TINFO, "Current environment %d shared memory segments are already in use",
>> +		existed_cnt);
>>   	SAFE_FILE_SCANF("/proc/sys/kernel/shmmni", "%i",&maxshms);
>>
>> -	queues = SAFE_MALLOC(maxshms * sizeof(int));
>> -	for (num = 0; num<  maxshms; num++) {
>> +	queues = SAFE_MALLOC((maxshms - existed_cnt) * sizeof(int));
>> +	for (num = 0; num<  maxshms - existed_cnt; num++) {
>>   		res = shmget(shmkey + num, SHM_SIZE, IPC_CREAT | IPC_EXCL | SHM_RW);
>>   		if (res == -1)
>>   			tst_brk(TBROK | TERRNO, "shmget failed unexpectedly");
>
> Other than the very minor differencies I would do in naming the
> variables and function this looks good to me.
>
> Reviewed-by: Cyril Hrubis<chrubis@suse.cz>
Thanks for your review. I spilt this patch into a patchset because it is 
more friendly for user or tester.

Best Regards
Yang Xu
>
>
Petr Vorel Aug. 5, 2021, 6:36 a.m. UTC | #13
Hi Xu,

> >> -	if (used_queues<  0) {
> >> -		tst_brk(TBROK, "can't read /proc/sysvipc/msg to get "
> >> -			"used message queues at %s:%d", file, lineno);
> >> +	if (used_cnt<  0) {
> >> +		tst_brk(TBROK, "can't read %s to get used message queues "
> >> +			"at %s:%d", sysvipc_file, file, lineno);
> >>   	}
> I also modify this info.
> message queues => sysvipc resource total
+1. nit: I'd also move "at" at the previous line (better for grep).

Kind regards,
Petr
xuyang2018.jy@fujitsu.com Aug. 5, 2021, 6:58 a.m. UTC | #14
Hi Petr
> Hi Xu,
>
>>>> -	if (used_queues<   0) {
>>>> -		tst_brk(TBROK, "can't read /proc/sysvipc/msg to get "
>>>> -			"used message queues at %s:%d", file, lineno);
>>>> +	if (used_cnt<   0) {
>>>> +		tst_brk(TBROK, "can't read %s to get used message queues "
>>>> +			"at %s:%d", sysvipc_file, file, lineno);
>>>>    	}
>> I also modify this info.
>> message queues =>  sysvipc resource total
> +1. nit: I'd also move "at" at the previous line (better for grep).
Sounds reasonable. Thanks.

Best Regards
Yang Xu
>
> Kind regards,
> Petr
>
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/ipc/msgget/msgget03.c b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
index 76cf82cd3..1ade8f942 100644
--- a/testcases/kernel/syscalls/ipc/msgget/msgget03.c
+++ b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
@@ -26,29 +26,30 @@  static key_t msgkey;
 
 static void verify_msgget(void)
 {
-	TST_EXP_FAIL2(msgget(msgkey + maxmsgs, IPC_CREAT | IPC_EXCL), ENOSPC,
-		"msgget(%i, %i)", msgkey + maxmsgs, IPC_CREAT | IPC_EXCL);
+	int res = 0, num;
+
+	errno = 0;
+	for (num = 0; num <= maxmsgs; ++num) {
+		res = msgget(msgkey + num, IPC_CREAT | IPC_EXCL);
+		if (res == -1)
+			break;
+		queues[queue_cnt++] = res;
+	}
+
+	if (res != -1 || errno != ENOSPC)
+		tst_brk(TFAIL | TERRNO, "Failed to trigger ENOSPC error");
+
+	tst_res(TPASS, "Maximum number of queues reached (%d), used by test %d",
+		maxmsgs, queue_cnt);
 }
 
 static void setup(void)
 {
-	int res, num;
-
 	msgkey = GETIPCKEY();
 
 	SAFE_FILE_SCANF("/proc/sys/kernel/msgmni", "%i", &maxmsgs);
 
-	queues = SAFE_MALLOC(maxmsgs * sizeof(int));
-
-	for (num = 0; num < maxmsgs; num++) {
-		res = msgget(msgkey + num, IPC_CREAT | IPC_EXCL);
-		if (res == -1)
-			tst_brk(TBROK | TERRNO, "msgget failed unexpectedly");
-		queues[queue_cnt++] = res;
-	}
-
-	tst_res(TINFO, "The maximum number of message queues (%d) reached",
-		maxmsgs);
+	queues = SAFE_MALLOC((maxmsgs + 1) * sizeof(int));
 }
 
 static void cleanup(void)