All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib
@ 2021-08-12  4:38 Joerg Vehlow
  2021-08-12  4:38 ` [LTP] [PATCH v2 2/2] pec: Improve reliability Joerg Vehlow
  2021-08-20 10:59 ` [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib Petr Vorel
  0 siblings, 2 replies; 10+ messages in thread
From: Joerg Vehlow @ 2021-08-12  4:38 UTC (permalink / raw)
  To: ltp

From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

The checkpoint api is even mentioned in shell-test-api.txt,
but with the old library used.
This also fixes the documentation.

memcg_lib.sh must be adapted in the same commit, because it already sets
TST_NEEDS_CHECKPOINTS=1 and had the ipc initialization code. This would
run the ipc initialization code twice.

Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---

Changes to v1:
 - Delete ipc file in _tst_do_exit
 - Replace TCID with TST_ID in ipc filename

 doc/shell-test-api.txt                        |  4 +-
 .../controllers/memcg/functional/memcg_lib.sh |  8 ----
 testcases/lib/tst_test.sh                     | 45 ++++++++++++++++++-
 3 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/doc/shell-test-api.txt b/doc/shell-test-api.txt
index bf297ab07..8d1bab5a1 100644
--- a/doc/shell-test-api.txt
+++ b/doc/shell-test-api.txt
@@ -729,14 +729,14 @@ The shell library provides an implementation of the checkpoint interface
 compatible with the C version. All 'TST_CHECKPOINT_*' functions are available.
 
 In order to initialize checkpoints '$TST_NEEDS_CHECKPOINTS' must be set to '1'
-before the inclusion of 'test.sh':
+before the inclusion of 'tst_test.sh':
 
 [source,sh]
 -------------------------------------------------------------------------------
 #!/bin/sh
 
 TST_NEEDS_CHECKPOINTS=1
-. test.sh
+. tst_test.sh
 -------------------------------------------------------------------------------
 
 Since both the implementations are compatible, it's also possible to start
diff --git a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
index dad66c798..e5fa215c3 100755
--- a/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
+++ b/testcases/kernel/controllers/memcg/functional/memcg_lib.sh
@@ -69,14 +69,6 @@ memcg_setup()
 		tst_brk TCONF "Either kernel does not support Memory Resource Controller or feature not enabled"
 	fi
 
-	# Setup IPC
-	LTP_IPC_PATH="/dev/shm/ltp_${TCID}_$$"
-	LTP_IPC_SIZE=$PAGESIZE
-	ROD_SILENT dd if=/dev/zero of="$LTP_IPC_PATH" bs="$LTP_IPC_SIZE" count=1
-	ROD_SILENT chmod 600 "$LTP_IPC_PATH"
-	export LTP_IPC_PATH
-	# Setup IPC end
-
 	ROD mkdir /dev/memcg
 	ROD mount -t cgroup -omemory memcg /dev/memcg
 
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index c6aa2c487..bc2bbaa8a 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -49,6 +49,10 @@ _tst_do_exit()
 		[ "$TST_TMPDIR_RHOST" = 1 ] && tst_cleanup_rhost
 	fi
 
+	if [ -n "$TST_NEEDS_CHECKPOINTS" -a -f "${LTP_IPC_PATH}" ]; then
+		rm ${LTP_IPC_PATH}
+	fi
+
 	_tst_cleanup_timer
 
 	if [ $TST_FAIL -gt 0 ]; then
@@ -253,6 +257,27 @@ TST_RTNL_CHK()
 	tst_brk TBROK "$@ failed: $output"
 }
 
+TST_CHECKPOINT_WAIT()
+{
+	ROD tst_checkpoint wait 10000 "$1"
+}
+
+TST_CHECKPOINT_WAKE()
+{
+	ROD tst_checkpoint wake 10000 "$1" 1
+}
+
+TST_CHECKPOINT_WAKE2()
+{
+	ROD tst_checkpoint wake 10000 "$1" "$2"
+}
+
+TST_CHECKPOINT_WAKE_AND_WAIT()
+{
+	TST_CHECKPOINT_WAKE "$1"
+	TST_CHECKPOINT_WAIT "$1"
+}
+
 tst_mount()
 {
 	local mnt_opt mnt_err
@@ -558,6 +583,20 @@ tst_set_timeout()
 	_tst_setup_timer
 }
 
