[PATCH][rt-tests] cyclictest: Make the tracemark option imply notrace

[PATCH][rt-tests] cyclictest: Make the tracemark option imply notrace

[John Kacur]

>From 6b663b39fc5dc7c4e9c78ab22012f07490e53145 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Thu, 24 Mar 2016 15:40:03 +0100
Subject: [PATCH] cyclictest: Make the tracemark option imply notrace

The new --tracemark option can be used to run cyclictest under
trace-cmd.

This means we don't want cyclictest's built-in tracing to be used, so
this option is only compatible with --notrace.

Therefore turn --notrace on if --tracemark is invoked even if the user
doesn't explicitly request this.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 src/cyclictest/cyclictest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 4844dfaf33a6..158bceda1204 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -1765,6 +1765,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
 #endif
 			break;
 		case OPT_TRACEMARK:
+			notrace = 1; /* using --tracemark implies --notrace */
 			trace_marker = 1; break;
 		}
 	}
-- 
2.4.3

Re: [PATCH][rt-tests] cyclictest: Make the tracemark option imply notrace

[Luiz Capitulino]

On Thu, 24 Mar 2016 15:44:39 +0100 (CET)
John Kacur <jkacur@redhat.com> wrote:

> From 6b663b39fc5dc7c4e9c78ab22012f07490e53145 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Thu, 24 Mar 2016 15:40:03 +0100
> Subject: [PATCH] cyclictest: Make the tracemark option imply notrace
> 
> The new --tracemark option can be used to run cyclictest under
> trace-cmd.
> 
> This means we don't want cyclictest's built-in tracing to be used, so
> this option is only compatible with --notrace.
> 
> Therefore turn --notrace on if --tracemark is invoked even if the user
> doesn't explicitly request this.

I'm not necessarily against this, but at the same time I think
I prefer options doing only one thing.

> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
>  src/cyclictest/cyclictest.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index 4844dfaf33a6..158bceda1204 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -1765,6 +1765,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
>  #endif
>  			break;
>  		case OPT_TRACEMARK:
> +			notrace = 1; /* using --tracemark implies --notrace */
>  			trace_marker = 1; break;
>  		}
>  	}

Re: [PATCH][rt-tests] cyclictest: Make the tracemark option imply notrace

[John Kacur]

On Thu, 24 Mar 2016, Luiz Capitulino wrote:

> On Thu, 24 Mar 2016 15:44:39 +0100 (CET)
> John Kacur <jkacur@redhat.com> wrote:
> 
> > From 6b663b39fc5dc7c4e9c78ab22012f07490e53145 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Thu, 24 Mar 2016 15:40:03 +0100
> > Subject: [PATCH] cyclictest: Make the tracemark option imply notrace
> > 
> > The new --tracemark option can be used to run cyclictest under
> > trace-cmd.
> > 
> > This means we don't want cyclictest's built-in tracing to be used, so
> > this option is only compatible with --notrace.
> > 
> > Therefore turn --notrace on if --tracemark is invoked even if the user
> > doesn't explicitly request this.
> 
> I'm not necessarily against this, but at the same time I think
> I prefer options doing only one thing.

Maybe think about it a little differently. The option implies a certain 
consistent state. So, the option really only does do one thing, it puts 
cyclictest in the correct state as required. If we don't do this, someone 
is sure to use the --tracemark option without --notrace, and some 
unexpected things are going to occur.

By the way, I like your new option, we've been thinking about doing 
something like that for awhile.

Cheers!

> 
> > 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> > ---
> >  src/cyclictest/cyclictest.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> > index 4844dfaf33a6..158bceda1204 100644
> > --- a/src/cyclictest/cyclictest.c
> > +++ b/src/cyclictest/cyclictest.c
> > @@ -1765,6 +1765,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
> >  #endif
> >  			break;
> >  		case OPT_TRACEMARK:
> > +			notrace = 1; /* using --tracemark implies --notrace */
> >  			trace_marker = 1; break;
> >  		}
> >  	}
> 
> --
> 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
> 

Re: [PATCH][rt-tests] cyclictest: Make the tracemark option imply notrace

[Mats Karrman]

On 2016-03-24 16:24, John Kacur wrote:
>
> On Thu, 24 Mar 2016, Luiz Capitulino wrote:
>
>> On Thu, 24 Mar 2016 15:44:39 +0100 (CET)
>> John Kacur <jkacur@redhat.com> wrote:
>>
...
>>> Therefore turn --notrace on if --tracemark is invoked even if the user
>>> doesn't explicitly request this.
>> I'm not necessarily against this, but at the same time I think
>> I prefer options doing only one thing.
> Maybe think about it a little differently. The option implies a certain
> consistent state. So, the option really only does do one thing, it puts
> cyclictest in the correct state as required. If we don't do this, someone
> is sure to use the --tracemark option without --notrace, and some
> unexpected things are going to occur.
>
Another alternative is to exit with an error if conflicting options are 
used.

// Mats

Re: [PATCH][rt-tests] cyclictest: Make the tracemark option imply notrace

[Luiz Capitulino]

On Thu, 24 Mar 2016 16:24:47 +0100 (CET)
John Kacur <jkacur@redhat.com> wrote:

> 
> 
> On Thu, 24 Mar 2016, Luiz Capitulino wrote:
> 
> > On Thu, 24 Mar 2016 15:44:39 +0100 (CET)
> > John Kacur <jkacur@redhat.com> wrote:
> > 
> > > From 6b663b39fc5dc7c4e9c78ab22012f07490e53145 Mon Sep 17 00:00:00 2001
> > > From: John Kacur <jkacur@redhat.com>
> > > Date: Thu, 24 Mar 2016 15:40:03 +0100
> > > Subject: [PATCH] cyclictest: Make the tracemark option imply notrace
> > > 
> > > The new --tracemark option can be used to run cyclictest under
> > > trace-cmd.
> > > 
> > > This means we don't want cyclictest's built-in tracing to be used, so
> > > this option is only compatible with --notrace.
> > > 
> > > Therefore turn --notrace on if --tracemark is invoked even if the user
> > > doesn't explicitly request this.
> > 
> > I'm not necessarily against this, but at the same time I think
> > I prefer options doing only one thing.
> 
> Maybe think about it a little differently. The option implies a certain 
> consistent state. So, the option really only does do one thing, it puts 
> cyclictest in the correct state as required. If we don't do this, someone 
> is sure to use the --tracemark option without --notrace, and some 
> unexpected things are going to occur.

I agree with your reasoning, and I again, I'm not against the change.

But my thinking is that, as a good practice, command-line options should
generally do one thing only. If we go too far in doing "one option
implies other" very soon we'll get unexpected behavior with long
command-lines.

> 
> By the way, I like your new option, we've been thinking about doing 
> something like that for awhile.
> 
> Cheers!
> 
> > 
> > > 
> > > Signed-off-by: John Kacur <jkacur@redhat.com>
> > > ---
> > >  src/cyclictest/cyclictest.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> > > index 4844dfaf33a6..158bceda1204 100644
> > > --- a/src/cyclictest/cyclictest.c
> > > +++ b/src/cyclictest/cyclictest.c
> > > @@ -1765,6 +1765,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
> > >  #endif
> > >  			break;
> > >  		case OPT_TRACEMARK:
> > > +			notrace = 1; /* using --tracemark implies --notrace */
> > >  			trace_marker = 1; break;
> > >  		}
> > >  	}
> > 
> > --
> > 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
> > 
>