* [PATCH] [BUILTIN] Allow SIG* signal names. @ 2012-07-01 10:12 Isaac Jurado 2012-07-02 13:51 ` Eric Blake 0 siblings, 1 reply; 11+ messages in thread From: Isaac Jurado @ 2012-07-01 10:12 UTC (permalink / raw) 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] [BUILTIN] Allow SIG* signal names. 2012-07-01 10:12 [PATCH] [BUILTIN] Allow SIG* signal names Isaac Jurado @ 2012-07-02 13:51 ` Eric Blake 2012-07-02 14:11 ` Paul Gilmartin 0 siblings, 1 reply; 11+ messages in thread From: Eric Blake @ 2012-07-02 13:51 UTC (permalink / raw) To: Isaac Jurado; +Cc: dash [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [BUILTIN] Allow SIG* signal names. 2012-07-02 13:51 ` Eric Blake @ 2012-07-02 14:11 ` Paul Gilmartin 2012-07-02 14:22 ` Eric Blake 0 siblings, 1 reply; 11+ messages in thread From: Paul Gilmartin @ 2012-07-02 14:11 UTC (permalink / raw) To: Eric Blake; +Cc: dash 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [BUILTIN] Allow SIG* signal names. 2012-07-02 14:11 ` Paul Gilmartin @ 2012-07-02 14:22 ` Eric Blake 2012-07-02 18:53 ` Isaac Jurado 2012-07-03 20:13 ` Jilles Tjoelker 0 siblings, 2 replies; 11+ messages in thread From: Eric Blake @ 2012-07-02 14:22 UTC (permalink / raw) To: Paul Gilmartin; +Cc: dash [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [BUILTIN] Allow SIG* signal names. 2012-07-02 14:22 ` Eric Blake @ 2012-07-02 18:53 ` Isaac Jurado 2012-07-02 18:57 ` Isaac Jurado 2012-07-02 19:07 ` Eric Blake 2012-07-03 20:13 ` Jilles Tjoelker 1 sibling, 2 replies; 11+ messages in thread From: Isaac Jurado @ 2012-07-02 18:53 UTC (permalink / raw) To: Eric Blake; +Cc: Paul Gilmartin, dash 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [BUILTIN] Allow SIG* signal names. 2012-07-02 18:53 ` Isaac Jurado @ 2012-07-02 18:57 ` Isaac Jurado 2012-07-02 19:07 ` Eric Blake 1 sibling, 0 replies; 11+ messages in thread From: Isaac Jurado @ 2012-07-02 18:57 UTC (permalink / raw) To: Eric Blake; +Cc: Paul Gilmartin, dash 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [BUILTIN] Allow SIG* signal names. 2012-07-02 18:53 ` Isaac Jurado 2012-07-02 18:57 ` Isaac Jurado @ 2012-07-02 19:07 ` Eric Blake 2012-07-02 19:21 ` Isaac Jurado 1 sibling, 1 reply; 11+ messages in thread From: Eric Blake @ 2012-07-02 19:07 UTC (permalink / raw) To: Isaac Jurado; +Cc: Paul Gilmartin, dash [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [BUILTIN] Allow SIG* signal names. 2012-07-02 19:07 ` Eric Blake @ 2012-07-02 19:21 ` Isaac Jurado 0 siblings, 0 replies; 11+ messages in thread From: Isaac Jurado @ 2012-07-02 19:21 UTC (permalink / raw) To: Eric Blake; +Cc: Paul Gilmartin, dash 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [BUILTIN] Allow SIG* signal names. 2012-07-02 14:22 ` Eric Blake 2012-07-02 18:53 ` Isaac Jurado @ 2012-07-03 20:13 ` Jilles Tjoelker 2012-07-05 7:43 ` Herbert Xu 1 sibling, 1 reply; 11+ messages in thread From: Jilles Tjoelker @ 2012-07-03 20:13 UTC (permalink / raw) To: Eric Blake; +Cc: Paul Gilmartin, dash 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [BUILTIN] Allow SIG* signal names. 2012-07-03 20:13 ` Jilles Tjoelker @ 2012-07-05 7:43 ` Herbert Xu 2012-07-05 21:01 ` Isaac Jurado 0 siblings, 1 reply; 11+ messages in thread From: Herbert Xu @ 2012-07-05 7:43 UTC (permalink / raw) To: Jilles Tjoelker; +Cc: eblake, PaulGBoulder, dash 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] [BUILTIN] Allow SIG* signal names. 2012-07-05 7:43 ` Herbert Xu @ 2012-07-05 21:01 ` Isaac Jurado 0 siblings, 0 replies; 11+ messages in thread From: Isaac Jurado @ 2012-07-05 21:01 UTC (permalink / raw) To: Herbert Xu; +Cc: Jilles Tjoelker, eblake, PaulGBoulder, dash 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-07-05 21:01 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-01 10:12 [PATCH] [BUILTIN] Allow SIG* signal names Isaac Jurado 2012-07-02 13:51 ` Eric Blake 2012-07-02 14:11 ` Paul Gilmartin 2012-07-02 14:22 ` Eric Blake 2012-07-02 18:53 ` Isaac Jurado 2012-07-02 18:57 ` Isaac Jurado 2012-07-02 19:07 ` Eric Blake 2012-07-02 19:21 ` Isaac Jurado 2012-07-03 20:13 ` Jilles Tjoelker 2012-07-05 7:43 ` Herbert Xu 2012-07-05 21:01 ` Isaac Jurado
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.