On Tue, Jun 17, 2008 at 6:35 PM, Rafael Vanoni <Rafael.Vanoni at sun.com> wrote:
> Rafael Vanoni wrote:
>>
>> Aubrey Li wrote:
>>>
>>> 2008/6/16 Rafael Vanoni <Rafael.Vanoni at sun.com>:
>>>>
>>>> Here's a diff that fixes the last three bugs listed on the powertop
>>>> project
>>>> page:
>>>>
>>>> # PowerTOP does not exit with non-zero exit code if handed an invalid
>>>> option
>>>> # PowerTOP works with invalid options
>>>> # PowerTOP dumps usage message to stdout, not stderr
>>>
>>> - if ((bit_depth = get_bit_depth()) < 0)
>>> - return -1;
>>> + if ((bit_depth = get_bit_depth()) < 0) {
>>> + exit(EXIT_FAILURE);
>>> + return (EXIT_FAILURE);
>>> + }
>>>
>>> Why exit(EXIT_FAILURE) needed here?
>>
>> That happens if sysinfo(2) fails to return the bit depth. Do you prefer to
>> fall back to a default value for bit depth ?
>>
>>> +
>>> + if (argc > 1)
>>> + if (argv[1][0] != '-')
>>> + usage();
>>>
>>> This code looks ugly, and not a fix if run "powertop --"
>>> Any better idea?
>>
>> getopt_long(3C) doesn't check this, so we're left with having to do it
>> manually. Do you know of a routine that we could use here ?
>>
>> I've seen code in nevada that checks if the options are valid before
>> parsing them. But that's ugly code as well.
>
> Nevermind this bit, Konstantin just pointed me to optind.
> Here's a diff for that
>
> --- a/usr/src/cmd/powertop/powertop.c Fri May 23 10:54:13 2008 +0100
> +++ b/usr/src/cmd/powertop/powertop.c Tue Jun 17 11:33:04 2008 +0100
> @@ -113,6 +113,10 @@
> return -1;
> }
> }
> +
> + if (optind < argc) {
> + usage();
> + }
>
> printf("Solaris PowerTOP 1.0 (C) 2007 Intel Corporation \n\n");
>
>
This one looks good.
Thanks,
-Aubrey
