Re: [LTP] [PATCH v15] semctl01: fix SEM_STAT failures under parallel LTP runs

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Stephan,

> diff --git a/include/tse_newipc.h b/include/tse_newipc.h
> index 1d3bbd129..175a60234 100644
> --- a/include/tse_newipc.h
> +++ b/include/tse_newipc.h
> @@ -49,4 +49,24 @@ void *probe_free_addr(const char *file, const int lineno);
>  #define PROBE_FREE_ADDR() \
>  	probe_free_addr(__FILE__, __LINE__)
>  
> +/**
> + * get_ipc_idx_from_id() - Find the kernel index of an IPC resource by id.
> + *
> + * @id: The msqid/semid/shmid to look for.
> + * @max_idx: Highest kernel index to search, from IPC_INFO/SEM_INFO/SHM_INFO.
> + * @stat_fn: Per-family *_STAT wrapper called as stat_fn(idx, buf) for
> + *           each idx in [0, max_idx]; must return the id at idx, or -1
> + *           on error, without aborting the test.
> + * @buf: Buffer passed through to @stat_fn.
> + *
> + * SysV *_STAT commands (MSG_STAT/SEM_STAT/SHM_STAT) take a kernel array
> + * index rather than the id returned by *get(), and that index is not
> + * guaranteed to match the id when other IPC users are active. This walks
> + * the live indices to find the one that currently maps to @id.
> + *
> + * Return: The kernel index mapping to @id, or -1 if none was found.
> + */
> +int get_ipc_idx_from_id(int id, int max_idx,
> +			int (*stat_fn)(int idx, void *buf), void *buf);
> +
>  #endif /* tse_newipc.h */
> diff --git a/libs/newipc/tse_newipc.c b/libs/newipc/tse_newipc.c
> index f7edda6b5..aee286a31 100644
> --- a/libs/newipc/tse_newipc.c
> +++ b/libs/newipc/tse_newipc.c
> @@ -86,3 +86,16 @@ void *probe_free_addr(const char *file, const int lineno)
>  
>  	return addr;
>  }
> +
> +int get_ipc_idx_from_id(int id, int max_idx,
> +			int (*stat_fn)(int idx, void *buf), void *buf)
> +{
> +	int i;
> +
> +	for (i = 0; i <= max_idx; i++) {
> +		if (stat_fn(i, buf) == id)
> +			return i;
> +	}
> +
> +	return -1;
> +}

can we add patch 1/2 for this..

