Re: [PATCH i-g-t] intel_l3_parity: More helpful output in case of errors

[Ben Widawsky <benjamin.widawsky@intel.com>]

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

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx