dash.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`
@ 2016-08-04  5:54 Kylie McClain
  2016-08-04  5:54 ` [PATCH 2/2] histedit: Remove non-glibc fallback code Kylie McClain
  2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk
  0 siblings, 2 replies; 9+ messages in thread
From: Kylie McClain @ 2016-08-04  5:54 UTC (permalink / raw)
  To: dash; +Cc: Kylie McClain

From: Kylie McClain <somasis@exherbo.org>

nl, while specified in POSIX, is rather obscure and isn't provided by small
coreutils implementations such as `busybox`. This while loop works just as
well for our purposes.
---
 src/mkbuiltins | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/mkbuiltins b/src/mkbuiltins
index b4d6f4e..a47bce8 100644
--- a/src/mkbuiltins
+++ b/src/mkbuiltins
@@ -101,7 +101,8 @@ cat <<\!
  */
 
 !
-sed 's/	-[a-z]*//' $temp2 | nl -ba -v0 |
+sed 's/	-[a-z]*//' $temp2 | while read line;do \
+	i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done |
 	LC_ALL= LC_COLLATE=C sort -u -k 3,3 |
 	tr abcdefghijklmnopqrstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ |
 	awk '{	printf "#define %s (builtincmd + %d)\n", $3, $1}'
-- 
2.9.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] histedit: Remove non-glibc fallback code
  2016-08-04  5:54 [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Kylie McClain
@ 2016-08-04  5:54 ` Kylie McClain
  2016-08-04 15:59   ` Jilles Tjoelker
  2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk
  1 sibling, 1 reply; 9+ messages in thread
From: Kylie McClain @ 2016-08-04  5:54 UTC (permalink / raw)
  To: dash; +Cc: Kylie McClain

From: Kylie McClain <somasis@exherbo.org>

This fallback code seems to go back the import of the repository back in 2005,
it fails on musl libc, and there aren't any comments explaining why this
difference is needed. Regardless, any compatibility ifdefs should probably be
checking macros defined on systems that need a different code path, rather than
just having fallback code for non-glibc.
---
 src/histedit.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/histedit.c b/src/histedit.c
index 94465d7..4498cf4 100644
--- a/src/histedit.c
+++ b/src/histedit.c
@@ -214,11 +214,7 @@ histcmd(int argc, char **argv)
 	if (argc == 1)
 		sh_error("missing history argument");
 
-#ifdef __GLIBC__
 	optind = 0;
-#else
-	optreset = 1; optind = 1; /* initialize getopt */
-#endif
 	while (not_fcnumber(argv[optind]) &&
 	      (ch = getopt(argc, argv, ":e:lnrs")) != -1)
 		switch ((char)ch) {
-- 
2.9.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] histedit: Remove non-glibc fallback code
  2016-08-04  5:54 ` [PATCH 2/2] histedit: Remove non-glibc fallback code Kylie McClain
@ 2016-08-04 15:59   ` Jilles Tjoelker
  2016-08-08 15:50     ` Jilles Tjoelker
  0 siblings, 1 reply; 9+ messages in thread
From: Jilles Tjoelker @ 2016-08-04 15:59 UTC (permalink / raw)
  To: Kylie McClain; +Cc: dash, Kylie McClain

On Thu, Aug 04, 2016 at 01:54:11AM -0400, Kylie McClain wrote:
> From: Kylie McClain <somasis@exherbo.org>

> This fallback code seems to go back the import of the repository back
> in 2005, it fails on musl libc, and there aren't any comments
> explaining why this difference is needed. Regardless, any
> compatibility ifdefs should probably be checking macros defined on
> systems that need a different code path, rather than just having
> fallback code for non-glibc.

Setting optreset is required on FreeBSD and NetBSD which do not
recognize setting optind to 0. POSIX specifies neither of these methods
to reset getopt().

However, removing the dependency on this feature is simple. The shell
already features a function nextopt() that can be used to parse options
and is automatically set up for builtins.

From 23da600dcff616662a93f307420d9142598e2cae Mon Sep 17 00:00:00 2001
From: Jilles Tjoelker <jilles@stack.nl>
Date: Thu, 4 Aug 2016 17:51:12 +0200
Subject: [PATCH 1/2] [HISTEDIT] Stop depending on getopt reset feature.

Instead, use our own nextopt() function.
---
 src/histedit.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/histedit.c b/src/histedit.c
index 94465d7..ec45065 100644
--- a/src/histedit.c
+++ b/src/histedit.c
@@ -214,13 +214,8 @@ histcmd(int argc, char **argv)
 	if (argc == 1)
 		sh_error("missing history argument");
 
-#ifdef __GLIBC__
-	optind = 0;
-#else
-	optreset = 1; optind = 1; /* initialize getopt */
-#endif
 	while (not_fcnumber(argv[optind]) &&
-	      (ch = getopt(argc, argv, ":e:lnrs")) != -1)
+	      (ch = nextopt(":e:lnrs")) != '\0')
 		switch ((char)ch) {
 		case 'e':
 			editor = optionarg;
-- 
2.9.2

-- 
Jilles Tjoelker

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`
  2016-08-04  5:54 [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Kylie McClain
  2016-08-04  5:54 ` [PATCH 2/2] histedit: Remove non-glibc fallback code Kylie McClain
@ 2016-08-04 17:17 ` Harald van Dijk
  2016-08-06  9:02   ` Seb
                     ` (2 more replies)
  1 sibling, 3 replies; 9+ messages in thread
From: Harald van Dijk @ 2016-08-04 17:17 UTC (permalink / raw)
  To: Kylie McClain, dash; +Cc: Kylie McClain

On 04/08/2016 07:54, Kylie McClain wrote:
> From: Kylie McClain <somasis@exherbo.org>
>
> nl, while specified in POSIX, is rather obscure and isn't provided by small
> coreutils implementations such as `busybox`. This while loop works just as
> well for our purposes.
...
> -sed 's/	-[a-z]*//' $temp2 | nl -ba -v0 |
> +sed 's/	-[a-z]*//' $temp2 | while read line;do \
> +	i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done |

$(( ... )) is mentioned in 
<https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Shell-Substitutions.html> 
as not universally supported, notably Solaris 10 /bin/sh does not have 
it. Given that dash was fairly recently changed to make it build on 
Solaris 9, it seems like a mistake to break that again.

Aside from that, i is such a common variable name that it seems risky to 
assume it is unset. I know I've set it myself in shell sessions that I 
ended up using for building dash. I never exported it, so it wouldn't 
break here, but it doesn't seem like a stretch that someone else does 
export it.

Cheers,
Harald van Dijk

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`
  2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk
@ 2016-08-06  9:02   ` Seb
  2016-08-07 10:17     ` Seb
  2016-08-06 16:51   ` Harald van Dijk
  2016-08-08 15:33   ` Herbert Xu
  2 siblings, 1 reply; 9+ messages in thread
From: Seb @ 2016-08-06  9:02 UTC (permalink / raw)
  To: dash; +Cc: Harald van Dijk

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

On Thu, Aug 04, 2016 at 07:17:47PM +0200, Harald van Dijk wrote:

> Given that dash was fairly recently changed to make it build on
> Solaris 9, it seems like a mistake to break that again.

Hello,

This is an attempt to simplify the current implementation. This
one does not require any temporary file anymore and relies only
on sort and (a rather basic usage of) awk.

I've checked the the Opensolaris' awk manual, trying to not
introduce some unsupported syntax. I still wonder about the
escaped newlines and the (currently commented) close()
statements, but both can be easily addressed. So, if someone
thinks it's worth testing...

++
Seb.

[-- Attachment #2: mkbuiltins --]
[-- Type: text/plain, Size: 2382 bytes --]

#!/bin/sh

LC_ALL=C      # Force the collate order of the builtins.
export LC_ALL # some shells may not support the "export FOO=bar" form.

awk '
(NF && ($1 !~ /^#/)) {
    # command [options] alias1 [[options] alias2] ...
    for (i = 2; i <= NF; i++) {
        mask = 0
        cmd = $1
        if ($i ~ /^-/) {
            if ($i ~ /n/)
                cmd = "NULL"
            if ($i ~ /s/)
                mask += 1
            if ($i ~ /[su]/)
                mask += 2
            if ($i ~ /a/)
                mask += 4
            i++
        }
        print $i, cmd, mask, $1
    }
}' $1 | sort -k1,1 | awk '
BEGIN {
    BUILTINS_H = "./builtins.h"
    BUILTINS_C = "./builtins.c"
    warn = "/*\n * This file was generated by the mkbuiltins program.\n */\n"
    print  warn >BUILTINS_H
    print  warn "\n#include \"shell.h\"" \
                "\n#include \"builtins.h\"\n" >BUILTINS_C
}
(!($NF in DEFINE)) {
    up = $NF # /bin/awk has no toupper() on Solaris.
    gsub(/a/, "A", up); gsub(/j/, "J", up); gsub(/s/, "S", up)
    gsub(/b/, "B", up); gsub(/k/, "K", up); gsub(/t/, "T", up)
    gsub(/c/, "C", up); gsub(/l/, "L", up); gsub(/u/, "U", up)
    gsub(/d/, "D", up); gsub(/m/, "M", up); gsub(/v/, "V", up)
    gsub(/e/, "E", up); gsub(/n/, "N", up); gsub(/w/, "W", up)
    gsub(/f/, "F", up); gsub(/o/, "O", up); gsub(/x/, "X", up)
    gsub(/g/, "G", up); gsub(/p/, "P", up); gsub(/y/, "Y", up)
    gsub(/h/, "H", up); gsub(/q/, "Q", up); gsub(/z/, "Z", up)
    gsub(/i/, "I", up); gsub(/r/, "R", up)


    print "#define "up" (builtincmd + "(NR-1)")" >BUILTINS_H
    print "int "$NF"(int, char **);" >BUILTINS_C
    DEFINE[$NF]
}
{
    CMD[NR] = "\""$1"\", "$2", "$3
}
END {
    print "\n#define NUMBUILTINS "NR"\n" \
          "\n#define BUILTIN_SPECIAL 0x1" \
          "\n#define BUILTIN_REGULAR 0x2" \
          "\n#define BUILTIN_ASSIGN  0x4\n" \
          "\nstruct builtincmd {" \
          "\n    const char *name;" \
          "\n    int (*builtin)(int, char **);" \
          "\n    unsigned flags;" \
          "\n};" \
          "\n\nextern const struct builtincmd builtincmd[];" >BUILTINS_H
    # close(BUILTINS_H) # not supported on Solaris ? 

    print "\nconst struct builtincmd builtincmd[] = {" >BUILTINS_C
    for (i = 1; i <= NR; i++)
        print "\t{ "CMD[i]" }," >BUILTINS_C
    print "};" >BUILTINS_C
    # close(BUILTINS_C)
}'

# EoF

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`
  2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk
  2016-08-06  9:02   ` Seb
@ 2016-08-06 16:51   ` Harald van Dijk
  2016-08-08 15:33   ` Herbert Xu
  2 siblings, 0 replies; 9+ messages in thread
From: Harald van Dijk @ 2016-08-06 16:51 UTC (permalink / raw)
  To: Kylie McClain, dash; +Cc: Kylie McClain

On 04/08/2016 19:17, Harald van Dijk wrote:
> On 04/08/2016 07:54, Kylie McClain wrote:
>> From: Kylie McClain <somasis@exherbo.org>
>>
>> nl, while specified in POSIX, is rather obscure and isn't provided by
>> small
>> coreutils implementations such as `busybox`. This while loop works
>> just as
>> well for our purposes.
> ...
>> -sed 's/    -[a-z]*//' $temp2 | nl -ba -v0 |
>> +sed 's/    -[a-z]*//' $temp2 | while read line;do \
>> +    i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done |
>
> $(( ... )) is mentioned in
> <https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Shell-Substitutions.html>
> as not universally supported, notably Solaris 10 /bin/sh does not have
> it. Given that dash was fairly recently changed to make it build on
> Solaris 9, it seems like a mistake to break that again.

I just realised that that particular change to make it build on Solaris 
9 involved avoiding running mkbuiltins using sh, choosing to use $SHELL 
instead, as $SHELL should already have been picked by configure as 
something more modern. So the use of $(( ... )) should probably not be a 
problem after all...

> Aside from that, i is such a common variable name that it seems risky to
> assume it is unset. I know I've set it myself in shell sessions that I
> ended up using for building dash. I never exported it, so it wouldn't
> break here, but it doesn't seem like a stretch that someone else does
> export it.

...as long as i is simply initialised before the loop.

Cheers,
Harald van Dijk

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`
  2016-08-06  9:02   ` Seb
@ 2016-08-07 10:17     ` Seb
  0 siblings, 0 replies; 9+ messages in thread
From: Seb @ 2016-08-07 10:17 UTC (permalink / raw)
  To: dash

On Sat, Aug 06, 2016 at 11:02:42AM +0200, Seb wrote:
> This is an attempt to simplify the current implementation [...]

BTW, I wonder if there is not a (virtual) bug with the current
mkbuiltins. Look at the TRUECMD macro in the generated builtins.h:

 #define TRUECMD (builtincmd + 1)

It doesn't point on "true" (mask=2, "special builtin") but on ":"
(mask=3, "standard builtin"), whatever the order of the aliases
you set in builtins.def.in. Maybe it's a mere problem with the
macro name ("TRUECMD" should rather be "COLONCMD"), but I don't
know if it is really an intended behaviour as the result is
finally a matter of sorting.

++
Seb.




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl`
  2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk
  2016-08-06  9:02   ` Seb
  2016-08-06 16:51   ` Harald van Dijk
@ 2016-08-08 15:33   ` Herbert Xu
  2 siblings, 0 replies; 9+ messages in thread
From: Herbert Xu @ 2016-08-08 15:33 UTC (permalink / raw)
  To: Harald van Dijk; +Cc: somasissounds, dash, somasis

Harald van Dijk <harald@gigawatt.nl> wrote:
> On 04/08/2016 07:54, Kylie McClain wrote:
>> From: Kylie McClain <somasis@exherbo.org>
>>
>> nl, while specified in POSIX, is rather obscure and isn't provided by small
>> coreutils implementations such as `busybox`. This while loop works just as
>> well for our purposes.
> ...
>> -sed 's/      -[a-z]*//' $temp2 | nl -ba -v0 |
>> +sed 's/      -[a-z]*//' $temp2 | while read line;do \
>> +     i=$(( ${i:--1} + 1 )); printf '%s %s\n' "${i}" "${line}";done |
> 
> $(( ... )) is mentioned in 
> <https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Shell-Substitutions.html> 
> as not universally supported, notably Solaris 10 /bin/sh does not have 
> it. Given that dash was fairly recently changed to make it build on 
> Solaris 9, it seems like a mistake to break that again.
> 
> Aside from that, i is such a common variable name that it seems risky to 
> assume it is unset. I know I've set it myself in shell sessions that I 
> ended up using for building dash. I never exported it, so it wouldn't 
> break here, but it doesn't seem like a stretch that someone else does 
> export it.

nl has been around forever.  I'm not taking this patch.

Cheers,
-- 
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] 9+ messages in thread

* Re: [PATCH 2/2] histedit: Remove non-glibc fallback code
  2016-08-04 15:59   ` Jilles Tjoelker
@ 2016-08-08 15:50     ` Jilles Tjoelker
  0 siblings, 0 replies; 9+ messages in thread
From: Jilles Tjoelker @ 2016-08-08 15:50 UTC (permalink / raw)
  To: Kylie McClain; +Cc: dash, Kylie McClain

On Thu, Aug 04, 2016 at 05:59:08PM +0200, Jilles Tjoelker wrote:
> >From 23da600dcff616662a93f307420d9142598e2cae Mon Sep 17 00:00:00 2001
> From: Jilles Tjoelker <jilles@stack.nl>
> Date: Thu, 4 Aug 2016 17:51:12 +0200
> Subject: [PATCH 1/2] [HISTEDIT] Stop depending on getopt reset feature.

> Instead, use our own nextopt() function.
> ---
>  src/histedit.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/src/histedit.c b/src/histedit.c
> index 94465d7..ec45065 100644
> --- a/src/histedit.c
> +++ b/src/histedit.c
> @@ -214,13 +214,8 @@ histcmd(int argc, char **argv)
>  	if (argc == 1)
>  		sh_error("missing history argument");
>  
> -#ifdef __GLIBC__
> -	optind = 0;
> -#else
> -	optreset = 1; optind = 1; /* initialize getopt */
> -#endif
>  	while (not_fcnumber(argv[optind]) &&
> -	      (ch = getopt(argc, argv, ":e:lnrs")) != -1)
> +	      (ch = nextopt(":e:lnrs")) != '\0')
>  		switch ((char)ch) {
>  		case 'e':
>  			editor = optionarg;

This is clearly wrong; not_fcnumber() should be passed *argptr instead
of something bogus depending on optind.

The fixed version is what FreeBSD sh has as of SVN r240541 but I have
not tested it in dash.

In any case, a side effect of this change is a small code size
reduction.

-- 
Jilles Tjoelker

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-08-08 15:50 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-04  5:54 [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Kylie McClain
2016-08-04  5:54 ` [PATCH 2/2] histedit: Remove non-glibc fallback code Kylie McClain
2016-08-04 15:59   ` Jilles Tjoelker
2016-08-08 15:50     ` Jilles Tjoelker
2016-08-04 17:17 ` [PATCH 1/2] mkbuiltins: Use a `while` loop rather than `nl` Harald van Dijk
2016-08-06  9:02   ` Seb
2016-08-07 10:17     ` Seb
2016-08-06 16:51   ` Harald van Dijk
2016-08-08 15:33   ` Herbert Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).