incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: exec util testing commands [patch]
Date Tue, 01 Aug 2006 16:52:42 GMT
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