[PATCH v2 0/3] rt-tests: minor fixes

[PATCH v2 0/3] rt-tests: minor fixes

[Luiz Capitulino]

I've found these little things while reading the code and
building rt-tests. They are all minor, but I'm sending
them anyways.

o v2

 - rebase on top of current master
 - drop already merged patch

Luiz Capitulino (3):
  rt-migrate-test: fix return code
  don't use exit(-1) for failures
  cyclictest: initialize 'stop' early

 src/cyclictest/cyclictest.c           | 4 +++-
 src/pi_tests/pi_stress.c              | 2 +-
 src/pi_tests/pip_stress.c             | 6 +++---
 src/rt-migrate-test/rt-migrate-test.c | 9 ++++-----
 src/signaltest/signaltest.c           | 2 +-
 5 files changed, 12 insertions(+), 11 deletions(-)

-- 
2.1.0

[PATCH 1/3] rt-migrate-test: fix return code

[Luiz Capitulino]

Change both return codes for the stop == true case:

 * For failures, use exit(1) as exit(-1) is wrong
   (it actually becomes 255 in the shell)

 * For success, use exit(2) instead of exit(1) as
   exit(1) is usually used for errors

This should preserve the requirement of allowing
shell script while loops to break when Ctrl-C is hit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 src/rt-migrate-test/rt-migrate-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
index d7b68dd..1362404 100644
--- a/src/rt-migrate-test/rt-migrate-test.c
+++ b/src/rt-migrate-test/rt-migrate-test.c
@@ -599,9 +599,9 @@ int main (int argc, char **argv)
 		 * loop know to break.
 		 */
 		if (check < 0)
-			exit(-1);
-		else
 			exit(1);
+		else
+			exit(2);
 	}
 	if (check < 0)
 		exit(-1);
-- 
2.1.0

[PATCH 2/3] don't use exit(-1) for failures

[Luiz Capitulino]

The kernel uses only 8 bits of the status as a return
code, so this actually becomes 255 in the shell.

In any case, the most widely convension is exit(1)
for failures, so let's be consistent.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 src/pi_tests/pi_stress.c              | 2 +-
 src/pi_tests/pip_stress.c             | 6 +++---
 src/rt-migrate-test/rt-migrate-test.c | 5 ++---
 src/signaltest/signaltest.c           | 2 +-
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/src/pi_tests/pi_stress.c b/src/pi_tests/pi_stress.c
index a02f706..36b64ca 100644
--- a/src/pi_tests/pi_stress.c
+++ b/src/pi_tests/pi_stress.c
@@ -634,7 +634,7 @@ int verify_cpu(int cpu)
 	if (status == -1) {
 		err = errno;
 		fprintf(stderr, "sched_getaffinity %s\n", strerror(err));
-		exit(-1);
+		exit(1);
 	}
 
 	if (CPU_ISSET(cpu, &mask))
diff --git a/src/pi_tests/pip_stress.c b/src/pi_tests/pip_stress.c
index a0477cc..e6cf0ce 100644
--- a/src/pi_tests/pip_stress.c
+++ b/src/pi_tests/pip_stress.c
@@ -95,7 +95,7 @@ int main(void)
 	*minimum_priority = sched_get_priority_min(policy);
 
 	if (check_privs())
-		exit(-1);
+		exit(1);
 
 	mptr = mmap_page();	/* Get a page of shared memory */
 	resource = (pthread_mutex_t*)mptr;	/* point our lock to it */
@@ -138,7 +138,7 @@ int main(void)
 		pid2 = fork();		/* parent code */
 		if (pid2 == -1) {
 			perror("fork: ");
-			exit(-1);
+			exit(1);
 		} else if (pid2 != 0) {		/* parent code */
 			high(pid2);
 		} else {			/* child code */
@@ -231,7 +231,7 @@ void *mmap_page(void)
 	mptr = mmap(NULL, pgsize, PROTRW, MMAP_FLAGS, 0, 0);
 	if (mptr == MAP_FAILED) {
 		perror("In function mmap_page - mmap");
-		exit(-1);
+		exit(1);
 	}
 
 	return mptr;
diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
index 1362404..a00e86f 100644
--- a/src/rt-migrate-test/rt-migrate-test.c
+++ b/src/rt-migrate-test/rt-migrate-test.c
@@ -138,7 +138,7 @@ static void perr(char *fmt, ...)
 
 	perror(buffer);
 	fflush(stderr);
-	exit(-1);
+	exit(1);
 }
 
 static void print_progress_bar(int percent)
@@ -226,7 +226,6 @@ static void parse_options (int argc, char *argv[])
 			break;
 		}
 	}
-
 }
 
 static unsigned long long get_time(void)
