| Message ID | 1504615189-13315-1-git-send-email-petri.latvala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 05, 2017 at 03:39:49PM +0300, Petri Latvala wrote: > When no action is specified on the command line, print the usage help > text and exit with failure instead of SIGABRT. Fix some typos on the > usage text. > > Keep the abort() call in places where they can only be reached by > expanding the tool and forgetting to handle new parameters, with an > error message printed. > > CC: Ben Widawsky <benjamin.widawsky@intel.com> > Signed-off-by: Petri Latvala <petri.latvala@intel.com> > --- > tools/intel_l3_parity.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c > index eb00c50..1a4fae5 100644 > --- a/tools/intel_l3_parity.c > +++ b/tools/intel_l3_parity.c > @@ -172,9 +172,9 @@ static void usage(const char *name) > " -l, --list List the current L3 logs\n" > " -a, --clear-all Clear all disabled rows\n" > " -e, --enable Enable row, bank, subbank (undo -d)\n" > - " -d, --disable=<row,bank,subbank> Disable row, bank, subbank (inline arguments are deprecated. Please use -r, -b, -s instead\n" > - " -i, --inject [HSW only] Cause hardware to inject a row errors\n" > - " -u, --uninject [HSW only] Turn off hardware error injectection (undo -i)\n" > + " -d, --disable=<row,bank,subbank> Disable row, bank, subbank (inline arguments are deprecated. Please use -r, -b, -s instead)\n" > + " -i, --inject [HSW only] Cause hardware to inject a row error\n" > + " -u, --uninject [HSW only] Turn off hardware error injection (undo -i)\n" > " -L, --listen Listen for uevent errors\n", > name); > } > @@ -301,6 +301,7 @@ int main(int argc, char *argv[]) > action = c; > break; > default: > + fprintf(stderr, "Internal error: Unhandled flag %c\n", c); > abort(); > } > } > @@ -374,7 +375,12 @@ int main(int argc, char *argv[]) > break; > case 'L': > break; > + case '0': > + /* No action given */ > + usage(argv[0]); > + exit(EXIT_FAILURE); Won't this print usage once per slice? Or am I misreading how the patch applies ... -Daniel > default: > + fprintf(stderr, "Internal error: Unhandled action %d\n", action); > abort(); > } > } > -- > 2.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 17-09-05 18:16:23, Daniel Vetter wrote: >On Tue, Sep 05, 2017 at 03:39:49PM +0300, Petri Latvala wrote: >> When no action is specified on the command line, print the usage help >> text and exit with failure instead of SIGABRT. Fix some typos on the >> usage text. >> >> Keep the abort() call in places where they can only be reached by >> expanding the tool and forgetting to handle new parameters, with an >> error message printed. >> >> CC: Ben Widawsky <benjamin.widawsky@intel.com> >> Signed-off-by: Petri Latvala <petri.latvala@intel.com> >> --- >> tools/intel_l3_parity.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c >> index eb00c50..1a4fae5 100644 >> --- a/tools/intel_l3_parity.c >> +++ b/tools/intel_l3_parity.c >> @@ -172,9 +172,9 @@ static void usage(const char *name) >> " -l, --list List the current L3 logs\n" >> " -a, --clear-all Clear all disabled rows\n" >> " -e, --enable Enable row, bank, subbank (undo -d)\n" >> - " -d, --disable=<row,bank,subbank> Disable row, bank, subbank (inline arguments are deprecated. Please use -r, -b, -s instead\n" >> - " -i, --inject [HSW only] Cause hardware to inject a row errors\n" >> - " -u, --uninject [HSW only] Turn off hardware error injectection (undo -i)\n" >> + " -d, --disable=<row,bank,subbank> Disable row, bank, subbank (inline arguments are deprecated. Please use -r, -b, -s instead)\n" >> + " -i, --inject [HSW only] Cause hardware to inject a row error\n" >> + " -u, --uninject [HSW only] Turn off hardware error injection (undo -i)\n" +1 for fixing the typo. I think we should be careful about using "HSW only". In fact I'm in favor of removing what was already there because I honestly don't know which platforms support this feature. >> " -L, --listen Listen for uevent errors\n", >> name); >> } >> @@ -301,6 +301,7 @@ int main(int argc, char *argv[]) >> action = c; >> break; >> default: >> + fprintf(stderr, "Internal error: Unhandled flag %c\n", c); >> abort(); >> } >> } >> @@ -374,7 +375,12 @@ int main(int argc, char *argv[]) >> break; >> case 'L': >> break; >> + case '0': >> + /* No action given */ >> + usage(argv[0]); >> + exit(EXIT_FAILURE); > >Won't this print usage once per slice? Or am I misreading how the patch >applies ... >-Daniel > Looks wrong to me as well. >> default: >> + fprintf(stderr, "Internal error: Unhandled action %d\n", action); >> abort(); >> } >> } >> -- >> 2.9.3 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch
On 09/05/2017 07:16 PM, Daniel Vetter wrote: > On Tue, Sep 05, 2017 at 03:39:49PM +0300, Petri Latvala wrote: >> When no action is specified on the command line, print the usage help >> text and exit with failure instead of SIGABRT. Fix some typos on the >> usage text. >> >> Keep the abort() call in places where they can only be reached by >> expanding the tool and forgetting to handle new parameters, with an >> error message printed. >> >> CC: Ben Widawsky <benjamin.widawsky@intel.com> >> Signed-off-by: Petri Latvala <petri.latvala@intel.com> >> --- >> tools/intel_l3_parity.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c >> index eb00c50..1a4fae5 100644 >> --- a/tools/intel_l3_parity.c >> +++ b/tools/intel_l3_parity.c >> @@ -172,9 +172,9 @@ static void usage(const char *name) >> " -l, --list List the current L3 logs\n" >> " -a, --clear-all Clear all disabled rows\n" >> " -e, --enable Enable row, bank, subbank (undo -d)\n" >> - " -d, --disable=<row,bank,subbank> Disable row, bank, subbank (inline arguments are deprecated. Please use -r, -b, -s instead\n" >> - " -i, --inject [HSW only] Cause hardware to inject a row errors\n" >> - " -u, --uninject [HSW only] Turn off hardware error injectection (undo -i)\n" >> + " -d, --disable=<row,bank,subbank> Disable row, bank, subbank (inline arguments are deprecated. Please use -r, -b, -s instead)\n" >> + " -i, --inject [HSW only] Cause hardware to inject a row error\n" >> + " -u, --uninject [HSW only] Turn off hardware error injection (undo -i)\n" >> " -L, --listen Listen for uevent errors\n", >> name); >> } >> @@ -301,6 +301,7 @@ int main(int argc, char *argv[]) >> action = c; >> break; >> default: >> + fprintf(stderr, "Internal error: Unhandled flag %c\n", c); >> abort(); >> } >> } >> @@ -374,7 +375,12 @@ int main(int argc, char *argv[]) >> break; >> case 'L': >> break; >> + case '0': >> + /* No action given */ >> + usage(argv[0]); >> + exit(EXIT_FAILURE); > Won't this print usage once per slice? Or am I misreading how the patch > applies ... It prints the usage and calls exit(), what is the control flow that leads to printing it multiple times?
On Wed, Sep 06, 2017 at 12:31:05PM +0300, Petri Latvala wrote: > > > On 09/05/2017 07:16 PM, Daniel Vetter wrote: > > On Tue, Sep 05, 2017 at 03:39:49PM +0300, Petri Latvala wrote: > > > When no action is specified on the command line, print the usage help > > > text and exit with failure instead of SIGABRT. Fix some typos on the > > > usage text. > > > > > > Keep the abort() call in places where they can only be reached by > > > expanding the tool and forgetting to handle new parameters, with an > > > error message printed. > > > > > > CC: Ben Widawsky <benjamin.widawsky@intel.com> > > > Signed-off-by: Petri Latvala <petri.latvala@intel.com> > > > --- > > > tools/intel_l3_parity.c | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c > > > index eb00c50..1a4fae5 100644 > > > --- a/tools/intel_l3_parity.c > > > +++ b/tools/intel_l3_parity.c > > > @@ -172,9 +172,9 @@ static void usage(const char *name) > > > " -l, --list List the current L3 logs\n" > > > " -a, --clear-all Clear all disabled rows\n" > > > " -e, --enable Enable row, bank, subbank (undo -d)\n" > > > - " -d, --disable=<row,bank,subbank> Disable row, bank, subbank (inline arguments are deprecated. Please use -r, -b, -s instead\n" > > > - " -i, --inject [HSW only] Cause hardware to inject a row errors\n" > > > - " -u, --uninject [HSW only] Turn off hardware error injectection (undo -i)\n" > > > + " -d, --disable=<row,bank,subbank> Disable row, bank, subbank (inline arguments are deprecated. Please use -r, -b, -s instead)\n" > > > + " -i, --inject [HSW only] Cause hardware to inject a row error\n" > > > + " -u, --uninject [HSW only] Turn off hardware error injection (undo -i)\n" > > > " -L, --listen Listen for uevent errors\n", > > > name); > > > } > > > @@ -301,6 +301,7 @@ int main(int argc, char *argv[]) > > > action = c; > > > break; > > > default: > > > + fprintf(stderr, "Internal error: Unhandled flag %c\n", c); > > > abort(); > > > } > > > } > > > @@ -374,7 +375,12 @@ int main(int argc, char *argv[]) > > > break; > > > case 'L': > > > break; > > > + case '0': > > > + /* No action given */ > > > + usage(argv[0]); > > > + exit(EXIT_FAILURE); > > Won't this print usage once per slice? Or am I misreading how the patch > > applies ... > > > It prints the usage and calls exit(), what is the control flow that leads to > printing it multiple times? Ah, that's indeed a bit confusing control flow that usage() exists. Would be cleaner if we don't hide the call somewhere in a loop, but bail out more top-level. -Daniel
diff --git a/tools/intel_l3_parity.c b/tools/intel_l3_parity.c index eb00c50..1a4fae5 100644 --- a/tools/intel_l3_parity.c +++ b/tools/intel_l3_parity.c @@ -172,9 +172,9 @@ static void usage(const char *name) " -l, --list List the current L3 logs\n" " -a, --clear-all Clear all disabled rows\n" " -e, --enable Enable row, bank, subbank (undo -d)\n" - " -d, --disable=<row,bank,subbank> Disable row, bank, subbank (inline arguments are deprecated. Please use -r, -b, -s instead\n" - " -i, --inject [HSW only] Cause hardware to inject a row errors\n" - " -u, --uninject [HSW only] Turn off hardware error injectection (undo -i)\n" + " -d, --disable=<row,bank,subbank> Disable row, bank, subbank (inline arguments are deprecated. Please use -r, -b, -s instead)\n" + " -i, --inject [HSW only] Cause hardware to inject a row error\n" + " -u, --uninject [HSW only] Turn off hardware error injection (undo -i)\n" " -L, --listen Listen for uevent errors\n", name); } @@ -301,6 +301,7 @@ int main(int argc, char *argv[]) action = c; break; default: + fprintf(stderr, "Internal error: Unhandled flag %c\n", c); abort(); } } @@ -374,7 +375,12 @@ int main(int argc, char *argv[]) break; case 'L': break; + case '0': + /* No action given */ + usage(argv[0]); + exit(EXIT_FAILURE); default: + fprintf(stderr, "Internal error: Unhandled action %d\n", action); abort(); } }
When no action is specified on the command line, print the usage help text and exit with failure instead of SIGABRT. Fix some typos on the usage text. Keep the abort() call in places where they can only be reached by expanding the tool and forgetting to handle new parameters, with an error message printed. CC: Ben Widawsky <benjamin.widawsky@intel.com> Signed-off-by: Petri Latvala <petri.latvala@intel.com> --- tools/intel_l3_parity.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)