+_tst_init_checkpoints()
+{
+	local pagesize
+
+	LTP_IPC_PATH="/dev/shm/ltp_${TST_ID}_$$"
+	pagesize=$(tst_getconf PAGESIZE)
+	if [ $? -ne 0 ]; then
+		tst_brk TBROK "tst_getconf PAGESIZE failed"
+	fi
+	ROD_SILENT dd if=/dev/zero of="$LTP_IPC_PATH" bs="$pagesize" count=1
+	ROD_SILENT chmod 600 "$LTP_IPC_PATH"
+	export LTP_IPC_PATH
+}
+
 tst_run()
 {
 	local _tst_i
@@ -577,7 +616,9 @@ tst_run()
 			IPV6|IPV6_FLAG|IPVER|TEST_DATA|TEST_DATA_IFS);;
 			RETRY_FUNC|RETRY_FN_EXP_BACKOFF|TIMEOUT);;
 			NET_DATAROOT|NET_MAX_PKT|NET_RHOST_RUN_DEBUG|NETLOAD_CLN_NUMBER);;
-			NET_SKIP_VARIABLE_INIT);;
+			NET_SKIP_VARIABLE_INIT|NEEDS_CHECKPOINTS);;
+			CHECKPOINT_WAIT|CHECKPOINT_WAKE);;
+			CHECKPOINT_WAKE2|CHECKPOINT_WAKE_AND_WAIT);;
 			*) tst_res TWARN "Reserved variable TST_$_tst_i used!";;
 			esac
 		done
@@ -652,6 +693,8 @@ tst_run()
 
 	[ -n "$TST_NEEDS_MODULE" ] && tst_require_module "$TST_NEEDS_MODULE"
 