@@ -604,7 +603,7 @@ int main (int argc, char **argv)
 			exit(2);
 	}
 	if (check < 0)
-		exit(-1);
+		exit(1);
 	else
 		exit(0);
 
diff --git a/src/signaltest/signaltest.c b/src/signaltest/signaltest.c
index b80969b..59f979e 100644
--- a/src/signaltest/signaltest.c
+++ b/src/signaltest/signaltest.c
@@ -323,7 +323,7 @@ int main(int argc, char **argv)
 	process_options(argc, argv);
 
 	if (check_privs())
-		exit(-1);
+		exit(1);
 
 	/* lock all memory (prevent paging) */
 	if (lockall)
-- 
2.1.0

[PATCH 3/3] cyclictest: initialize 'stop' early

[Luiz Capitulino]

For some modes like MODE_CLOCK_NANOSLEEP, the clock
is already ticking when 'stop' is initialized. This
shouldn't matter, as this memset() should very fast
(I'd guess a few dozens or hundrends nanoseconds at
most?), but I also think there's no reason to do it
this late. So, initialize it along with everything
else.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 src/cyclictest/cyclictest.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index f5a67dc..202939f 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -829,6 +829,8 @@ static void *timerthread(void *param)
 		tspec.it_interval = interval;
 	}
 
+	memset(&stop, 0, sizeof(stop));
+
 	memset(&schedp, 0, sizeof(schedp));
 	schedp.sched_priority = par->prio;
 	if (setscheduler(0, par->policy, &schedp))
@@ -868,10 +870,10 @@ static void *timerthread(void *param)
 	tsnorm(&next);
 
 	if (duration) {
-		memset(&stop, 0, sizeof(stop)); /* grrr */
 		stop = now;
 		stop.tv_sec += duration;
 	}
+
 	if (par->mode == MODE_CYCLIC) {
 		if (par->timermode == TIMER_ABSTIME)
 			tspec.it_value = next;
-- 
2.1.0

Re: [PATCH 2/3] don't use exit(-1) for failures

[John Kacur]

On Thu, 17 Mar 2016, Luiz Capitulino wrote:

> The kernel uses only 8 bits of the status as a return
> code, so this actually becomes 255 in the shell.
> 
> In any case, the most widely convension is exit(1)
> for failures, so let's be consistent.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  src/pi_tests/pi_stress.c              | 2 +-
>  src/pi_tests/pip_stress.c             | 6 +++---
>  src/rt-migrate-test/rt-migrate-test.c | 5 ++---
>  src/signaltest/signaltest.c           | 2 +-
>  4 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/src/pi_tests/pi_stress.c b/src/pi_tests/pi_stress.c
> index a02f706..36b64ca 100644
> --- a/src/pi_tests/pi_stress.c
> +++ b/src/pi_tests/pi_stress.c
> @@ -634,7 +634,7 @@ int verify_cpu(int cpu)
>  	if (status == -1) {
>  		err = errno;
>  		fprintf(stderr, "sched_getaffinity %s\n", strerror(err));
> -		exit(-1);
> +		exit(1);
>  	}
>  
>  	if (CPU_ISSET(cpu, &mask))
> diff --git a/src/pi_tests/pip_stress.c b/src/pi_tests/pip_stress.c
> index a0477cc..e6cf0ce 100644
> --- a/src/pi_tests/pip_stress.c
> +++ b/src/pi_tests/pip_stress.c
> @@ -95,7 +95,7 @@ int main(void)
>  	*minimum_priority = sched_get_priority_min(policy);
>  
>  	if (check_privs())
> -		exit(-1);
> +		exit(1);
>  
>  	mptr = mmap_page();	/* Get a page of shared memory */
>  	resource = (pthread_mutex_t*)mptr;	/* point our lock to it */
> @@ -138,7 +138,7 @@ int main(void)
>  		pid2 = fork();		/* parent code */
>  		if (pid2 == -1) {
>  			perror("fork: ");
> -			exit(-1);
> +			exit(1);
>  		} else if (pid2 != 0) {		/* parent code */
>  			high(pid2);
>  		} else {			/* child code */
> @@ -231,7 +231,7 @@ void *mmap_page(void)
>  	mptr = mmap(NULL, pgsize, PROTRW, MMAP_FLAGS, 0, 0);
>  	if (mptr == MAP_FAILED) {
>  		perror("In function mmap_page - mmap");
> -		exit(-1);
> +		exit(1);
>  	}
>  
>  	return mptr;
> diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
> index 1362404..a00e86f 100644
> --- a/src/rt-migrate-test/rt-migrate-test.c
> +++ b/src/rt-migrate-test/rt-migrate-test.c
> @@ -138,7 +138,7 @@ static void perr(char *fmt, ...)
>  
>  	perror(buffer);
>  	fflush(stderr);
> -	exit(-1);
> +	exit(1);
>  }
>  
>  static void print_progress_bar(int percent)
> @@ -226,7 +226,6 @@ static void parse_options (int argc, char *argv[])
>  			break;
>  		}
>  	}
> -
>  }
>  
>  static unsigned long long get_time(void)
> @@ -604,7 +603,7 @@ int main (int argc, char **argv)
>  			exit(2);
>  	}
>  	if (check < 0)
> -		exit(-1);
> +		exit(1);
>  	else
>  		exit(0);
>  
> diff --git a/src/signaltest/signaltest.c b/src/signaltest/signaltest.c
> index b80969b..59f979e 100644
> --- a/src/signaltest/signaltest.c
> +++ b/src/signaltest/signaltest.c
> @@ -323,7 +323,7 @@ int main(int argc, char **argv)
>  	process_options(argc, argv);
>  
>  	if (check_privs())
> -		exit(-1);
> +		exit(1);
>  
>  	/* lock all memory (prevent paging) */
>  	if (lockall)
> -- 
> 2.1.0
> 


