You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stdcxx.apache.org by Andrew Black <ab...@roguewave.com> on 2006/08/01 16:02:18 UTC
exec util testing commands [patch]
Greetings all
One thing that is needed is a method of testing the exec utility. This
patch provides a step towards this goal by adding several command line
switches. The majority of these switches allow the utility to function
as a test target which will produce certain (controlled) behavior. The
behavior needed includes exiting with a particular status or signal,
masking (ignoring) a particular signal, and sleeping a certain amount of
time.
The -v switch increases the value of the verbosity counter variable
The -q switch sets the value of the verbosity counter variable to 0
The --exit switch causes the utility to exit with the value specified.
The --sleep switch causes the utility to sleep for the number of seconds
specified.
The --signal switch causes the utility to send itself the signal
name/number specified.
The --ignore switch causes the utility to ignore the signal name/number
specified.
--Andrew Black
Log:
2006-08-01 Andrew Black <ab...@roguewave.com>
* exec.h (get_signo): Define macro for signal name to number conversion
* exec.cpp (get_signo): Implementation
* exec.cpp (signal_names): moved lookup table out of get_signame to
file scope
* cmdopt.h (verbose): Define global
* cmdopt.cpp (verbose): Instantiate global
* cmdopt.cpp (get_short_val, get_long_val, bad_option): Add helper
functions for option parsing
* cmdopt.cpp (eval_options): Use new functions, add -v, -q, --exit,
--sleep, --signal, --ignore switches
Re: exec util testing commands [patch]
Posted by Andrew Black <ab...@roguewave.com>.
Greetings all.
Attached is a replacement patch for the one in the previous email, with
a minor fix. This replacement fixes a bug in the --sleep switch, and a
similar bug in the --signal switch. (The switches were producing an
'unknown option' message after completing their actions. This probably
didn't matter for the --signal switch, as the flow usually wouldn't
reach that point, but it did matter for the --sleep switch)
--Andrew Black
Andrew Black wrote:
> Greetings all
>
> One thing that is needed is a method of testing the exec utility. This
> patch provides a step towards this goal by adding several command line
> switches. The majority of these switches allow the utility to function
> as a test target which will produce certain (controlled) behavior. The
> behavior needed includes exiting with a particular status or signal,
> masking (ignoring) a particular signal, and sleeping a certain amount of
> time.
>
> The -v switch increases the value of the verbosity counter variable
> The -q switch sets the value of the verbosity counter variable to 0
> The --exit switch causes the utility to exit with the value specified.
> The --sleep switch causes the utility to sleep for the number of seconds
> specified.
> The --signal switch causes the utility to send itself the signal
> name/number specified.
> The --ignore switch causes the utility to ignore the signal name/number
> specified.
>
> --Andrew Black
>
> Log:
> 2006-08-01 Andrew Black <ab...@roguewave.com>
> * exec.h (get_signo): Define macro for signal name to number conversion
> * exec.cpp (get_signo): Implementation
> * exec.cpp (signal_names): moved lookup table out of get_signame to
> file scope
> * cmdopt.h (verbose): Define global
> * cmdopt.cpp (verbose): Instantiate global
> * cmdopt.cpp (get_short_val, get_long_val, bad_option): Add helper
> functions for option parsing
> * cmdopt.cpp (eval_options): Use new functions, add -v, -q, --exit,
> --sleep, --signal, --ignore switches
Re: exec util testing commands [patch]
Posted by Martin Sebor <se...@roguewave.com>.
Andrew Black wrote:
[...]
> 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 <ab...@roguewave.com>
> * util.cpp (warn, terminate): Handle case of null target_name.
This is now in:
http://svn.apache.org/viewvc?rev=428096&view=rev
Martin
Re: exec util testing commands [patch]
Posted by Martin Sebor <se...@roguewave.com>.
Andrew Black wrote:
> Greetings
>
> One more take on this patch, hopefully the final. I think I've
> addressed all the concerns presented here.
Commited thus (I corrected your ChangeLog for you):
http://svn.apache.org/viewvc?rev=428171&view=rev
Martin
Re: exec util testing commands [patch]
Posted by Andrew Black <ab...@roguewave.com>.
Greetings
One more take on this patch, hopefully the final. I think I've
addressed all the concerns presented here.
--Andrew Black
Martin Sebor wrote:
> Andrew Black wrote:
>> 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.
>>
>
> Goody! :)
>
>> +const int
>> +get_signo (const char* signame)
>> +{
>> size_t i;
>> + int trans;
>> + char *junk;
>> +
>> + assert (0 != signame);
>> +
>> + if ('s' == tolower (signame [0]) && 'i' == tolower (signame [1])
>
> I'm afraid this is unsafe when char is a signed type. tolower() takes
> an int but the behavior of the function is undefined when the argument
> is neither EOF or representable in unsigned char.
>
> The safe way to use these functions is to convert the character to
> unsigned char first:
>
> typedef unsigned char UChar;
> if ( 's' == tolower ((UChar)signame [0])
> && 'i' == tolower ((UChar)signame [1])
>
>
>> + && 'g' == tolower (signame [2]))
>> + signame += 3;
>> + + if ('#' == signame [0])
>> + ++signame;
>> + + trans = strtol (signame, &junk, 0);
>
> Did you really intend to accept things like SIG#0xa and interpret
> input such as SIG#011 as octal? I would expect this function (and
> probably all the rest) to handle only decimal numbers and reject
> anything else.
>
>> +
>> + if (0 == *junk && 0 == errno)
>> + return trans;
>> +
>> + for (i = 0; signal_names [i].str; ++i) {
>> + if (0 == strcasecmp (signal_names [i].str, signame)) {
>
> strcasecmp() is non-standard, we can't use it. AFAIK, there's no
> standard case-insensitive string comparison function in C or in
> POSIX.
>
> Could you please fix these and resubmit?
>
> Thanks
> Martin
Re: exec util testing commands [patch]
Posted by Martin Sebor <se...@roguewave.com>.
Andrew Black wrote:
> 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.
>
Goody! :)
> +const int
> +get_signo (const char* signame)
> +{
> size_t i;
> + int trans;
> + char *junk;
> +
> + assert (0 != signame);
> +
> + if ('s' == tolower (signame [0]) && 'i' == tolower (signame [1])
I'm afraid this is unsafe when char is a signed type. tolower() takes
an int but the behavior of the function is undefined when the argument
is neither EOF or representable in unsigned char.
The safe way to use these functions is to convert the character to
unsigned char first:
typedef unsigned char UChar;
if ( 's' == tolower ((UChar)signame [0])
&& 'i' == tolower ((UChar)signame [1])
> + && 'g' == tolower (signame [2]))
> + signame += 3;
> +
> + if ('#' == signame [0])
> + ++signame;
> +
> + trans = strtol (signame, &junk, 0);
Did you really intend to accept things like SIG#0xa and interpret
input such as SIG#011 as octal? I would expect this function (and
probably all the rest) to handle only decimal numbers and reject
anything else.
> +
> + if (0 == *junk && 0 == errno)
> + return trans;
> +
> + for (i = 0; signal_names [i].str; ++i) {
> + if (0 == strcasecmp (signal_names [i].str, signame)) {
strcasecmp() is non-standard, we can't use it. AFAIK, there's no
standard case-insensitive string comparison function in C or in
POSIX.
Could you please fix these and resubmit?
Thanks
Martin
Re: exec util testing commands [patch]
Posted by Andrew Black <ab...@roguewave.com>.
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 <ab...@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
Re: exec util testing commands [patch]
Posted by Martin Sebor <se...@roguewave.com>.
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