stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Black <abl...@roguewave.com>
Subject Re: exec util testing commands [patch]
Date Tue, 01 Aug 2006 22:32:09 GMT
Ok...

I've tried to address the issues noted below in the attached (revised) 
version of the test_switches.diff patch.  In the process of testing the 
modifications, I discovered a bug with how get_signo and get_signame 
determine the end of the array.  That bug has also been resolved in this 
revised patch.

In addition, I discovered another minor bug in the warn and terminate 
utility functions.  It had been assumed that target_name would always be 
set.  This assumption falters with the use of warn in bad_option and 
terminate in eval_options, which are called prior to setting the initial 
target_name.  A patch to correct this assumption is attached as 
error_msg.diff, with the changelog below.

--Andrew Black

Log:
2006-08-01 Andrew Black <ablack@roguewave.com>
     * util.cpp (warn, terminate): Handle case of null target_name.

Martin Sebor wrote:
> Andrew Black wrote:
>>
> 
> This looks good but a few minor changes are needed before I can
> commit it -- please see my comments below.
> 
>>
> [...]
>> +const int
>> +get_signo (char* signame)
> 
> The argument should be const char*...
> 
>> +{
> 
> ...and we should assert (0 != signame) here since that's what the
> the function assumes.
> 
>>      size_t i;
>> +
>> +    if ('S' == signame [0] && 'I' == signame [1] && 'G' == signame
[2])
>> +        signame += 3;
> 
> It would be nice to also handle lowercase letters like the kill
> command does (this is not a showstopper but it seems easy enough
> to implement that I would suggest to include it with the rest of
> the changes).
> 
>> +    +    if ('#' == signame [0])
>> +        ++signame;
>> +    +    if ('0' <= signame [0] && '9' >= signame [0])
>> +        return atoi (signame);
> 
> We should use strtol here and detect invalid input such as "SIG1XXX".
> 
>> +
>> +    for (i = 0; i < name_count; ++i) {
>> +        if (0 == strcmp (signal_names [i].str, signame)) {
>> +            return signal_names [i].val;
> 
> Lowercase handling also needs to be done here.
> 
> [...]
>> Index: exec.h
>> ===================================================================
>> --- exec.h    (revision 427578)
>> +++ exec.h    (working copy)
>> @@ -32,6 +32,8 @@
>>      int killed;
>>  };
>>  
>> +const int get_signo (char* const signame);
> 
> Let's use const char* consistently.
> 
> [...]
>> +static char*
>> +get_short_val (char* const* argv, int* idx)
>> +{
> 
> Assert all preconditions please.
> 
> [...]
>> +static char*
>> +get_long_val (char* const * argv, int* idx, unsigned offset)
>> +{
> 
> Assert preconditions.
> 
> [...]
>> +static void
>> +bad_option (char* const opt)
>> +{
> 
> Assert preconditions.
> 
>> +    printf ("Unknown option: %s\n", opt);
> 
> This should (continue to) go to stderr.
> 
> Also, shouldn't this be handled via a call to terminate() so that
> the output is consistent and includes the name of the executable?
> 
> [...]
>> @@ -132,19 +191,70 @@
> [...]
>> +            else if (6 <= arglen && 0 == memcmp ("--exit", argv [i],

>> 6)) {
>> +                val = get_long_val (argv, &i, 6);
>> +                if (val)
>> +                    exit (atoi (val));
> 
> I would also suggest using strtol() here (and probably everywhere
> else we're using atoi) and handling malformed input for robustness.
> 
> Thanks
> Martin

Mime
View raw message