Signed-off-by: John Kacur <jkacur@gmail.com>

However it didn't apply cleanly, once again, please work with
Repo: git://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git
Branch: devel/v0.97

Re: [PATCH 1/3] rt-migrate-test: fix return code

[John Kacur]

On Thu, 17 Mar 2016, Luiz Capitulino wrote:

> Change both return codes for the stop == true case:
> 
>  * For failures, use exit(1) as exit(-1) is wrong
>    (it actually becomes 255 in the shell)
> 
>  * For success, use exit(2) instead of exit(1) as
>    exit(1) is usually used for errors
> 
> This should preserve the requirement of allowing
> shell script while loops to break when Ctrl-C is hit.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  src/rt-migrate-test/rt-migrate-test.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
> index d7b68dd..1362404 100644
> --- a/src/rt-migrate-test/rt-migrate-test.c
> +++ b/src/rt-migrate-test/rt-migrate-test.c
> @@ -599,9 +599,9 @@ int main (int argc, char **argv)
>  		 * loop know to break.
>  		 */
>  		if (check < 0)
> -			exit(-1);
> -		else
>  			exit(1);
> +		else
> +			exit(2);
>  	}
>  	if (check < 0)
>  		exit(-1);
> -- 
> 2.1.0
> 
> --

NAK - I've already told you this is wrong
0 is the normal value for exit_success, not 2
you can change the failure from -1 to 1 if you wish

John

Re: [PATCH 2/3] don't use exit(-1) for failures

[Luiz Capitulino]

On Tue, 22 Mar 2016 15:21:18 +0100 (CET)
John Kacur <jkacur@redhat.com> wrote:

> 
> 
> On Thu, 17 Mar 2016, Luiz Capitulino wrote:
> 
> > The kernel uses only 8 bits of the status as a return
> > code, so this actually becomes 255 in the shell.
> > 
> > In any case, the most widely convension is exit(1)
> > for failures, so let's be consistent.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  src/pi_tests/pi_stress.c              | 2 +-
> >  src/pi_tests/pip_stress.c             | 6 +++---
> >  src/rt-migrate-test/rt-migrate-test.c | 5 ++---
> >  src/signaltest/signaltest.c           | 2 +-
> >  4 files changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/pi_tests/pi_stress.c b/src/pi_tests/pi_stress.c
> > index a02f706..36b64ca 100644
> > --- a/src/pi_tests/pi_stress.c
> > +++ b/src/pi_tests/pi_stress.c
> > @@ -634,7 +634,7 @@ int verify_cpu(int cpu)
> >  	if (status == -1) {
> >  		err = errno;
> >  		fprintf(stderr, "sched_getaffinity %s\n", strerror(err));
> > -		exit(-1);
> > +		exit(1);
> >  	}
> >  
> >  	if (CPU_ISSET(cpu, &mask))
> > diff --git a/src/pi_tests/pip_stress.c b/src/pi_tests/pip_stress.c
> > index a0477cc..e6cf0ce 100644
> > --- a/src/pi_tests/pip_stress.c
> > +++ b/src/pi_tests/pip_stress.c
> > @@ -95,7 +95,7 @@ int main(void)
> >  	*minimum_priority = sched_get_priority_min(policy);
> >  
> >  	if (check_privs())
> > -		exit(-1);
> > +		exit(1);
> >  
> >  	mptr = mmap_page();	/* Get a page of shared memory */
> >  	resource = (pthread_mutex_t*)mptr;	/* point our lock to it */
> > @@ -138,7 +138,7 @@ int main(void)
> >  		pid2 = fork();		/* parent code */
> >  		if (pid2 == -1) {
> >  			perror("fork: ");
> > -			exit(-1);
> > +			exit(1);
> >  		} else if (pid2 != 0) {		/* parent code */
> >  			high(pid2);
> >  		} else {			/* child code */
> > @@ -231,7 +231,7 @@ void *mmap_page(void)
> >  	mptr = mmap(NULL, pgsize, PROTRW, MMAP_FLAGS, 0, 0);
> >  	if (mptr == MAP_FAILED) {
> >  		perror("In function mmap_page - mmap");
> > -		exit(-1);
> > +		exit(1);
> >  	}
> >  
> >  	return mptr;
> > diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
> > index 1362404..a00e86f 100644
> > --- a/src/rt-migrate-test/rt-migrate-test.c
> > +++ b/src/rt-migrate-test/rt-migrate-test.c
> > @@ -138,7 +138,7 @@ static void perr(char *fmt, ...)
> >  
> >  	perror(buffer);
> >  	fflush(stderr);
> > -	exit(-1);
> > +	exit(1);
> >  }
> >  
> >  static void print_progress_bar(int percent)
> > @@ -226,7 +226,6 @@ static void parse_options (int argc, char *argv[])
> >  			break;
> >  		}
> >  	}
> > -
> >  }
> >  
> >  static unsigned long long get_time(void)
> > @@ -604,7 +603,7 @@ int main (int argc, char **argv)
> >  			exit(2);
> >  	}
> >  	if (check < 0)
> > -		exit(-1);
> > +		exit(1);
> >  	else
> >  		exit(0);
> >  
> > diff --git a/src/signaltest/signaltest.c b/src/signaltest/signaltest.c
> > index b80969b..59f979e 100644
> > --- a/src/signaltest/signaltest.c
> > +++ b/src/signaltest/signaltest.c
> > @@ -323,7 +323,7 @@ int main(int argc, char **argv)
> >  	process_options(argc, argv);
> >  
> >  	if (check_privs())
> > -		exit(-1);
> > +		exit(1);
> >  
> >  	/* lock all memory (prevent paging) */
> >  	if (lockall)
> > -- 
> > 2.1.0
> > 
> 
> 
> Signed-off-by: John Kacur <jkacur@gmail.com>
> 
> However it didn't apply cleanly, once again, please work with
> Repo: git://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git
> Branch: devel/v0.97

I've used the correct repo, but I've based my series on top of
master. I'll rebase once again...

Re: [PATCH 1/3] rt-migrate-test: fix return code

[Luiz Capitulino]

On Tue, 22 Mar 2016 15:22:57 +0100 (CET)
John Kacur <jkacur@redhat.com> wrote:

> 
> 
> On Thu, 17 Mar 2016, Luiz Capitulino wrote:
> 
> > Change both return codes for the stop == true case:
> > 
> >  * For failures, use exit(1) as exit(-1) is wrong
> >    (it actually becomes 255 in the shell)
> > 
> >  * For success, use exit(2) instead of exit(1) as
> >    exit(1) is usually used for errors
> > 
> > This should preserve the requirement of allowing
> > shell script while loops to break when Ctrl-C is hit.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  src/rt-migrate-test/rt-migrate-test.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
> > index d7b68dd..1362404 100644
> > --- a/src/rt-migrate-test/rt-migrate-test.c
> > +++ b/src/rt-migrate-test/rt-migrate-test.c
> > @@ -599,9 +599,9 @@ int main (int argc, char **argv)
> >  		 * loop know to break.
> >  		 */
> >  		if (check < 0)
> > -			exit(-1);
> > -		else
> >  			exit(1);
> > +		else
> > +			exit(2);
> >  	}
> >  	if (check < 0)
> >  		exit(-1);
> > -- 
> > 2.1.0
> > 
> > --
> 
> NAK - I've already told you this is wrong

Right and wrong can be subjective concepts :)

> 0 is the normal value for exit_success, not 2
> you can change the failure from -1 to 1 if you wish

rt-migrate-test uses a different protocol as documented
in the code. If we change success to 0, we'll break this
protocol. Does it matter? I don't know, but I chose to
keep it.

Now, if this is a complicated matter, we can just skip this
patch.

Re: [PATCH 3/3] cyclictest: initialize 'stop' early

[John Kacur]

On Thu, 17 Mar 2016, Luiz Capitulino wrote:

> For some modes like MODE_CLOCK_NANOSLEEP, the clock
> is already ticking when 'stop' is initialized. This
> shouldn't matter, as this memset() should very fast
> (I'd guess a few dozens or hundrends nanoseconds at
> most?), but I also think there's no reason to do it
> this late. So, initialize it along with everything
> else.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  src/cyclictest/cyclictest.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index f5a67dc..202939f 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -829,6 +829,8 @@ static void *timerthread(void *param)
>  		tspec.it_interval = interval;
>  	}
>  
> +	memset(&stop, 0, sizeof(stop));
> +
>  	memset(&schedp, 0, sizeof(schedp));
>  	schedp.sched_priority = par->prio;
>  	if (setscheduler(0, par->policy, &schedp))
> @@ -868,10 +870,10 @@ static void *timerthread(void *param)
>  	tsnorm(&next);
>  
>  	if (duration) {
> -		memset(&stop, 0, sizeof(stop)); /* grrr */
>  		stop = now;
>  		stop.tv_sec += duration;
>  	}
> +
>  	if (par->mode == MODE_CYCLIC) {
>  		if (par->timermode == TIMER_ABSTIME)
>  			tspec.it_value = next;
> -- 
> 2.1.0
> 
> --

Not sure about this one. It might be okay, but it certainly
makes the code messier. Is this just a theorectical possibility
or do you have some evidence that this is a problem?

I think I'll skip it for now unless you can show that it's a
real problem.

Thanks

John

Re: [PATCH 1/3] rt-migrate-test: fix return code

[John Kacur]

On Tue, 22 Mar 2016, Luiz Capitulino wrote:

> On Tue, 22 Mar 2016 15:22:57 +0100 (CET)
> John Kacur <jkacur@redhat.com> wrote:
> 
> > 
> > 
> > On Thu, 17 Mar 2016, Luiz Capitulino wrote:
> > 
> > > Change both return codes for the stop == true case:
> > > 
> > >  * For failures, use exit(1) as exit(-1) is wrong
> > >    (it actually becomes 255 in the shell)
> > > 
> > >  * For success, use exit(2) instead of exit(1) as
> > >    exit(1) is usually used for errors
> > > 
> > > This should preserve the requirement of allowing
> > > shell script while loops to break when Ctrl-C is hit.
> > > 
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > ---
> > >  src/rt-migrate-test/rt-migrate-test.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
> > > index d7b68dd..1362404 100644
> > > --- a/src/rt-migrate-test/rt-migrate-test.c
> > > +++ b/src/rt-migrate-test/rt-migrate-test.c
> > > @@ -599,9 +599,9 @@ int main (int argc, char **argv)
> > >  		 * loop know to break.
> > >  		 */
> > >  		if (check < 0)
> > > -			exit(-1);
> > > -		else
> > >  			exit(1);
> > > +		else
> > > +			exit(2);
> > >  	}
> > >  	if (check < 0)
> > >  		exit(-1);
> > > -- 
> > > 2.1.0
> > > 
> > > --
> > 
> > NAK - I've already told you this is wrong
> 
> Right and wrong can be subjective concepts :)
> 
> > 0 is the normal value for exit_success, not 2
> > you can change the failure from -1 to 1 if you wish
> 
> rt-migrate-test uses a different protocol as documented
> in the code. If we change success to 0, we'll break this
> protocol. Does it matter? I don't know, but I chose to
> keep it.
> 
> Now, if this is a complicated matter, we can just skip this
> patch.
> --

Really not sure what you're talking about
update your git repo and read the code again.

John

Re: [PATCH 3/3] cyclictest: initialize 'stop' early

[Luiz Capitulino]

On Tue, 22 Mar 2016 15:55:10 +0100 (CET)
John Kacur <jkacur@redhat.com> wrote:

> 
> 
> On Thu, 17 Mar 2016, Luiz Capitulino wrote:
> 
> > For some modes like MODE_CLOCK_NANOSLEEP, the clock
> > is already ticking when 'stop' is initialized. This
> > shouldn't matter, as this memset() should very fast
> > (I'd guess a few dozens or hundrends nanoseconds at
> > most?), but I also think there's no reason to do it
> > this late. So, initialize it along with everything
> > else.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  src/cyclictest/cyclictest.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> > index f5a67dc..202939f 100644
> > --- a/src/cyclictest/cyclictest.c
> > +++ b/src/cyclictest/cyclictest.c
> > @@ -829,6 +829,8 @@ static void *timerthread(void *param)
> >  		tspec.it_interval = interval;
> >  	}
> >  
> > +	memset(&stop, 0, sizeof(stop));
> > +
> >  	memset(&schedp, 0, sizeof(schedp));
> >  	schedp.sched_priority = par->prio;
> >  	if (setscheduler(0, par->policy, &schedp))
> > @@ -868,10 +870,10 @@ static void *timerthread(void *param)
> >  	tsnorm(&next);
> >  
> >  	if (duration) {
> > -		memset(&stop, 0, sizeof(stop)); /* grrr */
> >  		stop = now;
> >  		stop.tv_sec += duration;
> >  	}
> > +
> >  	if (par->mode == MODE_CYCLIC) {
> >  		if (par->timermode == TIMER_ABSTIME)
> >  			tspec.it_value = next;
> > -- 
> > 2.1.0
> > 
> > --
> 
> Not sure about this one. It might be okay, but it certainly
> makes the code messier. Is this just a theorectical possibility
> or do you have some evidence that this is a problem?
> 
> I think I'll skip it for now unless you can show that it's a
> real problem.

It's not a problem. Personally, I think it's a good practice
to do all the initialization before the clock starts ticking,
but that doesn't mean that we have a problem with that memset.

Re: [PATCH 1/3] rt-migrate-test: fix return code

[John Kacur]

On Tue, 22 Mar 2016, Luiz Capitulino wrote:

> On Tue, 22 Mar 2016 15:22:57 +0100 (CET)
> John Kacur <jkacur@redhat.com> wrote:
> 
> > 
> > 
> > On Thu, 17 Mar 2016, Luiz Capitulino wrote:
> > 
> > > Change both return codes for the stop == true case:
> > > 
> > >  * For failures, use exit(1) as exit(-1) is wrong
> > >    (it actually becomes 255 in the shell)
> > > 
> > >  * For success, use exit(2) instead of exit(1) as
> > >    exit(1) is usually used for errors
> > > 
> > > This should preserve the requirement of allowing
> > > shell script while loops to break when Ctrl-C is hit.
> > > 
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > ---
> > >  src/rt-migrate-test/rt-migrate-test.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
> > > index d7b68dd..1362404 100644
> > > --- a/src/rt-migrate-test/rt-migrate-test.c
> > > +++ b/src/rt-migrate-test/rt-migrate-test.c
> > > @@ -599,9 +599,9 @@ int main (int argc, char **argv)
> > >  		 * loop know to break.
> > >  		 */
> > >  		if (check < 0)
> > > -			exit(-1);
> > > -		else
> > >  			exit(1);
> > > +		else
> > > +			exit(2);
> > >  	}
> > >  	if (check < 0)
> > >  		exit(-1);
> > > -- 
> > > 2.1.0
> > > 
> > > --
> > 
> > NAK - I've already told you this is wrong
> 
> Right and wrong can be subjective concepts :)
> 

or they can be defined by POSIX> 

Re: [PATCH 1/3] rt-migrate-test: fix return code

[Luiz Capitulino]

On Tue, 22 Mar 2016 15:59:50 +0100 (CET)
John Kacur <jkacur@redhat.com> wrote:

> 
> 
> On Tue, 22 Mar 2016, Luiz Capitulino wrote:
> 
> > On Tue, 22 Mar 2016 15:22:57 +0100 (CET)
> > John Kacur <jkacur@redhat.com> wrote:
> > 
> > > 
> > > 
> > > On Thu, 17 Mar 2016, Luiz Capitulino wrote:
> > > 
> > > > Change both return codes for the stop == true case:
> > > > 
> > > >  * For failures, use exit(1) as exit(-1) is wrong
> > > >    (it actually becomes 255 in the shell)
> > > > 
> > > >  * For success, use exit(2) instead of exit(1) as
> > > >    exit(1) is usually used for errors
> > > > 
> > > > This should preserve the requirement of allowing
> > > > shell script while loops to break when Ctrl-C is hit.
> > > > 
> > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > > ---
> > > >  src/rt-migrate-test/rt-migrate-test.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
> > > > index d7b68dd..1362404 100644
> > > > --- a/src/rt-migrate-test/rt-migrate-test.c
> > > > +++ b/src/rt-migrate-test/rt-migrate-test.c
> > > > @@ -599,9 +599,9 @@ int main (int argc, char **argv)
> > > >  		 * loop know to break.
> > > >  		 */
> > > >  		if (check < 0)
> > > > -			exit(-1);
> > > > -		else
> > > >  			exit(1);
> > > > +		else
> > > > +			exit(2);
> > > >  	}
> > > >  	if (check < 0)
> > > >  		exit(-1);
> > > > -- 
> > > > 2.1.0
> > > > 
> > > > --
> > > 
> > > NAK - I've already told you this is wrong
> > 
> > Right and wrong can be subjective concepts :)
> > 
> > > 0 is the normal value for exit_success, not 2
> > > you can change the failure from -1 to 1 if you wish
> > 
> > rt-migrate-test uses a different protocol as documented
> > in the code. If we change success to 0, we'll break this
> > protocol. Does it matter? I don't know, but I chose to
> > keep it.
> > 
> > Now, if this is a complicated matter, we can just skip this
> > patch.
> > --
> 
> Really not sure what you're talking about
> update your git repo and read the code again.

I'm looking at a638701a1899. Unless I grabbed the wrong branch
again, the code looks the same.

This is the block I'm referring to:

    if (stop) {
        /*
         * We use this test in bash while loops
         * So if we hit Ctrl-C then let the while
         * loop know to break.
         */
        if (check < 0)
            exit(-1);
        else
            exit(1);
    }

So, when Ctrl-C is hit by the user, stop=1 and we get here. Then,
two things may happen:

 1. check < 0: my understanding is that, this is the failure
    case. We do exit(-1). On Linux (and maybe most Unixes), this
    actually becomes 255 in the shell

 2. check >= 0: we do exit(1). This is the success case. According
    to the protocol, we return 1 to the shell to allow shell loops
    from breaking free

Re: [PATCH 1/3] rt-migrate-test: fix return code

[John Kacur]

On Tue, 22 Mar 2016, Luiz Capitulino wrote:

> On Tue, 22 Mar 2016 15:59:50 +0100 (CET)
> John Kacur <jkacur@redhat.com> wrote:
> 
> > 
> > 
> > On Tue, 22 Mar 2016, Luiz Capitulino wrote:
> > 
> > > On Tue, 22 Mar 2016 15:22:57 +0100 (CET)
> > > John Kacur <jkacur@redhat.com> wrote:
> > > 
> > > > 
> > > > 
> > > > On Thu, 17 Mar 2016, Luiz Capitulino wrote:
> > > > 
> > > > > Change both return codes for the stop == true case:
> > > > > 
> > > > >  * For failures, use exit(1) as exit(-1) is wrong
> > > > >    (it actually becomes 255 in the shell)
> > > > > 
> > > > >  * For success, use exit(2) instead of exit(1) as
> > > > >    exit(1) is usually used for errors
> > > > > 
> > > > > This should preserve the requirement of allowing
> > > > > shell script while loops to break when Ctrl-C is hit.
> > > > > 
> > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > > > ---
> > > > >  src/rt-migrate-test/rt-migrate-test.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
> > > > > index d7b68dd..1362404 100644
> > > > > --- a/src/rt-migrate-test/rt-migrate-test.c
> > > > > +++ b/src/rt-migrate-test/rt-migrate-test.c
> > > > > @@ -599,9 +599,9 @@ int main (int argc, char **argv)
> > > > >  		 * loop know to break.
> > > > >  		 */
> > > > >  		if (check < 0)
> > > > > -			exit(-1);
> > > > > -		else
> > > > >  			exit(1);
> > > > > +		else
> > > > > +			exit(2);
> > > > >  	}
> > > > >  	if (check < 0)
> > > > >  		exit(-1);
> > > > > -- 
> > > > > 2.1.0
> > > > > 
> > > > > --
> > > > 
> > > > NAK - I've already told you this is wrong
> > > 
> > > Right and wrong can be subjective concepts :)
> > > 
> > > > 0 is the normal value for exit_success, not 2
> > > > you can change the failure from -1 to 1 if you wish
> > > 
> > > rt-migrate-test uses a different protocol as documented
> > > in the code. If we change success to 0, we'll break this
> > > protocol. Does it matter? I don't know, but I chose to
> > > keep it.
> > > 
> > > Now, if this is a complicated matter, we can just skip this
> > > patch.
> > > --
> > 
> > Really not sure what you're talking about
> > update your git repo and read the code again.
> 
> I'm looking at a638701a1899. Unless I grabbed the wrong branch
> again, the code looks the same.
> 
> This is the block I'm referring to:
> 
>     if (stop) {
>         /*
>          * We use this test in bash while loops
>          * So if we hit Ctrl-C then let the while
>          * loop know to break.
>          */
>         if (check < 0)
>             exit(-1);
>         else
>             exit(1);
>     }
> 
> So, when Ctrl-C is hit by the user, stop=1 and we get here. Then,
> two things may happen:
> 
>  1. check < 0: my understanding is that, this is the failure
>     case. We do exit(-1). On Linux (and maybe most Unixes), this
>     actually becomes 255 in the shell
> 
>  2. check >= 0: we do exit(1). This is the success case. According
>     to the protocol, we return 1 to the shell to allow shell loops
>     from breaking free
> --

I assume you are now working with
Repo: git://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git
Branch: devel/v0.97

Yes, that looks like the right code. I was talking to Steve Rostedt
about how a bash script would consume the values, and it looks like we do 
need tri-state exit codes. I'm going to revert the hunk on this program 
that changes the exit(-1) to exit(0), and we'll leave this program alone 
for now, it's a bit of an exception as to how it works compared to the 
other rt-tests programs

Thanks

John

Re: [PATCH 1/3] rt-migrate-test: fix return code

[Luiz Capitulino]

On Tue, 22 Mar 2016 17:24:43 +0100 (CET)
John Kacur <jkacur@redhat.com> wrote:

