[PATCH] [BUILTIN] Allow SIG* signal names.

[PATCH] [BUILTIN] Allow SIG* signal names.

[Isaac Jurado]

In other shells, both trap and kill builtins accept two form of signal names,
i.e., TERM and SIGTERM.  Even /bin/kill allows the SIG* form.  Having dash fail
by not recognizing the SIG prefix introduces some confusion among users.
---
 src/trap.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/trap.c b/src/trap.c
index 17316c9..a64133c 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -408,6 +408,9 @@ int decode_signal(const char *string, int minsig)
 		return signo;
 	}
 
+	if (string[0] == 'S' && string[1] == 'I' && string[2] == 'G')
+		string += 3;
+
 	for (signo = minsig; signo < NSIG; signo++) {
 		if (!strcasecmp(string, signal_names[signo])) {
 			return signo;
-- 
1.7.10.4

Re: [PATCH] [BUILTIN] Allow SIG* signal names.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

On 07/01/2012 04:12 AM, Isaac Jurado wrote:
> In other shells, both trap and kill builtins accept two form of signal names,
> i.e., TERM and SIGTERM.  Even /bin/kill allows the SIG* form.  Having dash fail
> by not recognizing the SIG prefix introduces some confusion among users.
> ---
>  src/trap.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/trap.c b/src/trap.c
> index 17316c9..a64133c 100644
> --- a/src/trap.c
> +++ b/src/trap.c
> @@ -408,6 +408,9 @@ int decode_signal(const char *string, int minsig)
>  		return signo;
>  	}
>  
> +	if (string[0] == 'S' && string[1] == 'I' && string[2] == 'G')
> +		string += 3;
> +

POSIX says:

"Implementations may permit names with the SIG prefix or ignore case in
signal names as an extension."

>  	for (signo = minsig; signo < NSIG; signo++) {
>  		if (!strcasecmp(string, signal_names[signo])) {

And since we're already ignoring case in the rest of the name, you
should also ignore case for the SIG, if we decide to accept the
non-required bloat this patch provides.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [PATCH] [BUILTIN] Allow SIG* signal names.

[Paul Gilmartin]

On Jul 2, 2012, at 07:51, Eric Blake wrote:
> 
> ... non-required bloat ...
>  
The key phrase.  And one value of a shell lacking such
extensions is that it provides an excellent test bed for
code intended to be portable within the POSIX spec.

-- gil

Re: [PATCH] [BUILTIN] Allow SIG* signal names.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

On 07/02/2012 08:11 AM, Paul Gilmartin wrote:
> On Jul 2, 2012, at 07:51, Eric Blake wrote:
>>
>> ... non-required bloat ...
>>  
> The key phrase.  And one value of a shell lacking such
> extensions is that it provides an excellent test bed for
> code intended to be portable within the POSIX spec.

That argues that we should drop our strcasecmp() for the much simpler
strcmp(), in order to remove the bloat we already have.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [PATCH] [BUILTIN] Allow SIG* signal names.

[Isaac Jurado]

On Mon, Jul 2, 2012 at 4:22 PM, Eric Blake <eblake@redhat.com> wrote:
> On 07/02/2012 08:11 AM, Paul Gilmartin wrote:
>> On Jul 2, 2012, at 07:51, Eric Blake wrote:
>>>
>>> ... non-required bloat ...
>>>
>> The key phrase.  And one value of a shell lacking such
>> extensions is that it provides an excellent test bed for
>> code intended to be portable within the POSIX spec.
>
> That argues that we should drop our strcasecmp() for the much simpler
> strcmp(), in order to remove the bloat we already have.

I guess my patch has no chance to be accepted.  But I'm still curious
about what kind of "bloat" you are referring to.  I'm assuming it's not
code bloat in terms of lines of code.

If the signal name to number conversion seems too expensive (linear
search multiplied by the string lengths, wether it is case sensitive or
not), there is a much more elegant solution: perfect hashing.

Since the set of signal names to be recognized is known at compile time,
you can use a perfect hash function generator like GNU Perf [1] which
minimizes character comparisons in order to return the desired constant
assigned to a name/symbol/token/whatever-string.

Furthermore, you could even support POSIX and SIG* prefixed names,
wether they are case sensitive or not, at no additional cost.  Gperf is
a comple time dependency so the final binary can remain only linked to
libc.

Best regards.

-- 
Isaac Jurado

"The noblest pleasure is the joy of understanding"
Leonardo da Vinci

Re: [PATCH] [BUILTIN] Allow SIG* signal names.

[Isaac Jurado]

On Mon, Jul 2, 2012 at 8:53 PM, Isaac Jurado <diptongo@gmail.com> wrote:
>
> Since the set of signal names to be recognized is known at compile
> time, you can use a perfect hash function generator like GNU Perf [1]
> which minimizes character comparisons in order to return the desired
> constant assigned to a name/symbol/token/whatever-string.

As usual, I forgot the link.  My apologies:

[1] http://www.gnu.org/software/gperf/

-- 
Isaac Jurado

"The noblest pleasure is the joy of understanding"
Leonardo da Vinci

Re: [PATCH] [BUILTIN] Allow SIG* signal names.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2285 bytes --]

On 07/02/2012 12:53 PM, Isaac Jurado wrote:
> On Mon, Jul 2, 2012 at 4:22 PM, Eric Blake <eblake@redhat.com> wrote:
>> On 07/02/2012 08:11 AM, Paul Gilmartin wrote:
>>> On Jul 2, 2012, at 07:51, Eric Blake wrote:
>>>>
>>>> ... non-required bloat ...
>>>>
>>> The key phrase.  And one value of a shell lacking such
>>> extensions is that it provides an excellent test bed for
>>> code intended to be portable within the POSIX spec.
>>
>> That argues that we should drop our strcasecmp() for the much simpler
>> strcmp(), in order to remove the bloat we already have.
> 
> I guess my patch has no chance to be accepted.

I'm not the maintainer, so my decision is not indicative of what the
dash maintainer will choose.  But my personal preference would be that
we change this area of code, either to:

1. be lighter-weight (drop strcasecmp, which is locale-dependent, and
replace it with strcmp)

2. be more user-friendly (accept optional case-insensitive SIG prefix)

Both approaches are permitted by POSIX, so it boils down to a judgment
call of whether providing useful extensions or providing a minimally
compliant shell is more important.

>  But I'm still curious
> about what kind of "bloat" you are referring to.  I'm assuming it's not
> code bloat in terms of lines of code.

Even one byte larger in the final executable size has been deemed bloat
on this list in the past.  Dash prides itself on being minimalistic, but
you happened to point out an area of code that is not currently minimal.

> 
> If the signal name to number conversion seems too expensive (linear
> search multiplied by the string lengths, wether it is case sensitive or
> not), there is a much more elegant solution: perfect hashing.

Indeed, that would provide faster lookup, but it would also cost more
executable size (the storage requirements for a perfect hash table are
larger than the size of a loop comparison); I don't know whether the
preference is for speed, for minimal size, or for a hybrid of the two
(where larger size is okay only if it proves to have more speed).  So
hopefully the dash maintainer will chime in on the topic.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [PATCH] [BUILTIN] Allow SIG* signal names.

[Isaac Jurado]

On Mon, Jul 2, 2012 at 9:07 PM, Eric Blake <eblake@redhat.com> wrote:
>
> I'm not the maintainer, so my decision is not indicative of what the
> dash maintainer will choose.  But my personal preference would be that
> we change this area of code, either to:
>
> 1. be lighter-weight (drop strcasecmp, which is locale-dependent, and
>    replace it with strcmp)
>
> 2. be more user-friendly (accept optional case-insensitive SIG prefix)
>
> Both approaches are permitted by POSIX, so it boils down to a judgment
> call of whether providing useful extensions or providing a minimally
> compliant shell is more important.

Reasonable enough.

> Even one byte larger in the final executable size has been deemed
> bloat on this list in the past.  Dash prides itself on being
> minimalistic, but you happened to point out an area of code that is
> not currently minimal.

But swapping strcasecm with strcmp is not going to affect final binary
size (well, except for the extra bytes in the PLT), so I deduce that
bloat for dash also means runtime efficiency.

> Indeed, that would provide faster lookup, but it would also cost more
> executable size (the storage requirements for a perfect hash table are
> larger than the size of a loop comparison); I don't know whether the
> preference is for speed, for minimal size, or for a hybrid of the two
> (where larger size is okay only if it proves to have more speed).  So
> hopefully the dash maintainer will chime in on the topic.

I'm looking forward.  Thanks a lot for the explanations.

Cheers

P.S.: I guess the SIG* prefix check may be "minimized" with some ugly
casts and masks (in a platform dependent way and assuming the string is
properly aligned, of course).

-- 
Isaac Jurado

"The noblest pleasure is the joy of understanding"
Leonardo da Vinci

Re: [PATCH] [BUILTIN] Allow SIG* signal names.

[Jilles Tjoelker]

On Mon, Jul 02, 2012 at 08:22:20AM -0600, Eric Blake wrote:
> On 07/02/2012 08:11 AM, Paul Gilmartin wrote:
> > On Jul 2, 2012, at 07:51, Eric Blake wrote:

> >> ... non-required bloat ...

> > The key phrase.  And one value of a shell lacking such
> > extensions is that it provides an excellent test bed for
> > code intended to be portable within the POSIX spec.

> That argues that we should drop our strcasecmp() for the much simpler
> strcmp(), in order to remove the bloat we already have.

Such a change would be at odds with the goal of dash to be a usable
shell. It is not good to break existing scripts, even if those scripts
are not strictly POSIX-compliant, and particularly so if the scripts
relied on designed behaviour. Such changes can waste a lot of time.

Leaving the behaviour unchanged (keeping case-insensitivity but not
adding support for the SIG prefix) would strike a balance between
usability and avoiding bloat.

Moreover, dash has many other extensions to POSIX, such as:

* A particular behaviour for "export" without parameters.

* The "local" builtin.

* The XSI "type" builtin (depending on whether you want to allow XSI or
  not).

* XSI-style signal numbers in the "trap" special builtin, and more of
  them than required.

* The XSI "ulimit" builtin, with many more options than the required -f.

* Function definitions consisting of a simple command, like
    f() echo hi

* A particular (probably useful) behaviour for double-quotes within
  backticks within double-quotes, like
    echo "`echo "hi there"`"

* A particular behaviour for double-quotes within Bourne-style parameter
  substitutions within double-quotes, like
    echo "${IFS+"*"}"

* Arithmetic expansion with a larger range than signed long (even though
  POSIX "encourages" this in non-normative text, it is not required).

Although dash is fairly minimalistic and may be useful in testing
scripts (also because it detects certain syntax errors sooner), its
usability goal makes that it has many non-mandatory features. You may
like posh which attempts to comply to Debian policy only, even at the
cost of usability.

-- 
Jilles Tjoelker

Re: [PATCH] [BUILTIN] Allow SIG* signal names.

[Herbert Xu]

Jilles Tjoelker <jilles@stack.nl> wrote:
> On Mon, Jul 02, 2012 at 08:22:20AM -0600, Eric Blake wrote:
>> On 07/02/2012 08:11 AM, Paul Gilmartin wrote:
>> > On Jul 2, 2012, at 07:51, Eric Blake wrote:
> 
>> >> ... non-required bloat ...
> 
>> > The key phrase.  And one value of a shell lacking such
>> > extensions is that it provides an excellent test bed for
>> > code intended to be portable within the POSIX spec.
> 
>> That argues that we should drop our strcasecmp() for the much simpler
>> strcmp(), in order to remove the bloat we already have.
> 
> Such a change would be at odds with the goal of dash to be a usable
> shell. It is not good to break existing scripts, even if those scripts
> are not strictly POSIX-compliant, and particularly so if the scripts
> relied on designed behaviour. Such changes can waste a lot of time.

I agree with Jilles.  I don't think any change is required here.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [BUILTIN] Allow SIG* signal names.

[Isaac Jurado]

On Thu, Jul 5, 2012 at 9:43 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>>
>>> That argues that we should drop our strcasecmp() for the much simpler
>>> strcmp(), in order to remove the bloat we already have.
>>
>> Such a change would be at odds with the goal of dash to be a usable
>> shell. It is not good to break existing scripts, even if those
>> scripts are not strictly POSIX-compliant, and particularly so if the
>> scripts relied on designed behaviour. Such changes can waste a lot of
>> time.
>
> I agree with Jilles.  I don't think any change is required here.

That answers directly to the case sensitivity of signal names.  Is it
also the same answer about supporting the SIG* prefix?

Cheers.

-- 
Isaac Jurado

"The noblest pleasure is the joy of understanding"
Leonardo da Vinci