+	[ -n "$TST_NEEDS_CHECKPOINTS" ] && _tst_init_checkpoints
+
 	if [ -n "$TST_SETUP" ]; then
 		if type $TST_SETUP >/dev/null 2>/dev/null; then
 			TST_DO_CLEANUP=1
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [LTP] [PATCH v2 2/2] pec: Improve reliability
  2021-08-12  4:38 [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib Joerg Vehlow
@ 2021-08-12  4:38 ` Joerg Vehlow
  2021-08-20 11:01   ` Petr Vorel
  2021-08-20 10:59 ` [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib Petr Vorel
  1 sibling, 1 reply; 10+ messages in thread
From: Joerg Vehlow @ 2021-08-12  4:38 UTC (permalink / raw)
  To: ltp

From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

This fixes two problems:
1. It was possible, that the event listener was terminated,
before all events were recorded.
The listener now terminates, after receiving the exit event of the generator.
This also gets rid of the 100ms sleep in cn_pec.sh by
using proper synchronization points.
2. Sometimes exit events were generated in the wrong order,
Adding a short delay seems to solve this. While this adds in a new sleep,
it is just a 1 ms for default the execution.

Reported-by: Bogdan Lezhepekov <bogdan.lezhepekov@suse.com>
Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---
 testcases/kernel/connectors/pec/cn_pec.sh     | 36 ++++---
 .../kernel/connectors/pec/event_generator.c   | 97 ++++++++++---------
 .../kernel/connectors/pec/pec_listener.c      | 56 ++++++++++-
 3 files changed, 128 insertions(+), 61 deletions(-)

diff --git a/testcases/kernel/connectors/pec/cn_pec.sh b/testcases/kernel/connectors/pec/cn_pec.sh
index 9b85a5c81..74e94dc5e 100755
--- a/testcases/kernel/connectors/pec/cn_pec.sh
+++ b/testcases/kernel/connectors/pec/cn_pec.sh
@@ -16,12 +16,16 @@ TST_PARSE_ARGS=parse_args
 TST_USAGE=usage
 TST_NEEDS_ROOT=1
 TST_NEEDS_TMPDIR=1
+TST_NEEDS_CHECKPOINTS=1
 TST_TEST_DATA="fork exec exit uid gid"
 
 . tst_test.sh
 
 num_events=10
 
+LISTENER_ID=0
+GENERATOR_ID=1
+
 usage()
 {
 	cat << EOF
@@ -67,27 +71,31 @@ setup()
 test()
 {
 	local event=$2
-	local expected_events lis_rc pid fd_act failed act_nevents exp act
+	local gen_pid list_pid gen_rc lis_rc
+	local expected_events fd_act failed act_nevents exp act
 
 	tst_res TINFO "Testing $2 event (nevents=$num_events)"
 
-	pec_listener >lis_$event.log 2>lis_$event.err &
-	pid=$!
-	# wait for pec_listener to start listening
-	tst_sleep 100ms
+	event_generator -n $num_events -e $event -c $GENERATOR_ID >gen.log &
+	gen_pid=$!
+
+	pec_listener -p $gen_pid -c $LISTENER_ID >lis.log &
+	lis_pid=$!
 
-	ROD event_generator -n $num_events -e $event \>gen_$event.log 2\>gen_$event.err
+	TST_CHECKPOINT_WAIT $LISTENER_ID
+	TST_CHECKPOINT_WAKE $GENERATOR_ID
 
-	kill -s INT $pid 2> /dev/null
-	wait $pid
+	wait $gen_pid
+	gen_rc=$?
+	wait $lis_pid
 	lis_rc=$?
 
-	if [ ! -s gen_$event.log ]; then
-		tst_brk TBROK "failed to generate process events: $(cat gen_$event.err)"
+	if [ $gen_rc -ne 0 ]; then
+		tst_brk TBROK "failed to execute event_generator"
 	fi
 
 	if [ $lis_rc -ne 0 ]; then
-		tst_brk TBROK "failed to execute the listener: $(cat lis_$event.err)"
+		tst_brk TBROK "failed to execute pec_listener"
 	fi
 
 	# The listener writes the same messages as the generator, but it can
@@ -104,7 +112,7 @@ test()
 
 	fd_act=$(free_fd)
 	[ -z "$fd_act" ] && tst_brk TBROK "No free filehandle found"
-	eval "exec ${fd_act}<lis_$event.log"
+	eval "exec ${fd_act}<lis.log"
 
 	failed=0
 	act_nevents=0
@@ -122,7 +130,7 @@ test()
 			tst_res TFAIL "Event was not detected by the event listener: $exp"
 			break
 		fi
-	done <gen_$event.log
+	done <gen.log
 
 	eval "exec ${fd_act}<&-"
 
@@ -134,7 +142,7 @@ test()
 		fi
 	else
 		# TFAIL message is already printed in the loop above
-		cat lis_$event.log
+		cat lis.log
 	fi
 }
 
diff --git a/testcases/kernel/connectors/pec/event_generator.c b/testcases/kernel/connectors/pec/event_generator.c
index 62e341268..722efec96 100644
--- a/testcases/kernel/connectors/pec/event_generator.c
+++ b/testcases/kernel/connectors/pec/event_generator.c
@@ -24,17 +24,16 @@ static struct tst_test test = {
 	.forks_child = 1
 };
 
-#define DEFAULT_EVENT_NUM       1
+static uid_t ltp_uid;
+static gid_t ltp_gid;
+static const char *ltp_user = "nobody";
+static char *prog_name;
 
-unsigned long nr_event = DEFAULT_EVENT_NUM;
+static int checkpoint_id = -1;
+static int nr_event = 1;
 
-uid_t ltp_uid;
-gid_t ltp_gid;
-const char *ltp_user = "nobody";
+static void (*gen_event)(void);
 
-char **exec_argv;
-
-void (*gen_event) (void);
 static void usage(int status) LTP_ATTRIBUTE_NORETURN;
 
 /*
@@ -47,45 +46,29 @@ static void usage(int status)
 	FILE *stream = (status ? stderr : stdout);
 
 	fprintf(stream,
-		"Usage: event_generator -e fork|exit|exec|uid|gid [-n nr_event]\n");
+		"Usage: event_generator -e fork|exit|exec|uid|gid [-n nr_event] [-c checkpoint_id]\n");
 
 	exit(status);
 }
 
 /*
  * Generate exec event.
- *
- * We can't just exec nr_event times, because the current process image
- * will be replaced with the new process image, so we use environment
- * variable as event counters, as it will be inherited after exec.
  */
 static void gen_exec(void)
 {
-	char *val;
 	char buf[10];
-	unsigned long nr_exec;
-
-	/* get the event counter */
-	val = getenv("NR_EXEC");
-	if (!val) {
-		nr_exec = 0;
-		setenv("NR_EXEC", "1", 1);
-	} else {
-		nr_exec = atoi(val);
-		snprintf(buf, 10, "%lu", nr_exec + 1);
-		setenv("NR_EXEC", buf, 1);
-	}
-
-	/* stop generate exec event */
-	if (nr_exec >= nr_event)
-		return;
 
 	/* fflush is needed before exec */
 	printf("exec pid: %d\n", getpid());
 	fflush(stdout);
 
-	/* Note: This expects the full path to self in exec_argv[0]! */
-	SAFE_EXECVP(exec_argv[0], exec_argv);
+	/*
+	 * Decrease number of events to generate.
+	 * Don't pass checkpoint_id here. It is only used for synchronizing with
+	 * the shell script, before the first exec.
+	 */
+	sprintf(buf, "%u", nr_event - 1);
+	SAFE_EXECLP(prog_name, prog_name, "-e", "exec", "-n", buf, NULL);
 }
 
 /*
@@ -135,9 +118,8 @@ static inline void gen_gid(void)
 static void process_options(int argc, char **argv)
 {
 	int c;
-	char *end;
 
-	while ((c = getopt(argc, argv, "e:n:h")) != -1) {
+	while ((c = getopt(argc, argv, "e:n:c:h")) != -1) {
 		switch (c) {
 			/* which event to generate */
 		case 'e':
@@ -158,17 +140,21 @@ static void process_options(int argc, char **argv)
 			break;
 			/* number of event to generate */
 		case 'n':
-			nr_event = strtoul(optarg, &end, 10);
-			if (*end != '\0' || nr_event == 0) {
-				fprintf(stderr, "wrong -n argument!");
-				exit(1);
+			if (tst_parse_int(optarg, &nr_event, 0, INT_MAX)) {
+				fprintf(stderr, "invalid value for nr_event");
+				usage(1);
+			}
+			break;
+		case 'c':
+			if (tst_parse_int(optarg, &checkpoint_id, 0, INT_MAX)) {
+				fprintf(stderr, "invalid value for checkpoint_id");
+				usage(1);
 			}
 			break;
 			/* help */
 		case 'h':
 			usage(0);
 		default:
-			fprintf(stderr, "unknown option!\n");
 			usage(1);
 		}
 	}
@@ -179,11 +165,14 @@ static void process_options(int argc, char **argv)
 	}
 }
 
+void *tst_futexes;
 int main(int argc, char **argv)
 {
-	unsigned long i;
+	int i;
 	struct passwd *ent;
 
+	prog_name = argv[0];
+
 	tst_test = &test;
 
 	process_options(argc, argv);
@@ -196,13 +185,21 @@ int main(int argc, char **argv)
 	ltp_uid = ent->pw_uid;
 	ltp_gid = ent->pw_gid;
 
-	/* special processing for gen_exec, see comments above gen_exec() */
-	if (gen_event == gen_exec) {
-		exec_argv = argv;
-
-		gen_exec();
+	/* ready to generate events */
+	if (checkpoint_id != -1) {
+		tst_reinit();
+		TST_CHECKPOINT_WAIT(checkpoint_id);
+	}
 
-		/* won't reach here */
+	if (gen_event == gen_exec) {
+		/*
+		 * The nr_event events are generated,
+		 * by recursively replacing ourself with
+		 * a fresh copy, decrementing the number of events
+		 * for each execution
+		 */
+		if (nr_event != 0)
+			gen_exec();
 		return 0;
 	}
 
@@ -225,6 +222,14 @@ int main(int argc, char **argv)
 				fprintf(stderr, "Child process did not terminate with 0\n");
 				return 1;
 			}
+			/*
+			 * We need a tiny sleep here, so the kernel can generate
+			 * exit events in the correct order.
+			 * Otherwise it can happen, that exit events are generated
+			 * out-of-order.
+			 */
+			if (gen_event == gen_exit)
+				usleep(100);
 		}
 	}
 
diff --git a/testcases/kernel/connectors/pec/pec_listener.c b/testcases/kernel/connectors/pec/pec_listener.c
index 2707ea8fb..21ae53e87 100644
--- a/testcases/kernel/connectors/pec/pec_listener.c
+++ b/testcases/kernel/connectors/pec/pec_listener.c
@@ -19,6 +19,9 @@
 #include <signal.h>
 #include <linux/types.h>
 #include <linux/netlink.h>
+#include <tst_checkpoint.h>
+#define TST_NO_DEFAULT_MAIN
+#include <tst_test.h>
 
 #ifndef NETLINK_CONNECTOR
 
@@ -52,9 +55,13 @@ static __u32 seq;
 
 static volatile int exit_flag;
 static struct sigaction sigint_action;
+static pid_t terminate_pid;
+static int checkpoint_id = -1;
 
 struct nlmsghdr *nlhdr;
 
+static void usage(int status) LTP_ATTRIBUTE_NORETURN;
+
 /*
  * Handler for signal int. Set exit flag.
  *
@@ -176,6 +183,7 @@ static void process_event(struct nlmsghdr *nlhdr)
 
 	pe = (struct proc_event *)msg->data;
 
+	//printf("TS: %llu\n", pe->timestamp_ns);
 	switch (pe->what) {
 	case PROC_EVENT_NONE:
 		printf("none err: %u\n", pe->event_data.ack.err);
@@ -203,6 +211,9 @@ static void process_event(struct nlmsghdr *nlhdr)
 		       pe->event_data.exit.process_pid,
 		       pe->event_data.exit.exit_code,
 		       pe->event_data.exit.exit_signal);
+			if (terminate_pid
+				&& terminate_pid == pe->event_data.exec.process_pid)
+				exit_flag = 1;
 		break;
 	default:
 		printf("unknown event\n");
@@ -210,7 +221,42 @@ static void process_event(struct nlmsghdr *nlhdr)
 	}
 }
 
-int main(void)
+static void usage(int status)
+{
+	FILE *stream = (status ? stderr : stdout);
+
+	fprintf(stream, "Usage: pec_listener [-p terminate_pid] [-c checkpoint_id]\n");
+
+	exit(status);
+}
+
+static void parse_args(int argc, char * const argv[])
+{
+	int c;
+
+	while ((c = getopt(argc, argv, "p:c:h")) != -1) {
+		switch (c) {
+		case 'p':
+			if (tst_parse_int(optarg, &terminate_pid, 0, INT_MAX)) {
+				fprintf(stderr, "Invalid value for terminate pid\n");
+				exit(1);
+			}
+			break;
+		case 'c':
+			if (tst_parse_int(optarg, &checkpoint_id, 0, INT_MAX)) {
+				fprintf(stderr, "invalid value for checkpoint_id");
+				usage(1);
+			}
+			break;
+		case 'h':
+			usage(0);
+		default:
+			usage(1);
+		}
+	}
+}
+
+int main(int argc, char * const argv[])
 {
 	int ret;
 	int sd;
@@ -218,6 +264,8 @@ int main(void)
 	struct sockaddr_nl src_addr;
 	struct pollfd pfd;
 
+	parse_args(argc, argv);
+
 	sigint_action.sa_flags = SA_RESETHAND;
 	sigint_action.sa_handler = &sigint_handler;
 	sigaction(SIGINT, &sigint_action, NULL);
@@ -257,6 +305,12 @@ int main(void)
 		exit(1);
 	}
 
+	/* ready to receive events */
+	if (checkpoint_id != -1) {
+		tst_reinit();
+		TST_CHECKPOINT_WAKE(0);
+	}
+
 	/* Receive msg from PEC */
 	pfd.fd = sd;
 	pfd.events = POLLIN;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib
  2021-08-12  4:38 [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib Joerg Vehlow
  2021-08-12  4:38 ` [LTP] [PATCH v2 2/2] pec: Improve reliability Joerg Vehlow
@ 2021-08-20 10:59 ` Petr Vorel
  2021-08-23  4:36   ` Joerg Vehlow
  1 sibling, 1 reply; 10+ messages in thread
From: Petr Vorel @ 2021-08-20 10:59 UTC (permalink / raw)
  To: ltp

Hi Joerg,

LGTM, good idea.

But I'd rename $LTP_IPC_PATH to $TST_IPC_PATH to follow conventions to new API
(There are also some LTP_* definitions in the legacy C API which were renamed as
TST_* in the new API).


> +	if [ -n "$TST_NEEDS_CHECKPOINTS" -a -f "${LTP_IPC_PATH}" ]; then
nit (remove unnecessary {}):
    if [ -n "$TST_NEEDS_CHECKPOINTS" -a -f "$TST_IPC_PATH" ]; then
> +		rm ${LTP_IPC_PATH}
and here:
		rm $TST_IPC_PATH

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [LTP] [PATCH v2 2/2] pec: Improve reliability
  2021-08-12  4:38 ` [LTP] [PATCH v2 2/2] pec: Improve reliability Joerg Vehlow
@ 2021-08-20 11:01   ` Petr Vorel
  2021-08-23  4:31     ` Joerg Vehlow
  0 siblings, 1 reply; 10+ messages in thread
From: Petr Vorel @ 2021-08-20 11:01 UTC (permalink / raw)
  To: ltp

Hi Joerg,

I suppose this one is superseded by adding volatile by Martin's fix in
b1e7776bf ("connectors/pec_listener: Make exit_flag volatile")

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [LTP] [PATCH v2 2/2] pec: Improve reliability
  2021-08-20 11:01   ` Petr Vorel
@ 2021-08-23  4:31     ` Joerg Vehlow
  2021-08-24  6:29       ` Petr Vorel
  0 siblings, 1 reply; 10+ messages in thread
From: Joerg Vehlow @ 2021-08-23  4:31 UTC (permalink / raw)
  To: ltp

Hi Petr,

On 8/20/2021 1:01 PM, Petr Vorel wrote:
> Hi Joerg,
>
> I suppose this one is superseded by adding volatile by Martin's fix in
> b1e7776bf ("connectors/pec_listener: Make exit_flag volatile")
No, not at all.
The first issue is a timing issue between the event generator -> kernel 
-> pec_listener,
that cannot be fixed by a volatile, only by correct synchronization.
The second issue is also unrelated.

Martin's fix is only for the rare case, where the exit_flag could be 
optimized out by the compiler,
which would have made the event_listener run forever instead of terminating.

Joerg

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib
  2021-08-20 10:59 ` [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib Petr Vorel
@ 2021-08-23  4:36   ` Joerg Vehlow
  2021-08-24  5:47     ` Petr Vorel
  0 siblings, 1 reply; 10+ messages in thread
From: Joerg Vehlow @ 2021-08-23  4:36 UTC (permalink / raw)
  To: ltp

Hi Petr,

On 8/20/2021 12:59 PM, Petr Vorel wrote:
> Hi Joerg,
>
> LGTM, good idea.
>
> But I'd rename $LTP_IPC_PATH to $TST_IPC_PATH to follow conventions to new API
> (There are also some LTP_* definitions in the legacy C API which were renamed as
> TST_* in the new API).
This would also require changing the C API, because the environment 
variable is used for communicating with it.
I we want this, I guess it could be done in another changeset.
>
>
>> +	if [ -n "$TST_NEEDS_CHECKPOINTS" -a -f "${LTP_IPC_PATH}" ]; then
> nit (remove unnecessary {}):
Could be changed while merging.
>      if [ -n "$TST_NEEDS_CHECKPOINTS" -a -f "$TST_IPC_PATH" ]; then
>> +		rm ${LTP_IPC_PATH}
> and here:
> 		rm $TST_IPC_PATH
>


Joerg

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib
  2021-08-23  4:36   ` Joerg Vehlow
@ 2021-08-24  5:47     ` Petr Vorel
  0 siblings, 0 replies; 10+ messages in thread
From: Petr Vorel @ 2021-08-24  5:47 UTC (permalink / raw)
  To: ltp

Hi Joerg,

> Hi Petr,

> On 8/20/2021 12:59 PM, Petr Vorel wrote:
> > Hi Joerg,

> > LGTM, good idea.

> > But I'd rename $LTP_IPC_PATH to $TST_IPC_PATH to follow conventions to new API
> > (There are also some LTP_* definitions in the legacy C API which were renamed as
> > TST_* in the new API).
> This would also require changing the C API, because the environment variable
> is used for communicating with it.
> I we want this, I guess it could be done in another changeset.
Ah, you right.


> > > +	if [ -n "$TST_NEEDS_CHECKPOINTS" -a -f "${LTP_IPC_PATH}" ]; then
> > nit (remove unnecessary {}):
> Could be changed while merging.
Sure.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [LTP] [PATCH v2 2/2] pec: Improve reliability
  2021-08-23  4:31     ` Joerg Vehlow
@ 2021-08-24  6:29       ` Petr Vorel
  2021-08-24  7:27         ` Joerg Vehlow
  0 siblings, 1 reply; 10+ messages in thread
From: Petr Vorel @ 2021-08-24  6:29 UTC (permalink / raw)
  To: ltp

Hi Joerg,

FYI, this patch breaks build:

make[1]: Leaving directory 'ltp.git/lib'
/usr/lib64/gcc/x86_64-suse-linux/11/../../../../x86_64-suse-linux/bin/ld: ../../../../lib/libltp.a(tst_checkpoint.o):ltp.git/lib/tst_checkpoint.c:36: multiple definition of `tst_futexes'; /tmp/ccITQEiq.o:ltp.git/testcases/kernel/connectors/pec/event_generator.c:168: first defined here
collect2: error: ld returned 1 exit status
make: *** [../../../../include/mk/rules.mk:37: event_generator] Error 1

https://github.com/pevik/ltp/actions/runs/1161400737

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [LTP] [PATCH v2 2/2] pec: Improve reliability
  2021-08-24  6:29       ` Petr Vorel
@ 2021-08-24  7:27         ` Joerg Vehlow
  2021-08-24 10:47           ` Petr Vorel
  0 siblings, 1 reply; 10+ messages in thread
From: Joerg Vehlow @ 2021-08-24  7:27 UTC (permalink / raw)
  To: ltp

Hi Petr,

On 8/24/2021 8:29 AM, Petr Vorel wrote:
> Hi Joerg,
>
> FYI, this patch breaks build:
>
> make[1]: Leaving directory 'ltp.git/lib'
> /usr/lib64/gcc/x86_64-suse-linux/11/../../../../x86_64-suse-linux/bin/ld: ../../../../lib/libltp.a(tst_checkpoint.o):ltp.git/lib/tst_checkpoint.c:36: multiple definition of `tst_futexes'; /tmp/ccITQEiq.o:ltp.git/testcases/kernel/connectors/pec/event_generator.c:168: first defined here
> collect2: error: ld returned 1 exit status
> make: *** [../../../../include/mk/rules.mk:37: event_generator] Error 1
Strange that it builds on my system... The symbol is indeed duplicated 
and a leftover from experimenting.
The declaration in event_generator.c can be removed.
Should I post a v3?

Joerg

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [LTP] [PATCH v2 2/2] pec: Improve reliability
  2021-08-24  7:27         ` Joerg Vehlow
@ 2021-08-24 10:47           ` Petr Vorel
  0 siblings, 0 replies; 10+ messages in thread
From: Petr Vorel @ 2021-08-24 10:47 UTC (permalink / raw)
  To: ltp

Hi Joerg,

> > FYI, this patch breaks build:

> > make[1]: Leaving directory 'ltp.git/lib'
> > /usr/lib64/gcc/x86_64-suse-linux/11/../../../../x86_64-suse-linux/bin/ld: ../../../../lib/libltp.a(tst_checkpoint.o):ltp.git/lib/tst_checkpoint.c:36: multiple definition of `tst_futexes'; /tmp/ccITQEiq.o:ltp.git/testcases/kernel/connectors/pec/event_generator.c:168: first defined here
> > collect2: error: ld returned 1 exit status
> > make: *** [../../../../include/mk/rules.mk:37: event_generator] Error 1
> Strange that it builds on my system... The symbol is indeed duplicated and a
> leftover from experimenting.
> The declaration in event_generator.c can be removed.
> Should I post a v3?
Yes please.

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-08-24 10:47 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-12  4:38 [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib Joerg Vehlow
2021-08-12  4:38 ` [LTP] [PATCH v2 2/2] pec: Improve reliability Joerg Vehlow
2021-08-20 11:01   ` Petr Vorel
2021-08-23  4:31     ` Joerg Vehlow
2021-08-24  6:29       ` Petr Vorel
2021-08-24  7:27         ` Joerg Vehlow
2021-08-24 10:47           ` Petr Vorel
2021-08-20 10:59 ` [LTP] [PATCH v2 1/2] shell: Add checkpoints api for new lib Petr Vorel
2021-08-23  4:36   ` Joerg Vehlow
2021-08-24  5:47     ` Petr Vorel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.