> 
> 
> On Tue, 22 Mar 2016, Luiz Capitulino wrote:
> 
> > On Tue, 22 Mar 2016 15:59:50 +0100 (CET)
> > John Kacur <jkacur@redhat.com> wrote:
> > 
> > > 
> > > 
> > > On Tue, 22 Mar 2016, Luiz Capitulino wrote:
> > > 
> > > > On Tue, 22 Mar 2016 15:22:57 +0100 (CET)
> > > > John Kacur <jkacur@redhat.com> wrote:
> > > > 
> > > > > 
> > > > > 
> > > > > On Thu, 17 Mar 2016, Luiz Capitulino wrote:
> > > > > 
> > > > > > Change both return codes for the stop == true case:
> > > > > > 
> > > > > >  * For failures, use exit(1) as exit(-1) is wrong
> > > > > >    (it actually becomes 255 in the shell)
> > > > > > 
> > > > > >  * For success, use exit(2) instead of exit(1) as
> > > > > >    exit(1) is usually used for errors
> > > > > > 
> > > > > > This should preserve the requirement of allowing
> > > > > > shell script while loops to break when Ctrl-C is hit.
> > > > > > 
> > > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > > > > ---
> > > > > >  src/rt-migrate-test/rt-migrate-test.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/rt-migrate-test/rt-migrate-test.c b/src/rt-migrate-test/rt-migrate-test.c
> > > > > > index d7b68dd..1362404 100644
> > > > > > --- a/src/rt-migrate-test/rt-migrate-test.c
> > > > > > +++ b/src/rt-migrate-test/rt-migrate-test.c
> > > > > > @@ -599,9 +599,9 @@ int main (int argc, char **argv)
> > > > > >  		 * loop know to break.
> > > > > >  		 */
> > > > > >  		if (check < 0)
> > > > > > -			exit(-1);
> > > > > > -		else
> > > > > >  			exit(1);
> > > > > > +		else
> > > > > > +			exit(2);
> > > > > >  	}
> > > > > >  	if (check < 0)
> > > > > >  		exit(-1);
> > > > > > -- 
> > > > > > 2.1.0
> > > > > > 
> > > > > > --
> > > > > 
> > > > > NAK - I've already told you this is wrong
> > > > 
> > > > Right and wrong can be subjective concepts :)
> > > > 
> > > > > 0 is the normal value for exit_success, not 2
> > > > > you can change the failure from -1 to 1 if you wish
> > > > 
> > > > rt-migrate-test uses a different protocol as documented
> > > > in the code. If we change success to 0, we'll break this
> > > > protocol. Does it matter? I don't know, but I chose to
> > > > keep it.
> > > > 
> > > > Now, if this is a complicated matter, we can just skip this
> > > > patch.
> > > > --
> > > 
> > > Really not sure what you're talking about
> > > update your git repo and read the code again.
> > 
> > I'm looking at a638701a1899. Unless I grabbed the wrong branch
> > again, the code looks the same.
> > 
> > This is the block I'm referring to:
> > 
> >     if (stop) {
> >         /*
> >          * We use this test in bash while loops
> >          * So if we hit Ctrl-C then let the while
> >          * loop know to break.
> >          */
> >         if (check < 0)
> >             exit(-1);
> >         else
> >             exit(1);
> >     }
> > 
> > So, when Ctrl-C is hit by the user, stop=1 and we get here. Then,
> > two things may happen:
> > 
> >  1. check < 0: my understanding is that, this is the failure
> >     case. We do exit(-1). On Linux (and maybe most Unixes), this
> >     actually becomes 255 in the shell
> > 
> >  2. check >= 0: we do exit(1). This is the success case. According
> >     to the protocol, we return 1 to the shell to allow shell loops
> >     from breaking free
> > --
> 
> I assume you are now working with
> Repo: git://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git
> Branch: devel/v0.97
> 
> Yes, that looks like the right code. I was talking to Steve Rostedt
> about how a bash script would consume the values, and it looks like we do 
> need tri-state exit codes. I'm going to revert the hunk on this program 
> that changes the exit(-1) to exit(0), and we'll leave this program alone 
> for now, it's a bit of an exception as to how it works compared to the 
> other rt-tests programs

Fair enough.

Re: [PATCH 2/3] don't use exit(-1) for failures

[Uwe Kleine-König]

Hallo,

On Thu, Mar 17, 2016 at 02:29:04PM -0400, Luiz Capitulino wrote:
> The kernel uses only 8 bits of the status as a return
> code, so this actually becomes 255 in the shell.
> 
> In any case, the most widely convension is exit(1)
> for failures, so let's be consistent.

The more portable version for this is to use EXIT_FAILURE. Not sure
though if it matters even for software that is not linux-centric like
rt-tests.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html