> diff --git a/testcases/kernel/syscalls/semctl/Makefile b/testcases/kernel/syscalls/semctl/Makefile
> index 0e7223998..1f1ec9c81 100644
> --- a/testcases/kernel/syscalls/semctl/Makefile
> +++ b/testcases/kernel/syscalls/semctl/Makefile
> @@ -8,6 +8,6 @@ LTPLIBS = ipc newipc
>  include $(top_srcdir)/include/mk/testcases.mk
>  
>  semctl06: LTPLDLIBS = -lltpipc
> -semctl02 semctl03 semctl04 semctl05 semctl07 semctl08 semctl09: LTPLDLIBS = -lltpnewipc
> +semctl01 semctl02 semctl03 semctl04 semctl05 semctl07 semctl08 semctl09: LTPLDLIBS = -lltpnewipc
>  
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/semctl/semctl01.c b/testcases/kernel/syscalls/semctl/semctl01.c
> index 5bd675ab6..398cb3028 100644
> --- a/testcases/kernel/syscalls/semctl/semctl01.c
> +++ b/testcases/kernel/syscalls/semctl/semctl01.c
> @@ -210,13 +210,16 @@ static void func_rmid(void)
>  
>  static void func_iinfo(int hidx)
>  {
> -	if (hidx >= 0) {
> -		sem_index = hidx;
> -		tst_res(TPASS, "the highest index is correct");
> -	} else {
> -		sem_index = 0;
> -		tst_res(TFAIL, "the highest index is incorrect");
> -	}
> +	/*
> +	 * Return value is the highest used index. Our set is alive, so the
> +	 * highest used index must be at least our index.
> +	 */
> +	if (hidx >= sem_index)
> +		tst_res(TPASS, "IPC_INFO highest index %d >= our index %d",
> +			hidx, sem_index);
> +	else
> +		tst_res(TFAIL, "IPC_INFO highest index %d < our index %d",
> +			hidx, sem_index);
>  }
>  
>  static void func_sinfo(void)
> @@ -227,12 +230,54 @@ static void func_sinfo(void)
>  		tst_res(TPASS, "number of semaphore sets is correct");
>  }
>  
> -static void func_sstat(int semidx)
> +/*
> + * SEM_STAT fills buf the same way IPC_STAT does. sem_nsems and
> + * sem_perm.mode (set via IPC_SET earlier) verify SEM_STAT's own data,
> + * not just the id already used to find sem_index.
> + */
> +static void func_sstat(int semid)
>  {
> -	if (semidx >= 0)
> -		tst_res(TPASS, "id of the semaphore set is correct");
> +	if (semid != sem_id) {
> +		tst_res(TFAIL, "expected sem_id %d, got %d", sem_id, semid);
> +		return;
> +	}
> +
> +	if (buf.sem_nsems == PSEMS && buf.sem_perm.mode == (SEM_RA | NEWMODE))
> +		tst_res(TPASS, "id and semaphore STAT info are correct (id=%d)",
> +			sem_id);
>  	else
> -		tst_res(TFAIL, "id of the semaphore set is incorrect");
> +		tst_res(TFAIL, "nsems=%d (expected %d), mode=%o (expected %o)",
> +			(int)buf.sem_nsems, PSEMS,
> +			buf.sem_perm.mode, (SEM_RA | NEWMODE));
> +}
> +
> +/*
> + * SAFE_SEMCTL() would abort the test on per-index failures.
> + */
> +static int sem_stat(int idx, void *buf)
> +{
> +	union semun arg;
> +
> +	arg.buf = buf;
> +	return semctl(idx, 0, SEM_STAT, arg);
> +}
> +
> +/*
> + * SEM_STAT takes an index into the kernel's internal array, not a semid.
> + * Return the index that maps to this test's set.
> + */
> +static int get_sem_idx_from_id(int id)
> +{
> +	struct seminfo info;
> +	struct semid_ds dummy_ds;
> +	union semun arg;
> +	int max_idx;
> +
> +	arg.__buf = &info;
> +	/* SEM_INFO ignores semid; but SAFE_SEMCTL requires an lvalue */
> +	max_idx = SAFE_SEMCTL(id, 0, SEM_INFO, arg);
> +
> +	return get_ipc_idx_from_id(id, max_idx, sem_stat, &dummy_ds);
>  }
>  
>  static struct tcases {
> @@ -263,8 +308,15 @@ static void verify_semctl(unsigned int n)
>  	struct tcases *tc = &tests[n];
>  	int rval;
>  
> -	if (sem_id == -1)
> +	if (sem_id == -1) {
>  		sem_id = SAFE_SEMGET(IPC_PRIVATE, PSEMS, IPC_CREAT | IPC_EXCL | SEM_RA);
> +		sem_index = get_sem_idx_from_id(sem_id);
> +		if (sem_index < 0)
> +			tst_brk(TBROK,
> +				"Failed to get sem_id %d to idx mapping", sem_id);
> +		tst_res(TINFO, "sem_id=%d maps to kernel index=%d",
> +			sem_id, sem_index);
> +	}
>  	if (tc->func_setup) {
>  		switch (tc->cmd) {
>  		case GETNCNT:
> diff --git a/testcases/kernel/syscalls/shmctl/shmctl01.c b/testcases/kernel/syscalls/shmctl/shmctl01.c
> index 05aea58cc..bfc11fa39 100644
> --- a/testcases/kernel/syscalls/shmctl/shmctl01.c
> +++ b/testcases/kernel/syscalls/shmctl/shmctl01.c
> @@ -224,20 +224,20 @@ static void dummy_sighandler(int sig)
>  	(void)sig;
>  }
>  
> +static int shm_stat(int idx, void *buf)
> +{
> +	return shmctl(idx, SHM_STAT, buf);
> +}
> +
>  static int get_shm_idx_from_id(int shm_id)
>  {
>  	struct shm_info dummy;
>  	struct shmid_ds dummy_ds;
> -	int max_idx, i;
> +	int max_idx;
>  
>  	max_idx = SAFE_SHMCTL(shm_id, SHM_INFO, (void *)&dummy);
>  
> -	for (i = 0; i <= max_idx; i++) {
> -		if (shmctl(i, SHM_STAT, &dummy_ds) == shm_id)
> -			return i;
> -	}
> -
> -	return -1;
> +	return get_ipc_idx_from_id(shm_id, max_idx, shm_stat, &dummy_ds);
>  }
>  
>  static void setup(void)
> -- 
> 2.55.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

And patch 2/2 for this?

--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp