[PATCH 0/3] (Re-)instate some build guards
It would appear that some compile-time flags are used as if they were runtime flags, i.e. they are used in if (...) constructs, not in `#if ... #endif` guards. The effect *should* be the same, except when switching off optimization (e.g. for debugging) where dead code is not removed, possibly leading to build errors where features are disabled because the required functions are unavailable. Johannes Schindelin (3): copyfd: reinstate proper guard around munmap() inet_common: handle FEATURE_ETC_NETWORKS as a compile-time flag ash: use JOBS as the compile time option it actually is libbb/copyfd.c | 2 ++ libbb/inet_common.c | 5 - shell/ash.c | 12 +--- 3 files changed, 15 insertions(+), 4 deletions(-) base-commit: a03ac6067764549f4ae25f9a34e1ee9b0d2bb4f2 Published-As: https://github.com/dscho/busybox-w32/releases/tag/busybox-build-guards-v1 Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 busybox-build-guards-v1 -- 2.13.3.windows.1.13.gaf0c2223da0 ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH 2/3] inet_common: handle FEATURE_ETC_NETWORKS as a compile-time flag
This flag is used in #if ... #endif guards elsewhere and there is no
good reason to pretend that it is a runtime flag in just one case.
This probably was an oversight in a61cb92f2 (make /etc/network parsing
configurable. -200 bytes when off., 2007-06-19).
Besides, it fixes a compile problem on systems where the getnetbyaddr()
function is not available and the /etc/networks feature is disabled.
Signed-off-by: Johannes Schindelin
---
libbb/inet_common.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/libbb/inet_common.c b/libbb/inet_common.c
index 5b4a4a10b..f10ed29be 100644
--- a/libbb/inet_common.c
+++ b/libbb/inet_common.c
@@ -132,7 +132,9 @@ char* FAST_FUNC INET_rresolve(struct sockaddr_in *s_in, int
numeric, uint32_t ne
bb_error_msg("sockaddr2host_noport(%08x)", (unsigned)nip);
#endif
name = xmalloc_sockaddr2host_noport((void*)s_in);
- } else if (ENABLE_FEATURE_ETC_NETWORKS) {
+ }
+#if ENABLE_FEATURE_ETC_NETWORKS
+ else {
struct netent *np;
#ifdef DEBUG
bb_error_msg("getnetbyaddr(%08x)", (unsigned)ntohl(nip));
@@ -141,6 +143,7 @@ char* FAST_FUNC INET_rresolve(struct sockaddr_in *s_in, int
numeric, uint32_t ne
if (np)
name = xstrdup(np->n_name);
}
+#endif
if (!name)
name = xmalloc_sockaddr2dotted_noport((void*)s_in);
--
2.13.3.windows.1.13.gaf0c2223da0
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH 3/3] ash: use JOBS as the compile time option it actually is
Using the constant in a regular `if (...)` pretends that this is
anything but a compile time option. Let's use `#if JOBS` instead,
making it *much* clearer what `JOBS` actually is.
Incidentally, this change fixes the build in setups where there are
no headers defining WIFSTOPPED and WSTOPSIG (where JOBS has to be
set to 0).
This partially reverts 4700fb5be (ash: make dowait() a bit more
readable. Logic is unchanged, 2015-10-09).
Signed-off-by: Johannes Schindelin
---
shell/ash.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index 8c2098dd9..5c8216331 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -4022,15 +4022,19 @@ sprint_status48(char *s, int status, int sigonly)
col = 0;
if (!WIFEXITED(status)) {
- if (JOBS && WIFSTOPPED(status))
+#if JOBS
+ if (WIFSTOPPED(status))
st = WSTOPSIG(status);
else
+#endif
st = WTERMSIG(status);
if (sigonly) {
if (st == SIGINT || st == SIGPIPE)
goto out;
- if (JOBS && WIFSTOPPED(status))
+#if JOBS
+ if (WIFSTOPPED(status))
goto out;
+#endif
}
st &= 0x7f;
//TODO: use bbox's get_signame? strsignal adds ~600 bytes to text+rodata
@@ -4182,7 +4186,9 @@ dowait(int block, struct job *job)
goto out;
}
/* The process wasn't found in job list */
- if (JOBS && !WIFSTOPPED(status))
+#if JOBS
+ if (!WIFSTOPPED(status))
+#endif
jobless--;
out:
INT_ON;
--
2.13.3.windows.1.13.gaf0c2223da0
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH 1/3] copyfd: reinstate proper guard around munmap()
In 4c1392296 (Introduce FEATURE_COPYBUF_KB, 2007-12-02), a feature was introduced where a large stack was allocated via mmap(), and consequently released via munmap(). Since this is overkill for small stacks, the mmap()/munmap() code was guarded inside an #if CONFIG_FEATURE_COPYBUF_KB > 4 ... #endif guard. In pure Win32 API, there is no support to abuse mmap() to allocate a stack that way, therefore the code is simply disabled in busybox-w32. But 8d75d794e (libbb: use sendfile() to copy data between file descriptors, 2014-11-27) removed that guard around the munmap() call. That was most likely a mistake, as the corresponding mmap() call is *still* inside an equivalent #if ... #endif guard. Let's revert the mistaken change. Signed-off-by: Johannes Schindelin --- libbb/copyfd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libbb/copyfd.c b/libbb/copyfd.c index 7e3531903..eedf03790 100644 --- a/libbb/copyfd.c +++ b/libbb/copyfd.c @@ -119,8 +119,10 @@ static off_t bb_full_fd_action(int src_fd, int dst_fd, off_t size) } out: +#if CONFIG_FEATURE_COPYBUF_KB > 4 if (buffer_size > 4 * 1024) munmap(buffer, buffer_size); +#endif return status ? -1 : total; } -- 2.13.3.windows.1.13.gaf0c2223da0 ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 3/3] ash: use JOBS as the compile time option it actually is
Hi Xabier, On Fri, 14 Jul 2017, Xabier Oneca -- xOneca wrote: > 2017-07-14 16:11 GMT+02:00 Johannes Schindelin : > > @@ -4182,7 +4186,9 @@ dowait(int block, struct job *job) > > goto out; > > } > > /* The process wasn't found in job list */ > > - if (JOBS && !WIFSTOPPED(status)) > > +#if JOBS > > + if (!WIFSTOPPED(status)) > > +#endif > > jobless--; > > Here the logic does change. Whoops, you're right, the #endif should enclose the jobless--; statement. Will fix, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 3/3] ash: use JOBS as the compile time option it actually is
Hi Denys, On Fri, 14 Jul 2017, Denys Vlasenko wrote: > On Fri, Jul 14, 2017 at 4:11 PM, Johannes Schindelin > wrote: > > Using the constant in a regular `if (...)` pretends that this is > > anything but a compile time option. Let's use `#if JOBS` instead, > > making it *much* clearer what `JOBS` actually is. > > > > Incidentally, this change fixes the build in setups where there are > > no headers defining WIFSTOPPED and WSTOPSIG (where JOBS has to be > > set to 0). > > > > This partially reverts 4700fb5be (ash: make dowait() a bit more > > readable. Logic is unchanged, 2015-10-09). > > Some people (not me) have the completely opposite POV > and used to ask me to remove excessive #ifs. > > It's difficult to find a middle ground here... Sure. But when something cannot build because of missing symbols, and there is not even a compile time flag to fix it (even when the compile time flag clearly switches on/off users of the symbols in question), I feel that there is no need for middle ground. If it fixes the build, it fixes the build. The patches in this patch series fix the build here. Ciao, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 1/3] copyfd: reinstate proper guard around munmap()
Hi Denys, On Fri, 14 Jul 2017, Denys Vlasenko wrote: > On Fri, Jul 14, 2017 at 4:11 PM, Johannes Schindelin > wrote: > > In 4c1392296 (Introduce FEATURE_COPYBUF_KB, 2007-12-02), a feature was > > introduced where a large stack was allocated via mmap(), and > > consequently released via munmap(). Since this is overkill for small > > stacks, the mmap()/munmap() code was guarded inside an #if > > CONFIG_FEATURE_COPYBUF_KB > 4 ... #endif guard. > > > > In pure Win32 API, there is no support to abuse mmap() to allocate a > > stack that way, therefore the code is simply disabled in busybox-w32. > > > > But 8d75d794e (libbb: use sendfile() to copy data between file > > descriptors, 2014-11-27) removed that guard around the munmap() call. > > That was most likely a mistake, as the corresponding mmap() call is > > *still* inside an equivalent #if ... #endif guard. > > > > Let's revert the mistaken change. > > > > Signed-off-by: Johannes Schindelin > > --- > > libbb/copyfd.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libbb/copyfd.c b/libbb/copyfd.c > > index 7e3531903..eedf03790 100644 > > --- a/libbb/copyfd.c > > +++ b/libbb/copyfd.c > > @@ -119,8 +119,10 @@ static off_t bb_full_fd_action(int src_fd, int dst_fd, > > off_t size) > > } > > out: > > > > +#if CONFIG_FEATURE_COPYBUF_KB > 4 > > if (buffer_size > 4 * 1024) > > munmap(buffer, buffer_size); > > +#endif > > If CONFIG_FEATURE_COPYBUF_KB <= 4, then buffer_size is a constant < 4k > and compiler will eliminate the code anyway. That would be correct only when optimizing. In a debug build, in a setup without mmap()/munmap(), the current code simply fails to build. My patch fixes that build failure. Ciao, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2 2/2] ash: use JOBS as the compile time option it actually is
Using the constant in a regular `if (...)` pretends that this is
anything but a compile time option. Let's use `#if JOBS` instead,
making it *much* clearer what `JOBS` actually is.
Incidentally, this change fixes the build in setups where there are
no headers defining WIFSTOPPED and WSTOPSIG (where JOBS has to be
set to 0).
This partially reverts 4700fb5be (ash: make dowait() a bit more
readable. Logic is unchanged, 2015-10-09).
Signed-off-by: Johannes Schindelin
---
shell/ash.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index 8c2098dd9..b0c7dac54 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -4022,15 +4022,19 @@ sprint_status48(char *s, int status, int sigonly)
col = 0;
if (!WIFEXITED(status)) {
- if (JOBS && WIFSTOPPED(status))
+#if JOBS
+ if (WIFSTOPPED(status))
st = WSTOPSIG(status);
else
+#endif
st = WTERMSIG(status);
if (sigonly) {
if (st == SIGINT || st == SIGPIPE)
goto out;
- if (JOBS && WIFSTOPPED(status))
+#if JOBS
+ if (WIFSTOPPED(status))
goto out;
+#endif
}
st &= 0x7f;
//TODO: use bbox's get_signame? strsignal adds ~600 bytes to text+rodata
@@ -4182,8 +4186,10 @@ dowait(int block, struct job *job)
goto out;
}
/* The process wasn't found in job list */
- if (JOBS && !WIFSTOPPED(status))
+#if JOBS
+ if (!WIFSTOPPED(status))
jobless--;
+#endif
out:
INT_ON;
--
2.13.3.windows.1.13.gaf0c2223da0
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2 0/2] (Re-)instate some build guards
It would appear that some compile-time flags are used as if they were runtime flags, i.e. they are used in if (...) constructs, not in `#if ... #endif` guards. The effect *should* be the same, except when switching off optimization (e.g. for debugging) where dead code is not removed, possibly leading to build errors where features are disabled because the required functions are unavailable. Changes since v1: - enclosed also the `jobless--;` statement in the #if JOBS ... #endif that clearly was part of the `if()` already inside that clause. - skipped 2/3, as it was already applied as 7d7c7bb22 (libbb: hide getnetbyaddr() inside "#if ENABLE_FEATURE_ETC_NETWORKS" block, 2017-07-14) (silently dropping my authorship, and smooshing it together with plenty of `#if DEBUG bb_error_msg(...) #endif` => `dbg(...)` conversions, making it hard to see the functional change) Johannes Schindelin (2): copyfd: reinstate proper guard around munmap() ash: use JOBS as the compile time option it actually is libbb/copyfd.c | 2 ++ shell/ash.c| 12 +--- 2 files changed, 11 insertions(+), 3 deletions(-) base-commit: 7d7c7bb2205b92359ac88f3469d3af672e2be929 Published-As: https://github.com/dscho/busybox-w32/releases/tag/busybox-build-guards-v2 Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 busybox-build-guards-v2 Interdiff vs v1: diff --git a/shell/ash.c b/shell/ash.c index 5c8216331..b0c7dac54 100644 --- a/shell/ash.c +++ b/shell/ash.c @@ -4188,8 +4188,8 @@ dowait(int block, struct job *job) /* The process wasn't found in job list */ #if JOBS if (!WIFSTOPPED(status)) -#endif jobless--; +#endif out: INT_ON; -- 2.13.3.windows.1.13.gaf0c2223da0 ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2 1/2] copyfd: reinstate proper guard around munmap()
In 4c1392296 (Introduce FEATURE_COPYBUF_KB, 2007-12-02), a feature was introduced where a large stack was allocated via mmap(), and consequently released via munmap(). Since this is overkill for small stacks, the mmap()/munmap() code was guarded inside an #if CONFIG_FEATURE_COPYBUF_KB > 4 ... #endif guard. In pure Win32 API, there is no support to abuse mmap() to allocate a stack that way, therefore the code is simply disabled in busybox-w32. But 8d75d794e (libbb: use sendfile() to copy data between file descriptors, 2014-11-27) removed that guard around the munmap() call. That was most likely a mistake, as the corresponding mmap() call is *still* inside an equivalent #if ... #endif guard. Let's revert the mistaken change. Signed-off-by: Johannes Schindelin --- libbb/copyfd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libbb/copyfd.c b/libbb/copyfd.c index 7e3531903..eedf03790 100644 --- a/libbb/copyfd.c +++ b/libbb/copyfd.c @@ -119,8 +119,10 @@ static off_t bb_full_fd_action(int src_fd, int dst_fd, off_t size) } out: +#if CONFIG_FEATURE_COPYBUF_KB > 4 if (buffer_size > 4 * 1024) munmap(buffer, buffer_size); +#endif return status ? -1 : total; } -- 2.13.3.windows.1.13.gaf0c2223da0 ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] ash: avoid GLIBC'ism %m
GLIBC's printf() family supports the extension where the placeholder %m
is interpolated to strerror(errno).
This is not portable. So don't use it. (It was only used in ash's source
code to begin with.)
Signed-off-by: Johannes Schindelin
---
Published-As:
https://github.com/dscho/busybox-w32/releases/tag/busybox-glibc-ism-v1
Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32
busybox-glibc-ism-v1
shell/ash.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index b0c7dac54..e2ff15767 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -3809,8 +3809,10 @@ freejob(struct job *jp)
static void
xtcsetpgrp(int fd, pid_t pgrp)
{
- if (tcsetpgrp(fd, pgrp))
- ash_msg_and_raise_error("can't set tty process group (%m)");
+ if (tcsetpgrp(fd, pgrp)) {
+ const char *err = strerror(errno);
+ ash_msg_and_raise_error("can't set tty process group (%s)",
err);
+ }
}
/*
@@ -5343,7 +5345,7 @@ savefd(int from)
err = newfd < 0 ? errno : 0;
if (err != EBADF) {
if (err)
- ash_msg_and_raise_error("%d: %m", from);
+ ash_msg_and_raise_error("%d: %s", from,
strerror(errno));
close(from);
fcntl(newfd, F_SETFD, FD_CLOEXEC);
}
@@ -5358,7 +5360,7 @@ dup2_or_raise(int from, int to)
newfd = (from != to) ? dup2(from, to) : to;
if (newfd < 0) {
/* Happens when source fd is not open: try "echo >&99" */
- ash_msg_and_raise_error("%d: %m", from);
+ ash_msg_and_raise_error("%d: %s", from, strerror(errno));
}
return newfd;
}
@@ -5489,7 +5491,7 @@ redirect(union node *redir, int flags)
/* "echo >&10" and 10 is a fd opened to a sh script? */
if (is_hidden_fd(sv, right_fd)) {
errno = EBADF; /* as if it is closed */
- ash_msg_and_raise_error("%d: %m", right_fd);
+ ash_msg_and_raise_error("%d: %s", right_fd,
strerror(errno));
}
newfd = -1;
} else {
@@ -5523,7 +5525,7 @@ redirect(union node *redir, int flags)
if (newfd >= 0)
close(newfd);
errno = i;
- ash_msg_and_raise_error("%d: %m", fd);
+ ash_msg_and_raise_error("%d: %s", fd,
strerror(errno));
/* NOTREACHED */
}
/* EBADF: it is not open - good, remember to
close it */
base-commit: 68e980545af6a8ffb2980f94a6edac4dd89940f3
--
2.13.3.windows.1.13.gaf0c2223da0
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2 2/2] ash: avoid GLIBC'ism %m
GLIBC's printf() family supports the extension where the placeholder %m
is interpolated to strerror(errno).
This is not portable. So don't use it. (It was only used in ash's source
code to begin with.)
Let's use the newly-introduced ash_perror_msg_and_raise_error() helper
function instead.
Helped-by: Kang-Che Sung
Signed-off-by: Johannes Schindelin
---
shell/ash.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index c21b25ab7..51d13e537 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -3826,7 +3826,7 @@ static void
xtcsetpgrp(int fd, pid_t pgrp)
{
if (tcsetpgrp(fd, pgrp))
- ash_msg_and_raise_error("can't set tty process group (%m)");
+ ash_perror_msg_and_raise_error("can't set tty process group");
}
/*
@@ -5359,7 +5359,7 @@ savefd(int from)
err = newfd < 0 ? errno : 0;
if (err != EBADF) {
if (err)
- ash_msg_and_raise_error("%d: %m", from);
+ ash_perror_msg_and_raise_error("%d", from);
close(from);
fcntl(newfd, F_SETFD, FD_CLOEXEC);
}
@@ -5374,7 +5374,7 @@ dup2_or_raise(int from, int to)
newfd = (from != to) ? dup2(from, to) : to;
if (newfd < 0) {
/* Happens when source fd is not open: try "echo >&99" */
- ash_msg_and_raise_error("%d: %m", from);
+ ash_perror_msg_and_raise_error("%d", from);
}
return newfd;
}
@@ -5505,7 +5505,7 @@ redirect(union node *redir, int flags)
/* "echo >&10" and 10 is a fd opened to a sh script? */
if (is_hidden_fd(sv, right_fd)) {
errno = EBADF; /* as if it is closed */
- ash_msg_and_raise_error("%d: %m", right_fd);
+ ash_perror_msg_and_raise_error("%d", right_fd);
}
newfd = -1;
} else {
@@ -5539,7 +5539,7 @@ redirect(union node *redir, int flags)
if (newfd >= 0)
close(newfd);
errno = i;
- ash_msg_and_raise_error("%d: %m", fd);
+ ash_perror_msg_and_raise_error("%d",
fd);
/* NOTREACHED */
}
/* EBADF: it is not open - good, remember to
close it */
--
2.13.3.windows.1.13.gaf0c2223da0
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2 0/2] Avoid GLIBC'ism "%m"
As pointed out by Kang-Che Sung and verified by Jody Bruchon, the "%m"
format placeholder is really not in any open standard.
The previous iteration of these changes tried to replace the "%m" by
explicit strerror(errno) manually, but as Kang-Che Sung indicated, this
is not very elegant.
Sadly, we cannot use bb_perror_msg() here because the functionality of
that helper is very different (and lacks support for ash's TRACE).
Happily, I could easily follow bb_perror_msg()'s example and introduce
a new helper in ash's source code.
Johannes Schindelin (2):
ash: introduce ash_perror_msg_and_raise_error()
ash: avoid GLIBC'ism %m
shell/ash.c | 49 +
1 file changed, 33 insertions(+), 16 deletions(-)
base-commit: 1ef3ce91c70cb6a536438132d3202ccb3eddadbc
Published-As:
https://github.com/dscho/busybox-w32/releases/tag/busybox-glibc-ism-v2
Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32
busybox-glibc-ism-v2
Interdiff vs v1:
diff --git a/shell/ash.c b/shell/ash.c
index 9038ab576..51d13e537 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -1200,7 +1200,7 @@ showtree(union node *n)
/* Parser data */
/*
- * ash_vmsg() needs parsefile->fd, hence parsefile definition is moved up.
+ * ash_verror_msg() needs parsefile->fd, hence parsefile definition is moved
up.
*/
struct strlist {
struct strlist *next;
@@ -1253,7 +1253,7 @@ static struct strlist *cmdenviron; /* environment
for builtin command */
/* Message printing */
static void
-ash_vmsg(const char *msg, va_list ap)
+ash_verror_msg(const char *msg, va_list ap, const char *strerr)
{
fprintf(stderr, "%s: ", arg0);
if (commandname) {
@@ -1263,6 +1263,8 @@ ash_vmsg(const char *msg, va_list ap)
fprintf(stderr, "line %d: ", startlinno);
}
vfprintf(stderr, msg, ap);
+ if (strerr)
+ vfprintf(stderr, ": ", strerr);
newline_and_flush(stderr);
}
@@ -1271,19 +1273,20 @@ ash_vmsg(const char *msg, va_list ap)
* is not NULL then error prints an error message using printf style
* formatting. It then raises the error exception.
*/
-static void ash_vmsg_and_raise(int, const char *, va_list) NORETURN;
-static void
-ash_vmsg_and_raise(int cond, const char *msg, va_list ap)
+static void ash_verror_msg_and_raise(int, const char *, va_list, const char *)
+ NORETURN;
+static void ash_verror_msg_and_raise(int cond, const char *msg, va_list ap,
+ const char *strerr)
{
#if DEBUG
if (msg) {
- TRACE(("ash_vmsg_and_raise(%d):", cond));
+ TRACE(("ash_verror_msg_and_raise(%d):", cond));
TRACEV((msg, ap));
} else
- TRACE(("ash_vmsg_and_raise(%d):NULL\n", cond));
+ TRACE(("ash_verror_msg_and_raise(%d):NULL\n", cond));
if (msg)
#endif
- ash_vmsg(msg, ap);
+ ash_verror_msg(msg, ap, strerr);
flush_stdout_stderr();
raise_exception(cond);
@@ -1299,7 +1302,21 @@ ash_msg_and_raise_error(const char *msg, ...)
exitstatus = 2;
va_start(ap, msg);
- ash_vmsg_and_raise(EXERROR, msg, ap);
+ ash_verror_msg_and_raise(EXERROR, msg, ap, NULL);
+ /* NOTREACHED */
+ va_end(ap);
+}
+
+static void ash_perror_msg_and_raise_error(const char *, ...) NORETURN;
+static void
+ash_perror_msg_and_raise_error(const char *msg, ...)
+{
+ va_list ap;
+
+ exitstatus = 2;
+
+ va_start(ap, msg);
+ ash_verror_msg_and_raise(EXERROR, msg, ap, errno ? strerror(errno) :
NULL);
/* NOTREACHED */
va_end(ap);
}
@@ -1319,7 +1336,7 @@ ash_msg_and_raise(int cond, const char *msg, ...)
va_list ap;
va_start(ap, msg);
- ash_vmsg_and_raise(cond, msg, ap);
+ ash_verror_msg_and_raise(cond, msg, ap, NULL);
/* NOTREACHED */
va_end(ap);
}
@@ -1333,7 +1350,7 @@ ash_msg(const char *fmt, ...)
va_list ap;
va_start(ap, fmt);
- ash_vmsg(fmt, ap);
+ ash_verror_msg(fmt, ap, NULL);
va_end(ap);
}
@@ -3808,10 +3825,8 @@ freejob(struct job *jp)
static void
xtcsetpgrp(int fd, pid_t pgrp)
{
- if (tcsetpgrp(fd, pgrp)) {
- const char *err = strerror(errno);
- ash_msg_and_raise_error("can't set tty process group (%s)",
err);
- }
+ if (tcsetpgrp(fd, pgrp))
+ ash_perror_msg_and_raise_error("can't set tty process group");
}
/*
@@ -5344,7 +5359,7 @@ savefd(int from)
err = newfd < 0 ? errno : 0;
if (err != EBADF) {
if (err)
- ash_msg_and_raise_error("%d: %s", from,
strerror(errno));
+
[PATCH v2 1/2] ash: introduce ash_perror_msg_and_raise_error()
This new helper does the very same thing as ash_msg_and_raise_error()
with one additional feature: if errno is not 0, it appends ": "
(where is the output of strerror(errno)).
Helped-by: Kang-Che Sung
Signed-off-by: Johannes Schindelin
---
shell/ash.c | 39 ---
1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index 9d81f4ba8..c21b25ab7 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -1200,7 +1200,7 @@ showtree(union node *n)
/* Parser data */
/*
- * ash_vmsg() needs parsefile->fd, hence parsefile definition is moved up.
+ * ash_verror_msg() needs parsefile->fd, hence parsefile definition is moved
up.
*/
struct strlist {
struct strlist *next;
@@ -1253,7 +1253,7 @@ static struct strlist *cmdenviron; /* environment for
builtin command */
/* Message printing */
static void
-ash_vmsg(const char *msg, va_list ap)
+ash_verror_msg(const char *msg, va_list ap, const char *strerr)
{
fprintf(stderr, "%s: ", arg0);
if (commandname) {
@@ -1263,6 +1263,8 @@ ash_vmsg(const char *msg, va_list ap)
fprintf(stderr, "line %d: ", startlinno);
}
vfprintf(stderr, msg, ap);
+ if (strerr)
+ vfprintf(stderr, ": ", strerr);
newline_and_flush(stderr);
}
@@ -1271,19 +1273,20 @@ ash_vmsg(const char *msg, va_list ap)
* is not NULL then error prints an error message using printf style
* formatting. It then raises the error exception.
*/
-static void ash_vmsg_and_raise(int, const char *, va_list) NORETURN;
-static void
-ash_vmsg_and_raise(int cond, const char *msg, va_list ap)
+static void ash_verror_msg_and_raise(int, const char *, va_list, const char *)
+ NORETURN;
+static void ash_verror_msg_and_raise(int cond, const char *msg, va_list ap,
+ const char *strerr)
{
#if DEBUG
if (msg) {
- TRACE(("ash_vmsg_and_raise(%d):", cond));
+ TRACE(("ash_verror_msg_and_raise(%d):", cond));
TRACEV((msg, ap));
} else
- TRACE(("ash_vmsg_and_raise(%d):NULL\n", cond));
+ TRACE(("ash_verror_msg_and_raise(%d):NULL\n", cond));
if (msg)
#endif
- ash_vmsg(msg, ap);
+ ash_verror_msg(msg, ap, strerr);
flush_stdout_stderr();
raise_exception(cond);
@@ -1299,7 +1302,21 @@ ash_msg_and_raise_error(const char *msg, ...)
exitstatus = 2;
va_start(ap, msg);
- ash_vmsg_and_raise(EXERROR, msg, ap);
+ ash_verror_msg_and_raise(EXERROR, msg, ap, NULL);
+ /* NOTREACHED */
+ va_end(ap);
+}
+
+static void ash_perror_msg_and_raise_error(const char *, ...) NORETURN;
+static void
+ash_perror_msg_and_raise_error(const char *msg, ...)
+{
+ va_list ap;
+
+ exitstatus = 2;
+
+ va_start(ap, msg);
+ ash_verror_msg_and_raise(EXERROR, msg, ap, errno ? strerror(errno) :
NULL);
/* NOTREACHED */
va_end(ap);
}
@@ -1319,7 +1336,7 @@ ash_msg_and_raise(int cond, const char *msg, ...)
va_list ap;
va_start(ap, msg);
- ash_vmsg_and_raise(cond, msg, ap);
+ ash_verror_msg_and_raise(cond, msg, ap, NULL);
/* NOTREACHED */
va_end(ap);
}
@@ -1333,7 +1350,7 @@ ash_msg(const char *fmt, ...)
va_list ap;
va_start(ap, fmt);
- ash_vmsg(fmt, ap);
+ ash_verror_msg(fmt, ap, NULL);
va_end(ap);
}
--
2.13.3.windows.1.13.gaf0c2223da0
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] ash: avoid GLIBC'ism %m
Hi Denys, On Fri, 21 Jul 2017, Denys Vlasenko wrote: > On Wed, Jul 19, 2017 at 3:47 AM, Jody Bruchon wrote: > > On 2017-07-18 9:15 PM, Kang-Che Sung wrote: > >> > >> On Wed, Jul 19, 2017 at 2:11 AM, Markus Gothe > >> wrote: > >>> > >>> Actually last time I checked ‘%m’ is POSIX contrary to glibc’s deprecated > >>> '%a’. However, I agree that it should not be used since at least uClibc > >>> can > >>> be built without support for it. > >> > >> How come %m is POSIX when I didn't see any mention of it in this page? > >> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html > > > > It does not appear to be part of POSIX or the Single UNIX Specification. The > > glibc man page (as of 2016-12-12) even indicates that it is a glibc-specific > > extension: > > > > *m *(Glibc extension; supported by uClibc and musl.) Print output of > > /strerror(errno)/. No argument is required. > > This sounds like every libc has already conceded to implementing it. > > Let's benefit from it then? No, not every libc. I would not have spent the time and effort to develop the patch, contribute it, rework it and contribute a second iteration if it was not for a good reason now, would I. Ciao, Johannes___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2 1/2] ash: introduce ash_perror_msg_and_raise_error()
Hi Ron, On Sun, 23 Jul 2017, Ron Yorston wrote: > Johannes Schindelin wrote: > >@@ -1263,6 +1263,8 @@ ash_vmsg(const char *msg, va_list ap) > > fprintf(stderr, "line %d: ", startlinno); > > } > > vfprintf(stderr, msg, ap); > >+if (strerr) > >+vfprintf(stderr, ": ", strerr); > > newline_and_flush(stderr); > > } > > > > I think that should be: > > >+if (strerr) > >+fprintf(stderr, ": %s", strerr); Oops, you're right! Funny how GCC did not catch that error. Thanks, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v3 0/2] Avoid GLIBC'ism "%m"
As pointed out by Kang-Che Sung and verified by Jody Bruchon, the "%m" format placeholder is really not in any open standard. Also: contrary to Denys' assumption, there are libc versions out there which did not follow GLIBC's example to implement this non-POSIX placeholder. That must be the reason why nothing else in BusyBox' source code relies on %m but uses helpers such as bb_perror_msg() instead. It is very easy, and a good readability improvement, too, to introduce a new helper in ash's source code. Changes since v2: - fixed an erronous vfprintf() that does not take a va_list (thanks, Ron). - inserted the missing %s in the same fprintf() statement. Johannes Schindelin (2): ash: introduce ash_perror_msg_and_raise_error() ash: avoid GLIBC'ism %m shell/ash.c | 49 + 1 file changed, 33 insertions(+), 16 deletions(-) base-commit: be669fa1fdff6f751c8cdd3fc18a9fa7a7f46cd3 Published-As: https://github.com/dscho/busybox-w32/releases/tag/busybox-glibc-ism-v3 Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 busybox-glibc-ism-v3 Interdiff vs v2: diff --git a/shell/ash.c b/shell/ash.c index 110f9bb21..0fde5791d 100644 --- a/shell/ash.c +++ b/shell/ash.c @@ -1264,7 +1264,7 @@ ash_verror_msg(const char *msg, va_list ap, const char *strerr) } vfprintf(stderr, msg, ap); if (strerr) - vfprintf(stderr, ": ", strerr); + fprintf(stderr, ": %s", strerr); newline_and_flush(stderr); } -- 2.13.3.windows.1.13.gaf0c2223da0 ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] ash: avoid GLIBC'ism %m
Hi Denys, On Sun, 23 Jul 2017, Denys Vlasenko wrote: > On Sat, Jul 22, 2017 at 8:56 PM, Johannes Schindelin > wrote: > > On Fri, 21 Jul 2017, Denys Vlasenko wrote: > > > >> On Wed, Jul 19, 2017 at 3:47 AM, Jody Bruchon wrote: > >> > On 2017-07-18 9:15 PM, Kang-Che Sung wrote: > >> >> > >> >> On Wed, Jul 19, 2017 at 2:11 AM, Markus Gothe > >> >> wrote: > >> >>> > >> >>> Actually last time I checked ‘%m’ is POSIX contrary to glibc’s > >> >>> deprecated > >> >>> '%a’. However, I agree that it should not be used since at least > >> >>> uClibc can > >> >>> be built without support for it. > >> >> > >> >> How come %m is POSIX when I didn't see any mention of it in this page? > >> >> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html > >> > > >> > It does not appear to be part of POSIX or the Single UNIX Specification. > >> > The > >> > glibc man page (as of 2016-12-12) even indicates that it is a > >> > glibc-specific > >> > extension: > >> > > >> > *m *(Glibc extension; supported by uClibc and musl.) Print output of > >> > /strerror(errno)/. No argument is required. > >> > >> This sounds like every libc has already conceded to implementing it. > >> > >> Let's benefit from it then? > > > > No, not every libc. I would not have spent the time and effort to develop > > the patch, contribute it, rework it and contribute a second iteration if > > it was not for a good reason now, would I. > > Good point. > What libc is that? MSVC runtime. I see that Ron got a cleaner patch in already. Thanks, Johannes___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] ash: remove no-longer-used variable
As of 035486c75 (ash: significant overhaul of redirect saving logic,
2017-07-31), the sv_pos variable is no longer used (just assigned to,
with no further effect).
Let's just remove it.
Signed-off-by: Johannes Schindelin
---
Published-As:
https://github.com/dscho/busybox-w32/releases/tag/busybox-unused-sv_pos-v1
Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32
busybox-unused-sv_pos-v1
shell/ash.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index 8c9f4adc6..8b81c42a9 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -5544,12 +5544,10 @@ static void
redirect(union node *redir, int flags)
{
struct redirtab *sv;
- int sv_pos;
if (!redir)
return;
- sv_pos = 0;
sv = NULL;
INT_OFF;
if (flags & REDIR_PUSH)
base-commit: 4dc86699b57ff35c287ca396d562ec206776694a
--
2.13.3.windows.1.1055.g9b9ea5f2d78
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] ash: support BASH_XTRACEFD when built Bash compatibly
There is a very useful feature in Bash where you can redirect the trace
enabled by `set -x` to a file descriptor *different* than 2. This comes
in particularly handy when validating the error output of commands, say,
in Git's test suite, while tracing at the same time.
It is such a useful feature, and very easily implemented.
Signed-off-by: Johannes Schindelin
---
shell/ash.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/shell/ash.c b/shell/ash.c
index 8c9f4adc6..1d1596fec 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -182,6 +182,8 @@
#define IF_BASH_PATTERN_SUBST IF_ASH_BASH_COMPAT
#defineBASH_SUBSTR ENABLE_ASH_BASH_COMPAT
#define IF_BASH_SUBSTR IF_ASH_BASH_COMPAT
+#defineBASH_XTRACEFDENABLE_ASH_BASH_COMPAT
+#define IF_BASH_XTRACEFDIF_ASH_BASH_COMPAT
/* [[ EXPR ]] */
#defineBASH_TEST2 (ENABLE_ASH_BASH_COMPAT * ENABLE_ASH_TEST)
#defineBASH_SOURCE ENABLE_ASH_BASH_COMPAT
@@ -9740,6 +9742,7 @@ evalcommand(union node *cmd, int flags)
int status;
char **nargv;
smallint cmd_is_exec;
+ IF_BASH_XTRACEFD(const char *xtracefd;)
/* First expand the arguments. */
TRACE(("evalcommand(0x%lx, %d) called\n", (long)cmd, flags));
@@ -9791,6 +9794,10 @@ evalcommand(union node *cmd, int flags)
expredir(cmd->ncmd.redirect);
redir_stop = pushredir(cmd->ncmd.redirect);
+#ifdef BASH_XTRACEFD
+ xtracefd = lookupvar("BASH_XTRACEFD");
+ if (!xtracefd || (preverrout_fd = atoi(xtracefd)) < 0)
+#endif
preverrout_fd = 2;
status = redirectsafe(cmd->ncmd.redirect, REDIR_PUSH | REDIR_SAVEFD2);
base-commit: 4dc86699b57ff35c287ca396d562ec206776694a
--
2.13.3.windows.1.1055.g9b9ea5f2d78
Published-As:
https://github.com/dscho/busybox-w32/releases/tag/busybox-xtracefd-v1
Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 busybox-xtracefd-v1
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] ash: avoid GLIBC'ism %m
Hi Markus, On Wed, 19 Jul 2017, Markus Gothe wrote: > However it should be noted that this [scanf's %m format] seems not to be > the same as glibc %m: "To use the dynamic allocation conversion > specifier in C99 and C11, specify m as a length modifier as per > POSIX.1-2008. That is, use %ms or %m[…]." It would have been very surprising if scanf could interpret %m as reading strerror(errno)... How would that work, to begin with? Ciao, Johannes___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] Silence misguided GCC warning about alignment issues
When compiling xz_dec_stream.c with GCC 7.1.0, it complains thusly: In function 'dec_stream_footer': error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing] if (xz_crc32(s->temp.buf + 4, 6, 0) != get_le32(s->temp.buf)) ^~ The thing is, the `buf` field was put just after two fields of type size_t for the express purpose of avoiding alignment issues, as per the comment above the `temp` struct. Meaning: GCC gets this all wrong and should not complain. So let's force it to be quiet for a couple of lines. Signed-off-by: Johannes Schindelin --- archival/libarchive/unxz/xz_dec_stream.c | 12 1 file changed, 12 insertions(+) diff --git a/archival/libarchive/unxz/xz_dec_stream.c b/archival/libarchive/unxz/xz_dec_stream.c index bf791055b..8131ee30a 100644 --- a/archival/libarchive/unxz/xz_dec_stream.c +++ b/archival/libarchive/unxz/xz_dec_stream.c @@ -423,6 +423,15 @@ static enum xz_ret XZ_FUNC dec_stream_footer(struct xz_dec *s) if (!memeq(s->temp.buf + 10, FOOTER_MAGIC, FOOTER_MAGIC_SIZE)) return XZ_DATA_ERROR; +#if defined(__GNUC__) + /* +* The temp.buf field is put just after two fields of type size_t for +* the express purpose of avoiding alignment issues. But GCC complains +* about it nevertheless... so: shut GCC up for a few lines. +*/ +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wstrict-aliasing" +#endif if (xz_crc32(s->temp.buf + 4, 6, 0) != get_le32(s->temp.buf)) return XZ_DATA_ERROR; @@ -433,6 +442,9 @@ static enum xz_ret XZ_FUNC dec_stream_footer(struct xz_dec *s) */ if ((s->index.size >> 2) != get_le32(s->temp.buf + 4)) return XZ_DATA_ERROR; +#if defined(__GNUC__) +#pragma GCC diagnostic pop +#endif if (s->temp.buf[8] != 0 || s->temp.buf[9] != s->check_type) return XZ_DATA_ERROR; base-commit: 4dea1edd08a45c5987448719e56ee61a20fb9210 -- 2.14.0.windows.1.2.g0f3342804fc Published-As: https://github.com/dscho/busybox-w32/releases/tag/busybox-type-punned-warning-v1 Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 busybox-type-punned-warning-v1 ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH] ash/hush: implement -d DELIM option for `read`
The POSIX standard only requires the `read` builtin to handle `-r`:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/read.html
However, Bash introduced the option `-d ` to override IFS for
just one invocation, and it is quite useful.
It is also super easy to implement in BusyBox' ash, so let's do that.
The motivation: This option is used by Git's test suite.
Signed-off-by: Johannes Schindelin
---
Published-As:
https://github.com/dscho/busybox-w32/releases/tag/busybox-read-d-v1
Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 busybox-read-d-v1
shell/ash.c | 11 ---
shell/hush.c | 7 +--
shell/shell_common.c | 10 +++---
shell/shell_common.h | 3 ++-
4 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index e8f3ed26b..b4f6dc3df 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -13402,10 +13402,10 @@ letcmd(int argc UNUSED_PARAM, char **argv)
* -p PROMPT Display PROMPT on stderr (if input is from tty)
* -t SECONDS Timeout after SECONDS (tty or pipe only)
* -u FD Read from given FD instead of fd 0
+ * -d DELIMEnd on DELIM char, not newline
* This uses unbuffered input, which may be avoidable in some cases.
* TODO: bash also has:
* -a ARRAYRead into array[0],[1],etc
- * -d DELIMEnd on DELIM char, not newline
* -e Use line editing (tty only)
*/
static int FAST_FUNC
@@ -13415,11 +13415,12 @@ readcmd(int argc UNUSED_PARAM, char **argv
UNUSED_PARAM)
char *opt_p = NULL;
char *opt_t = NULL;
char *opt_u = NULL;
+ char *opt_d = NULL;
int read_flags = 0;
const char *r;
int i;
- while ((i = nextopt("p:u:rt:n:s")) != '\0') {
+ while ((i = nextopt("p:u:rt:n:sd:")) != '\0') {
switch (i) {
case 'p':
opt_p = optionarg;
@@ -13439,6 +13440,9 @@ readcmd(int argc UNUSED_PARAM, char **argv UNUSED_PARAM)
case 'u':
opt_u = optionarg;
break;
+ case 'd':
+ opt_d = optionarg;
+ break;
default:
break;
}
@@ -13456,7 +13460,8 @@ readcmd(int argc UNUSED_PARAM, char **argv UNUSED_PARAM)
opt_n,
opt_p,
opt_t,
- opt_u
+ opt_u,
+ opt_d
);
INT_ON;
diff --git a/shell/hush.c b/shell/hush.c
index bb80f422c..efa518a89 100644
--- a/shell/hush.c
+++ b/shell/hush.c
@@ -9434,13 +9434,15 @@ static int FAST_FUNC builtin_read(char **argv)
char *opt_p = NULL;
char *opt_t = NULL;
char *opt_u = NULL;
+ char *opt_d = NULL;
const char *ifs;
int read_flags;
/* "!": do not abort on errors.
* Option string must start with "sr" to match BUILTIN_READ_xxx
*/
- read_flags = getopt32(argv, "!srn:p:t:u:", &opt_n, &opt_p, &opt_t,
&opt_u);
+ read_flags = getopt32(argv, "!srn:p:t:u:d:", &opt_n, &opt_p, &opt_t,
+ &opt_u, &opt_d);
if (read_flags == (uint32_t)-1)
return EXIT_FAILURE;
argv += optind;
@@ -9454,7 +9456,8 @@ static int FAST_FUNC builtin_read(char **argv)
opt_n,
opt_p,
opt_t,
- opt_u
+ opt_u,
+ opt_d
);
if ((uintptr_t)r == 1 && errno == EINTR) {
diff --git a/shell/shell_common.c b/shell/shell_common.c
index a9f8d8413..2db8ea3e2 100644
--- a/shell/shell_common.c
+++ b/shell/shell_common.c
@@ -54,7 +54,8 @@ shell_builtin_read(void FAST_FUNC (*setvar)(const char *name,
const char *val),
const char *opt_n,
const char *opt_p,
const char *opt_t,
- const char *opt_u
+ const char *opt_u,
+ const char *opt_d
)
{
struct pollfd pfd[1];
@@ -237,14 +238,17 @@ shell_builtin_read(void FAST_FUNC (*setvar)(const char
*name, const char *val),
continue;
}
}
- if (c == '\n')
+ if (opt_d) {
+ if (c == *opt_d)
+ break;
+ } else if (c == '\n')
break;
/* $IFS splitting. NOT done if we run "read"
* without variable names (bash compat).
* Thus, "read" and "read REPLY" are not the same.
*/
- if (argv[0]) {
+ if (!opt_d && argv[0]) {
/*
http://www.opengroup.org/onlinepubs/9699919799
Re: [PATCH] Silence misguided GCC warning about alignment issues
Hi Denys, On Mon, 7 Aug 2017, Denys Vlasenko wrote: > On Mon, Aug 7, 2017 at 12:09 PM, Johannes Schindelin > wrote: > > When compiling xz_dec_stream.c with GCC 7.1.0, it complains thusly: > > > > In function 'dec_stream_footer': > > error: dereferencing type-punned pointer will break strict-aliasing > > rules [-Werror=strict-aliasing] > >if (xz_crc32(s->temp.buf + 4, 6, 0) != get_le32(s->temp.buf)) > >^~ > > > > The thing is, the `buf` field was put just after two fields of type > > size_t for the express purpose of avoiding alignment issues, as per the > > comment above the `temp` struct. > > > > Meaning: GCC gets this all wrong and should not complain. So let's force > > it to be quiet for a couple of lines. > > xz_crc32 is: > > xz_crc32(const uint8_t *buf, size_t size, uint32_t crc) > > I think gcc is not complaining about xz_crc32, > it complains about get_le32! > > Can you test this theory by splitting that statement in two? I, too, thought at first that it is clearly about get_le32(), but GCC was really adamant that it was the xz_crc32() call. And a little further investigation suggests that GCC really meant xz_crc32(), which is defined as a static function in archival/libarchive/decompress_unxz.c that gets inlined by GCC because it simply hands off to crc32_block_endian0() which in turn takes an `uint32_t *` as first parameter. And that's where the type-punning is broken. Ciao, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Silence misguided GCC warning about alignment issues
Hi, On Mon, 7 Aug 2017, Johannes Schindelin wrote: > On Mon, 7 Aug 2017, Denys Vlasenko wrote: > > > On Mon, Aug 7, 2017 at 12:09 PM, Johannes Schindelin > > wrote: > > > When compiling xz_dec_stream.c with GCC 7.1.0, it complains thusly: > > > > > > In function 'dec_stream_footer': > > > error: dereferencing type-punned pointer will break > > > strict-aliasing > > > rules [-Werror=strict-aliasing] > > >if (xz_crc32(s->temp.buf + 4, 6, 0) != get_le32(s->temp.buf)) > > >^~ > > > > > > The thing is, the `buf` field was put just after two fields of type > > > size_t for the express purpose of avoiding alignment issues, as per the > > > comment above the `temp` struct. > > > > > > Meaning: GCC gets this all wrong and should not complain. So let's force > > > it to be quiet for a couple of lines. > > > > xz_crc32 is: > > > > xz_crc32(const uint8_t *buf, size_t size, uint32_t crc) > > > > I think gcc is not complaining about xz_crc32, > > it complains about get_le32! > > > > Can you test this theory by splitting that statement in two? > > I, too, thought at first that it is clearly about get_le32(), but GCC was > really adamant that it was the xz_crc32() call. > > And a little further investigation suggests that GCC really meant > xz_crc32(), which is defined as a static function in > archival/libarchive/decompress_unxz.c that gets inlined by GCC because it > simply hands off to crc32_block_endian0() which in turn takes an > `uint32_t *` as first parameter. And that's where the type-punning is > broken. Okay, nevermind, I misread the xz_crc32() function. The first parameter passed to crc32_block_endian0() is not the first parameter passed to xz_crc32(). And splitting the statement in two does indeed suggest that get_le32() is the actual cause for that warning. Ciao, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Silence misguided GCC warning about alignment issues
Hi Emmanuel, On Mon, 7 Aug 2017, Emmanuel Deloget wrote: > On Mon, Aug 7, 2017 at 12:25 PM, Lauri Kasanen wrote: > > On Mon, 7 Aug 2017 12:09:45 +0200 (CEST) > > Johannes Schindelin wrote: > >> Meaning: GCC gets this all wrong and should not complain. So let's force > >> it to be quiet for a couple of lines. > > > > I believe this is both wrong and dangerous. If gcc warns about it, it > > will also very likely try to mis-optimize the same case. > > > > - Lauri > > I tried to carefully read the code, and I don't see where the aliasing > issue migh be. > * buf is an uin8_t [] > * the code either use it as is, or promote it to const uint8_t *, or > cast it to (const) char *. > > All of these types are compatible w.r.t. aliasing (and the C99 > standard). I may have missed something though and my eyes might be > rusted. > > Now, it seems that some might have seen a regression in GCC with > respect to aliasing rules [1][2]. This might be an instance of the bug > (according to the bug report, it's no longer reproducible in GCC > trunk). I dug a little deeper, and it seems this issue may indeed have been reported as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80593 (and been fixed in the meantime). However... > Anyway, it might be a good idea to avoid unsetting the warning for one > buggy GCC version :) ... I get this warning *also* with the default GCC version of Ubuntu 16.04.3 LTS, gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609. So it is not just one buggy GCC version. I do agree, however, that it is a super-ugly patch, even if it does the job for the described purpose. So I set out to find a more elegant solution. More elegant, even, than removing the get_le32() macro as Denys did in 76b65624b (unxz: get_le32 macro is obviously wrong, 2017-08-07): the macro was there for a reason, namely to avoid accessing the (purposefully aligned) data using get_unaligned_le32() which is slow and unlikely to get optimized. It turns out that GCC simply has a hard time with the cast, without assigning the pointer explicitly to a variable. Turning get_le32() into an inline function lets GCC figure out that everything is groovy, and it also adds a bit more of concrete compile-time safety. I will send out v2 in a moment, assuming that Denys will agree that direct, provably aligned 32-bit access is better than an unnecessary assembly-from-individual-bytes. Ciao, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2] Silence misguided GCC warning about alignment issues
When compiling xz_dec_stream.c with GCC (at least with versions 5.4.0
and 7.1.0), it complains thusly:
In function 'dec_stream_footer':
error: dereferencing type-punned pointer will break
strict-aliasing rules [-Werror=strict-aliasing]
if (xz_crc32(s->temp.buf + 4, 6, 0) != get_le32(s->temp.buf))
^~
Despite the arrow pointing to the xz_crc32() call, the actual problem is
the get_le32() macro expansion.
The thing is, the `buf` field was put just after two fields of type
size_t for the express purpose of avoiding alignment issues, as per the
comment above the `temp` struct.
Meaning: GCC gets this all wrong and should not complain.
An earlier attempt to fix (76b65624b (unxz: get_le32
macro is obviously wrong, 2017-08-07)) this by simply forcing
get_unaligned_32() (which is unnecessarily slow because it rebuilds a
32-bit int from accessing already-aligned bytes one by one) was a bit
heavy-handed.
But we can help GCC by turning the get_le32() macro into an inline
function. So let's do that, it also helps compile time safety by making
the code a little bit stricter.
Signed-off-by: Johannes Schindelin
---
Published-As:
https://github.com/dscho/busybox-w32/releases/tag/busybox-type-punned-warning-v2
Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32
busybox-type-punned-warning-v2
Interdiff vs v1:
diff --git a/archival/libarchive/decompress_unxz.c
b/archival/libarchive/decompress_unxz.c
index 0be85500c..e24cff98b 100644
--- a/archival/libarchive/decompress_unxz.c
+++ b/archival/libarchive/decompress_unxz.c
@@ -37,6 +37,11 @@ static uint32_t xz_crc32(const uint8_t *buf, size_t size,
uint32_t crc)
|| !defined(put_unaligned_be32)
# error get_unaligned_le32 accessors are not defined
#endif
+static ALWAYS_INLINE uint32_t get_le32_fast(const void *p)
+{
+ return *(uint32_t *)p;
+}
+#define get_le32 get_le32_fast
#include "unxz/xz_dec_bcj.c"
#include "unxz/xz_dec_lzma2.c"
diff --git a/archival/libarchive/unxz/xz_dec_stream.c
b/archival/libarchive/unxz/xz_dec_stream.c
index 8131ee30a..bf791055b 100644
--- a/archival/libarchive/unxz/xz_dec_stream.c
+++ b/archival/libarchive/unxz/xz_dec_stream.c
@@ -423,15 +423,6 @@ static enum xz_ret XZ_FUNC dec_stream_footer(struct
xz_dec *s)
if (!memeq(s->temp.buf + 10, FOOTER_MAGIC, FOOTER_MAGIC_SIZE))
return XZ_DATA_ERROR;
-#if defined(__GNUC__)
- /*
- * The temp.buf field is put just after two fields of type size_t for
- * the express purpose of avoiding alignment issues. But GCC complains
- * about it nevertheless... so: shut GCC up for a few lines.
- */
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wstrict-aliasing"
-#endif
if (xz_crc32(s->temp.buf + 4, 6, 0) != get_le32(s->temp.buf))
return XZ_DATA_ERROR;
@@ -442,9 +433,6 @@ static enum xz_ret XZ_FUNC dec_stream_footer(struct xz_dec
*s)
*/
if ((s->index.size >> 2) != get_le32(s->temp.buf + 4))
return XZ_DATA_ERROR;
-#if defined(__GNUC__)
-#pragma GCC diagnostic pop
-#endif
if (s->temp.buf[8] != 0 || s->temp.buf[9] != s->check_type)
return XZ_DATA_ERROR;
archival/libarchive/decompress_unxz.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/archival/libarchive/decompress_unxz.c
b/archival/libarchive/decompress_unxz.c
index 0be85500c..e24cff98b 100644
--- a/archival/libarchive/decompress_unxz.c
+++ b/archival/libarchive/decompress_unxz.c
@@ -37,6 +37,11 @@ static uint32_t xz_crc32(const uint8_t *buf, size_t size,
uint32_t crc)
|| !defined(put_unaligned_be32)
# error get_unaligned_le32 accessors are not defined
#endif
+static ALWAYS_INLINE uint32_t get_le32_fast(const void *p)
+{
+ return *(uint32_t *)p;
+}
+#define get_le32 get_le32_fast
#include "unxz/xz_dec_bcj.c"
#include "unxz/xz_dec_lzma2.c"
base-commit: a907b828d6e9f1357fc2e1db09d3eb1d3fb9b826
--
2.14.0.windows.1.2.g0f3342804fc
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Silence misguided GCC warning about alignment issues
Hi Emmanuel, On Mon, 7 Aug 2017, Emmanuel Deloget wrote: > And yes, its seems that the get_le32() macro in xz_private.h is a bit > illegal with respect to strict aliasing, as it casts a uint8_t * into a > const uint32_t *. It would seem so. Until you realize that it is used only on s->temp.buf (or `s->temp.buf + 4 * `), and that the `buf` field was carefully placed after two fields of type `size_t` (i.e. naturally aligned *at least* to a 32-bit boundary). If you are worried about that magic, you can even mark the `buf` field using ALIGN4. But that *still* does not fix GCC's compiler warning. What fixes it is my workaround of defining a static ALWAYS_INLINE function. Denys' fix works around the problem, alright, but it also makes the code slower. And it is *not* necessary to make the code slower. Ciao, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2 0/2] Support for `read -d`
This feature is used by Git's test suite. That is the incentive. It was
also pretty to implement, so why not ;-)
Changes since v1:
- split into two patches, one for ash, one for hush, to make it easier
to drop the hush one if so desired.
- hide the feature behind the *_BASH_COMPAT knobs, as suggested by
Kang-Che Sung.
Johannes Schindelin (2):
ash: implement -d DELIM option for `read`
hush: implement -d DELIM option for `read`
shell/ash.c | 19 ---
shell/hush.c | 24 ++--
shell/shell_common.c | 10 +++---
shell/shell_common.h | 3 ++-
4 files changed, 47 insertions(+), 9 deletions(-)
base-commit: 73adef14b25533b71238362da75bfb482d43d98b
Published-As:
https://github.com/dscho/busybox-w32/releases/tag/busybox-read-d-v2
Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 busybox-read-d-v2
Interdiff vs v1:
diff --git a/shell/ash.c b/shell/ash.c
index e2d98584a..4b0896573 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -189,6 +189,8 @@
#defineBASH_HOSTNAME_VARENABLE_ASH_BASH_COMPAT
#defineBASH_SHLVL_VAR ENABLE_ASH_BASH_COMPAT
#defineBASH_XTRACEFDENABLE_ASH_BASH_COMPAT
+#defineBASH_READ_D ENABLE_ASH_BASH_COMPAT
+#define IF_BASH_READ_D IF_ASH_BASH_COMPAT
#if defined(__ANDROID_API__) && __ANDROID_API__ <= 24
/* Bionic at least up to version 24 has no glob() */
@@ -13415,7 +13417,7 @@ readcmd(int argc UNUSED_PARAM, char **argv
UNUSED_PARAM)
char *opt_p = NULL;
char *opt_t = NULL;
char *opt_u = NULL;
- char *opt_d = NULL;
+ IF_BASH_READ_D(char *opt_d = NULL;)
int read_flags = 0;
const char *r;
int i;
@@ -13440,9 +13442,11 @@ readcmd(int argc UNUSED_PARAM, char **argv
UNUSED_PARAM)
case 'u':
opt_u = optionarg;
break;
+#if BASH_READ_D
case 'd':
opt_d = optionarg;
break;
+#endif
default:
break;
}
@@ -13461,7 +13465,11 @@ readcmd(int argc UNUSED_PARAM, char **argv
UNUSED_PARAM)
opt_p,
opt_t,
opt_u,
+#if BASH_READ_D
opt_d
+#else
+ NULL
+#endif
);
INT_ON;
diff --git a/shell/hush.c b/shell/hush.c
index e5e59c949..278b44a9c 100644
--- a/shell/hush.c
+++ b/shell/hush.c
@@ -352,6 +352,7 @@
#define BASH_SOURCEENABLE_HUSH_BASH_COMPAT
#define BASH_HOSTNAME_VAR ENABLE_HUSH_BASH_COMPAT
#define BASH_TEST2 (ENABLE_HUSH_BASH_COMPAT && ENABLE_HUSH_TEST)
+#define BASH_READ_DENABLE_HUSH_BASH_COMPAT
/* Build knobs */
@@ -9434,15 +9435,27 @@ static int FAST_FUNC builtin_read(char **argv)
char *opt_p = NULL;
char *opt_t = NULL;
char *opt_u = NULL;
+#if BASH_READ_D
char *opt_d = NULL;
+#endif
const char *ifs;
int read_flags;
/* "!": do not abort on errors.
* Option string must start with "sr" to match BUILTIN_READ_xxx
*/
- read_flags = getopt32(argv, "!srn:p:t:u:d:", &opt_n, &opt_p, &opt_t,
- &opt_u, &opt_d);
+ read_flags = getopt32(argv,
+#if BASH_READ_D
+"!srn:p:t:u:d:",
+#else
+"!srn:p:t:u:",
+#endif
+&opt_n, &opt_p, &opt_t,
+ &opt_u
+#if BASH_READ_D
+, &opt_d
+#endif
+ );
if (read_flags == (uint32_t)-1)
return EXIT_FAILURE;
argv += optind;
@@ -9457,7 +9470,11 @@ static int FAST_FUNC builtin_read(char **argv)
opt_p,
opt_t,
opt_u,
+#if BASH_READ_D
opt_d
+#else
+NULL
+#endif
);
if ((uintptr_t)r == 1 && errno == EINTR) {
--
2.14.0.windows.1.2.g0f3342804fc
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2 1/2] ash: implement -d DELIM option for `read`
The POSIX standard only requires the `read` builtin to handle `-r`:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/read.html
However, Bash introduced the option `-d ` to override IFS for
just one invocation, and it is quite useful.
It is also super easy to implement in BusyBox' ash, so let's do that.
The motivation: This option is used by Git's test suite.
function old new delta
.rodata 163505 163587 +82
shell_builtin_read 12441289 +45
readcmd 233 259 +26
builtin_read 258 263 +5
--
(add/remove: 0/0 grow/shrink: 4/0 up/down: 158/0) Total: 158 bytes
Signed-off-by: Johannes Schindelin
---
shell/ash.c | 19 ---
shell/hush.c | 3 ++-
shell/shell_common.c | 10 +++---
shell/shell_common.h | 3 ++-
4 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index 6dc1cfef7..4b0896573 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -189,6 +189,8 @@
#defineBASH_HOSTNAME_VARENABLE_ASH_BASH_COMPAT
#defineBASH_SHLVL_VAR ENABLE_ASH_BASH_COMPAT
#defineBASH_XTRACEFDENABLE_ASH_BASH_COMPAT
+#defineBASH_READ_D ENABLE_ASH_BASH_COMPAT
+#define IF_BASH_READ_D IF_ASH_BASH_COMPAT
#if defined(__ANDROID_API__) && __ANDROID_API__ <= 24
/* Bionic at least up to version 24 has no glob() */
@@ -13402,10 +13404,10 @@ letcmd(int argc UNUSED_PARAM, char **argv)
* -p PROMPT Display PROMPT on stderr (if input is from tty)
* -t SECONDS Timeout after SECONDS (tty or pipe only)
* -u FD Read from given FD instead of fd 0
+ * -d DELIMEnd on DELIM char, not newline
* This uses unbuffered input, which may be avoidable in some cases.
* TODO: bash also has:
* -a ARRAYRead into array[0],[1],etc
- * -d DELIMEnd on DELIM char, not newline
* -e Use line editing (tty only)
*/
static int FAST_FUNC
@@ -13415,11 +13417,12 @@ readcmd(int argc UNUSED_PARAM, char **argv
UNUSED_PARAM)
char *opt_p = NULL;
char *opt_t = NULL;
char *opt_u = NULL;
+ IF_BASH_READ_D(char *opt_d = NULL;)
int read_flags = 0;
const char *r;
int i;
- while ((i = nextopt("p:u:rt:n:s")) != '\0') {
+ while ((i = nextopt("p:u:rt:n:sd:")) != '\0') {
switch (i) {
case 'p':
opt_p = optionarg;
@@ -13439,6 +13442,11 @@ readcmd(int argc UNUSED_PARAM, char **argv
UNUSED_PARAM)
case 'u':
opt_u = optionarg;
break;
+#if BASH_READ_D
+ case 'd':
+ opt_d = optionarg;
+ break;
+#endif
default:
break;
}
@@ -13456,7 +13464,12 @@ readcmd(int argc UNUSED_PARAM, char **argv
UNUSED_PARAM)
opt_n,
opt_p,
opt_t,
- opt_u
+ opt_u,
+#if BASH_READ_D
+ opt_d
+#else
+ NULL
+#endif
);
INT_ON;
diff --git a/shell/hush.c b/shell/hush.c
index 8dc531657..d4e647fc0 100644
--- a/shell/hush.c
+++ b/shell/hush.c
@@ -9454,7 +9454,8 @@ static int FAST_FUNC builtin_read(char **argv)
opt_n,
opt_p,
opt_t,
- opt_u
+ opt_u,
+NULL
);
if ((uintptr_t)r == 1 && errno == EINTR) {
diff --git a/shell/shell_common.c b/shell/shell_common.c
index a9f8d8413..2db8ea3e2 100644
--- a/shell/shell_common.c
+++ b/shell/shell_common.c
@@ -54,7 +54,8 @@ shell_builtin_read(void FAST_FUNC (*setvar)(const char *name,
const char *val),
const char *opt_n,
const char *opt_p,
const char *opt_t,
- const char *opt_u
+ const char *opt_u,
+ const char *opt_d
)
{
struct pollfd pfd[1];
@@ -237,14 +238,17 @@ shell_builtin_read(void FAST_FUNC (*setvar)(const char
*name, const char *val),
continue;
}
}
- if (c == '\n')
+ if (opt_d) {
+ if (c == *opt_d)
+ break;
+ } else if (c == '\n')
break;
/* $IFS splitting. NOT done if we run "read"
* without variable names (bash compat).
* Thus, "read" and "read REPLY" are not the same.
Re: [PATCH] ash/hush: implement -d DELIM option for `read`
Hi, On Tue, 8 Aug 2017, Kang-Che Sung wrote: > On Mon, Aug 7, 2017 at 6:18 PM, Johannes Schindelin > wrote: > > The POSIX standard only requires the `read` builtin to handle `-r`: > > http://pubs.opengroup.org/onlinepubs/9699919799/utilities/read.html > > > > However, Bash introduced the option `-d ` to override IFS for > > just one invocation, and it is quite useful. > > > > It is also super easy to implement in BusyBox' ash, so let's do that. > > > > The motivation: This option is used by Git's test suite. > > > > Signed-off-by: Johannes Schindelin > > Can you wrap the change within a macro conditional like #if BASH_READ_D ? I can! And I did! It made the patch a little less readable, though. Ciao, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2 2/2] hush: implement -d DELIM option for `read`
The POSIX standard only requires the `read` builtin to handle `-r`:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/read.html
However, Bash introduced the option `-d ` to override IFS for
just one invocation, and it is quite useful.
We already support this in ash, let's add it to hush, too.
function old new delta
builtin_read 263 284 +21
.rodata 163587 163589 +2
--
(add/remove: 0/0 grow/shrink: 2/0 up/down: 23/0) Total: 23 bytes
Signed-off-by: Johannes Schindelin
---
shell/hush.c | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/shell/hush.c b/shell/hush.c
index d4e647fc0..278b44a9c 100644
--- a/shell/hush.c
+++ b/shell/hush.c
@@ -352,6 +352,7 @@
#define BASH_SOURCEENABLE_HUSH_BASH_COMPAT
#define BASH_HOSTNAME_VAR ENABLE_HUSH_BASH_COMPAT
#define BASH_TEST2 (ENABLE_HUSH_BASH_COMPAT && ENABLE_HUSH_TEST)
+#define BASH_READ_DENABLE_HUSH_BASH_COMPAT
/* Build knobs */
@@ -9434,13 +9435,27 @@ static int FAST_FUNC builtin_read(char **argv)
char *opt_p = NULL;
char *opt_t = NULL;
char *opt_u = NULL;
+#if BASH_READ_D
+ char *opt_d = NULL;
+#endif
const char *ifs;
int read_flags;
/* "!": do not abort on errors.
* Option string must start with "sr" to match BUILTIN_READ_xxx
*/
- read_flags = getopt32(argv, "!srn:p:t:u:", &opt_n, &opt_p, &opt_t,
&opt_u);
+ read_flags = getopt32(argv,
+#if BASH_READ_D
+"!srn:p:t:u:d:",
+#else
+"!srn:p:t:u:",
+#endif
+&opt_n, &opt_p, &opt_t,
+ &opt_u
+#if BASH_READ_D
+, &opt_d
+#endif
+ );
if (read_flags == (uint32_t)-1)
return EXIT_FAILURE;
argv += optind;
@@ -9455,7 +9470,11 @@ static int FAST_FUNC builtin_read(char **argv)
opt_p,
opt_t,
opt_u,
+#if BASH_READ_D
+ opt_d
+#else
NULL
+#endif
);
if ((uintptr_t)r == 1 && errno == EINTR) {
--
2.14.0.windows.1.2.g0f3342804fc
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2 0/2] Support for `read -d`
Hi Denys, On Wed, 9 Aug 2017, Denys Vlasenko wrote: > Applied, thanks. Thank you! > Does git testsuite work now with current git? I have not tested on Linux. But I did test on Windows. The motiviation behind all my BusyBox work was to start releasing an experimental BusyBox-based MinGit (read: stripped-down Git for Windows intended purely for non-interactive usage by third-party applications, such as Visual Studio). As of v2.14.0, we have exactly this (see the MinGit versions at https://github.com/git-for-windows/git/releases/tag/v2.14.0.windows.2 whose versions end in -busybox). And yes, BusyBox-based MinGit passed all of Git's test suite in my hands. Of course, I still have a couple of patches to contribute, but I think only three "report-warnings" ones are suitable for BusyBox proper. The rest is mostly for BusyBox-w32, with a couple of patches most likely too specific to Git for Windows' setup. Ciao, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] Silence misguided GCC warning about alignment issues
Hi Emmanuel, On Tue, 8 Aug 2017, Emmanuel Deloget wrote: > On Tue, Aug 8, 2017 at 1:19 PM, Johannes Schindelin < > johannes.schinde...@gmx.de> wrote: > > > On Mon, 7 Aug 2017, Emmanuel Deloget wrote: > > > > > And yes, its seems that the get_le32() macro in xz_private.h is a > > > bit illegal with respect to strict aliasing, as it casts a uint8_t * > > > into a const uint32_t *. > > > > It would seem so. > > > > Until you realize that it is used only on s->temp.buf (or `s->temp.buf + 4 > > * `), and that the `buf` field was carefully placed after two > > fields of type `size_t` (i.e. naturally aligned *at least* to a 32-bit > > boundary). > > > > If you are worried about that magic, you can even mark the `buf` field > > using ALIGN4. But that *still* does not fix GCC's compiler warning. What > > fixes it is my workaround of defining a static ALWAYS_INLINE function. > > > > > No, I'm not worried by the magic :) > > Yet the problem is not the aliasing per see, it's the way the compiler > handles it and what it means to him. Please do not refer to the compiler as a "he". You are a "he", I assume, and I am a "he" (I am certain about the latter). But the compiler is an "it". Writing "he" for the compiler makes me stumble every single time, which means I had a much harder time understanding your otherwise excellent explanation. I was under the impression that the C standard explicitly forbade the duplication/reordering you mentioned when it comes to struct fields. But maybe I was wrong on that. Having been educated about this, I agree that the easier fix is to simply drop the optimization, even if it is a pity to give up on performance. Ciao, Johannes___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH 0/2] Use the new ash_msg_and_raise_perror() in more places
This is by no means an exhaustive list of patches where using the *_perror() instead of the *_error() function would make sense. Yet, I encountered both errors in my development and really needed to get a better idea what was wrong than, say, "can't cd to ...". Hopefully these patches are useful for other developers, too. Johannes Schindelin (2): ash: report reason when a script file could not be opened ash: when cd fails, say why shell/ash.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) base-commit: 3505e38bd1e5714fa1203e6752c573861aab8521 Published-As: https://github.com/dscho/busybox-w32/releases/tag/busybox-report-errors-v1 Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 busybox-report-errors-v1 -- 2.14.1.windows.1.11.gc06fee21d46.dirty ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH 1/2] ash: report reason when a script file could not be opened
It is always nicer to give the user some sort of indication why an
operation failed.
Signed-off-by: Johannes Schindelin
---
shell/ash.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/shell/ash.c b/shell/ash.c
index 703802ff5..a67a45376 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -10575,7 +10575,7 @@ setinputfile(const char *fname, int flags)
if (flags & INPUT_NOFILE_OK)
goto out;
exitstatus = 127;
- ash_msg_and_raise_error("can't open '%s'", fname);
+ ash_msg_and_raise_perror("can't open '%s'", fname);
}
if (fd < 10)
fd = savefd(fd);
--
2.14.1.windows.1.11.gc06fee21d46.dirty
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH 2/2] ash: when cd fails, say why
Signed-off-by: Johannes Schindelin
---
shell/ash.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/shell/ash.c b/shell/ash.c
index a67a45376..60bcd1b0a 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -2748,7 +2748,7 @@ cdcmd(int argc UNUSED_PARAM, char **argv UNUSED_PARAM)
goto docd;
err:
- ash_msg_and_raise_error("can't cd to %s", dest);
+ ash_msg_and_raise_perror("can't cd to %s", dest);
/* NOTREACHED */
out:
if (flags & CD_PRINT)
--
2.14.1.windows.1.11.gc06fee21d46.dirty
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH 0/1] xargs: support spawning processes in parallel
A GNU-specific extension of the xargs command allows spawning the processes in parallel by passing the `-P ` option, where `` is the maximal number of processes to spawn at the same time. I need this feature to run Git's test suite in an environment where only BusyBox is available and no development tools such as `make` or `prove`. Please give this a good look-over, as I want to make sure that this is correct. It seems to work alright in my tests, yet I would feel more comfortable if I got at least one other developer to agree that the feature is implemented correctly. Johannes Schindelin (1): xargs: support -P (parallel execution) findutils/xargs.c | 112 +- 1 file changed, 111 insertions(+), 1 deletion(-) base-commit: 8b77a9ea81a0bd89ee69e7742d9b920dd1562763 Published-As: https://github.com/dscho/busybox-w32/releases/tag/busybox-xargs-p-v1 Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 busybox-xargs-p-v1 -- 2.14.1.windows.1.11.gc06fee21d46 ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH 1/1] xargs: support -P (parallel execution)
The GNU variant of xargs supports a special, non-POSIX extension to run
processes in parallel, triggered by the -P option.
This feature comes in handy e.g. when running Git's test suite outside
of the development environment using only BusyBox (which does not
support `make` nor `prove`, Git's go-to solutions for running the test
suite in parallel).
It so just happens that this patch also addresses the feature requested
in https://bugs.busybox.net/show_bug.cgi?id=9511.
Signed-off-by: Johannes Schindelin
---
findutils/xargs.c | 112 +-
1 file changed, 111 insertions(+), 1 deletion(-)
diff --git a/findutils/xargs.c b/findutils/xargs.c
index c3d37a64d..cf9a8d474 100644
--- a/findutils/xargs.c
+++ b/findutils/xargs.c
@@ -60,6 +60,13 @@
//config: depends on XARGS
//config: help
//config: Support -I STR and -i[STR] options.
+//config:
+//config:config FEATURE_XARGS_SUPPORT_PARALLEL
+//config: bool "Enable -P N: processes to run in parallel"
+//config: default y
+//config: depends on XARGS
+//config: help
+//config: Support -P N option.
//applet:IF_XARGS(APPLET_NOEXEC(xargs, xargs, BB_DIR_USR_BIN, BB_SUID_DROP,
xargs))
@@ -100,6 +107,10 @@ struct globals {
#endif
const char *eof_str;
int idx;
+#if ENABLE_FEATURE_XARGS_SUPPORT_PARALLEL
+ int max_procs, running_procs;
+ pid_t *procs;
+#endif
} FIX_ALIASING;
#define G (*(struct globals*)bb_common_bufsiz1)
#define INIT_G() do { \
@@ -107,14 +118,92 @@ struct globals {
G.eof_str = NULL; /* need to clear by hand because we are NOEXEC applet
*/ \
IF_FEATURE_XARGS_SUPPORT_REPL_STR(G.repl_str = "{}";) \
IF_FEATURE_XARGS_SUPPORT_REPL_STR(G.eol_ch = '\n';) \
+ IF_FEATURE_XARGS_SUPPORT_PARALLEL(G.max_procs = 1;) \
+ IF_FEATURE_XARGS_SUPPORT_PARALLEL(G.running_procs = 0;) \
+ IF_FEATURE_XARGS_SUPPORT_PARALLEL(G.procs = NULL;) \
} while (0)
+#if ENABLE_FEATURE_XARGS_SUPPORT_PARALLEL
+static int wait_for_slot(int *idx)
+{
+ int status;
+ pid_t pid;
+
+ /* if less than max_procs running, set status to 0, return next free
slot */
+ if (G.running_procs < G.max_procs) {
+ *idx = G.running_procs++;
+ return 0;
+ }
+
+ pid = safe_waitpid(-1, &status, 0);
+ if (pid < 0)
+ return pid;
+
+ for (*idx = 0; *idx < G.max_procs; (*idx)++)
+ if (G.procs[*idx] == pid) {
+ G.procs[*idx] = 0;
+ return WEXITSTATUS(status);
+ }
+
+ bb_error_msg("waitpid returned %"PRIu64" but we did not spawn it",
+ (uint64_t)pid);
+
+ return -1;
+}
+
+static int reap_remaining(void)
+{
+ int status, ret = 0, ret2;
+ while (G.running_procs) {
+ pid_t pid = safe_waitpid(-1, &status, 0);
+ if (pid < 0)
+ return errno == ENOENT ? 127 : 126;
+ G.running_procs--;
+ status = WEXITSTATUS(status);
+ if (status == 255)
+ ret2 = 124;
+ else if (status >= 0x180)
+ ret2 = 125;
+ else if (status)
+ ret2 = 123;
+ else
+ ret2 = 0;
+ if (ret < ret2)
+ ret = ret2;
+ }
+ return ret;
+}
+#endif /* SUPPORT_PARALLEL */
+
static int xargs_exec(void)
{
int status;
+#if !ENABLE_FEATURE_XARGS_SUPPORT_PARALLEL
status = spawn_and_wait(G.args);
+#else
+ if (!G.max_procs) {
+ pid_t p = spawn(G.args);
+ if (p == -1)
+ status = -1;
+ else {
+ status = 0;
+ G.running_procs++;
+ }
+ } else if (G.max_procs > 1) {
+ int idx = -1;
+ status = wait_for_slot(&idx);
+ if (status >= 0 && status < 0x180) {
+ pid_t p = spawn(G.args);
+ if (p < 0)
+ status = -1;
+ else
+ G.procs[idx] = p;
+ }
+ } else
+ status = spawn_and_wait(G.args);
+#endif
if (status < 0) {
bb_simple_perror_msg(G.args[0]);
return errno == ENOENT ? 127 : 126;
@@ -436,6 +525,9 @@ static int xargs_ask_confirmation(void)
//usage: IF_FEATURE_XARGS_SUPPORT_REPL_STR(
//usage: "\n -I STR Replace STR within PROG ARGS with input line"
//usage: )
+//usage: IF_FEATURE_XARGS_SUPPORT_PARALLEL(
+//usage: "\n -P NRun up to N processes in parallel"
+//usage: )
//usage: I
Re: [PATCH 1/1] xargs: support -P (parallel execution)
Hi Walter,
On Fri, 25 Aug 2017, walter harms wrote:
> Am 24.08.2017 13:19, schrieb Denys Vlasenko:
>
> > +/*
> > + * Returns 0 if xargs should continue (but may set G.xargs_exitcode to
> > 123).
> > + * Else sets G.xargs_exitcode to error code and returns nonzero.
> > + *
> > + * If G.max_procs == 0, performs final waitpid() loop for all children.
> > + */
> > static int xargs_exec(void)
> > {
> > int status;
> > [..]
> > +} else {
> > +/* final waitpid() loop: must be ECHILD "no more children" */
> > +status = 0;
> > +}
>
> maybe you can make it a function returning status ?
> that would improve readability.
No, it would not, as the status has to be interpreted in a very specific
way: first, you have to update the global exit status, but only if it was
indicating a less serious error (or no error at all), and then you have to
either continue or stop based on the value.
What Denys wrote separates the concern of updating the global status from
determining whether to continue or not.
Ciao,
Johannes
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 1/1] xargs: support -P (parallel execution)
Hi Denys,
On Thu, 24 Aug 2017, Denys Vlasenko wrote:
> function old new delta
> xargs_main 7871174+387
> packed_usage 31757 31769 +12
> --
> (add/remove: 0/0 grow/shrink: 2/0 up/down: 399/0) Total: 399 bytes
>
>
> How about this version:
>
> function old new delta
> xargs_exec - 294+294
> packed_usage 31757 31772 +15
> xargs_main 787 719 -68
> --
> (add/remove: 1/0 grow/shrink: 1/1 up/down: 309/-68) Total: 241 bytes
>
I see, you did away with the `procs` array. I am a little worried that the
*wrong* children will be mistaken for the xargs-children, and that as a
consequence too many children will be running at the same time.
Also, I implemented it using the array because I need this feature to work
on Windows, after all, where we do not have an equivalent for the
waitpid(-1, ...) call to wait for any child process to exit.
A couple of comments on your patch:
> diff --git a/findutils/xargs.c b/findutils/xargs.c
> index b4e3db0..77e01ef 100644
> --- a/findutils/xargs.c
> +++ b/findutils/xargs.c
> [...]
> +/*
> + * Returns 0 if xargs should continue (but may set G.xargs_exitcode to 123).
> + * Else sets G.xargs_exitcode to error code and returns nonzero.
> + *
> + * If G.max_procs == 0, performs final waitpid() loop for all children.
> + */
> static int xargs_exec(void)
Nice, this makes it a lot less magic.
> {
> int status;
>
> +#if !ENABLE_FEATURE_XARGS_SUPPORT_PARALLEL
> status = spawn_and_wait(G.args);
> +#else
> +if (G.max_procs == 1) {
> +status = spawn_and_wait(G.args);
> +} else {
> +pid_t pid;
> +int wstat;
> + again:
> +if (G.running_procs >= G.max_procs)
> +pid = safe_waitpid(-1, &wstat, 0);
> +else
> +pid = wait_any_nohang(&wstat);
What if running_procs == max_procs == 0? In that case, we should avoid
calling either safe_waitpid() or wait_any_nohang(). Maybe instead
something like
+if (G.running_procs < G.max_procs)
+pid = wait_any_nohang(&wstat);
+else if (G.running_procs > 0)
+pid = safe_waitpid(-1, &wstat, 0);
But then, the wait_any_nohang() call is still unnecessary if running_procs
== 0. But you want to save space, not necessarily make the thing more
performant, so...
> +if (pid > 0) {
> +/* We may have children we don't know about:
> + * sh -c 'sleep 1 & exec xargs ...'
> + * Do not make G.running_procs go negative.
> + */
This hints very eloquently at the problem I am concerned about. But I
think it is worse than just running_procs going negative. You could easily
mistake child processes for having exited when they have not, including
possibly failing children (and hence the exit status can be all wrong).
> +if (G.running_procs != 0)
> +G.running_procs--;
> +status = WIFSIGNALED(wstat)
> +? 0x180 + WTERMSIG(wstat)
> +: WEXITSTATUS(wstat);
> +if (status > 0 && status < 255) {
> +/* See below why 123 does not abort */
> +G.xargs_exitcode = 123;
> +status = 0;
> +}
> +if (status == 0)
> +goto again; /* maybe we have more children? */
I think this logic still needs some love:
- if running_procs is 0, there is no need to go at it again
- if running_procs is greater than 0, we still have to wait for the
remaining children to exit, even if one child exited due to a signal.
> +/* else: "bad" status, will bail out */
> +} else if (G.max_procs != 0) {
> +/* Not in final waitpid() loop,
> + * and G.running_procs < G.max_procs: start more procs
> + */
> +status = spawn(G.args);
> +/* here "status" actually holds pid, or -1 */
> +if (status > 0) {
> +G.running_procs++;
> +status = 0;
> +}
> +/* else: status == -1 (failed to fork or exec) */
> +} else {
> +/* final waitpid() loop: must be ECHILD "no more children" */
> +status = 0;
> +}
> +}
> +#endif
> +/* Manpage:
> + * """xargs exits with the following status:
> + * 0 if it succeeds
> + * 123 if any invocation of the command exited with status 1-125
> + * 124 if the command exited with status 255
> + * ("""If any invocation of the command exits with a status of
[PATCH] xargs: support -a args-file
The GNU-specific option -a lets xargs read the arguments from a file
rather than from stdin.
This is particularly convenient when debugging in gdb interactively,
and it might be of more general use.
function old new delta
xargs_main 841 923 +82
.rodata 164296 164330 +34
packed_usage 32073 32085 +12
--
(add/remove: 0/0 grow/shrink: 3/0 up/down: 128/0) Total: 128 bytes
Signed-off-by: Johannes Schindelin
---
As stated in the commit message, this was used for debugging, so I
could imagine that we'd want it to default to N...
findutils/xargs.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/findutils/xargs.c b/findutils/xargs.c
index 77e01ef6c..31f33c697 100644
--- a/findutils/xargs.c
+++ b/findutils/xargs.c
@@ -64,6 +64,11 @@
//config: bool "Enable -P N: processes to run in parallel"
//config: default y
//config: depends on XARGS
+//config:
+//config:config FEATURE_XARGS_SUPPORT_ARGS_FILE
+//config: bool "Enable --arg-file PATH: use file instead of stdin"
+//config: default y
+//config: depends on XARGS
//applet:IF_XARGS(APPLET_NOEXEC(xargs, xargs, BB_DIR_USR_BIN, BB_SUID_DROP,
xargs))
@@ -517,6 +522,9 @@ static int xargs_ask_confirmation(void)
//usage: IF_FEATURE_XARGS_SUPPORT_ZERO_TERM(
//usage: "\n -0 Input is separated by NUL characters"
//usage: )
+//usage: IF_FEATURE_XARGS_SUPPORT_ARGS_FILE(
+//usage: "\n -a PATH Read args from file instead of stdin"
+//usage: )
//usage: "\n -t Print the command on stderr before execution"
//usage: "\n -e[STR] STR stops input processing"
//usage: "\n -n NPass no more than N args to PROG"
@@ -565,7 +573,8 @@ enum {
IF_FEATURE_XARGS_SUPPORT_TERMOPT( "x") \
IF_FEATURE_XARGS_SUPPORT_ZERO_TERM( "0") \
IF_FEATURE_XARGS_SUPPORT_REPL_STR("I:i::") \
- IF_FEATURE_XARGS_SUPPORT_PARALLEL("P:+")
+ IF_FEATURE_XARGS_SUPPORT_PARALLEL("P:+") \
+ IF_FEATURE_XARGS_SUPPORT_ARGS_FILE( "a:")
int xargs_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
int xargs_main(int argc UNUSED_PARAM, char **argv)
@@ -584,6 +593,7 @@ int xargs_main(int argc UNUSED_PARAM, char **argv)
#else
#define read_args process_stdin
#endif
+ IF_FEATURE_XARGS_SUPPORT_PARALLEL(char *opt_a = NULL;)
INIT_G();
@@ -592,6 +602,7 @@ int xargs_main(int argc UNUSED_PARAM, char **argv)
&max_args, &max_chars, &G.eof_str, &G.eof_str
IF_FEATURE_XARGS_SUPPORT_REPL_STR(, &G.repl_str, &G.repl_str)
IF_FEATURE_XARGS_SUPPORT_PARALLEL(, &G.max_procs)
+ IF_FEATURE_XARGS_SUPPORT_ARGS_FILE(, &opt_a)
);
#if ENABLE_FEATURE_XARGS_SUPPORT_PARALLEL
@@ -599,6 +610,16 @@ int xargs_main(int argc UNUSED_PARAM, char **argv)
G.max_procs = 100; /* let's not go crazy high */
#endif
+#if ENABLE_FEATURE_XARGS_SUPPORT_ARGS_FILE
+ if (opt_a) {
+ int fd = open(opt_a, O_RDONLY);
+ if (fd < 0)
+ bb_perror_msg_and_die("can't open '%s'", opt_a);
+ if (dup2(fd, 0) < 0)
+ bb_perror_msg_and_die("Could not reopen stdin");
+ }
+#endif
+
/* -E ""? You may wonder why not just omit -E?
* This is used for portability:
* old xargs was using "_" as default for -E / -e */
base-commit: 14551b7036acf98f81d76674f351ce99148762c8
--
2.14.1.windows.1.48.g37d08f901a6
Published-As: https://github.com/dscho/busybox-w32/releases/tag/xargs-a-v1
Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 xargs-a-v1
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] xargs: support -a args-file
Hi Xabier, On Wed, 30 Aug 2017, Xabier Oneca -- xOneca wrote: > 2017-08-29 21:06 GMT+02:00 Denys Vlasenko : > > > > On Fri, Aug 25, 2017 at 10:42 PM, Johannes Schindelin > > wrote: > >> @@ -584,6 +593,7 @@ int xargs_main(int argc UNUSED_PARAM, char **argv) > >> #else > >> #define read_args process_stdin > >> #endif > >> + IF_FEATURE_XARGS_SUPPORT_PARALLEL(char *opt_a = NULL;) > > This should have been IF_FEATURE_XARGS_SUPPORT_ARGS_FILE... D'oh. You're correct! Denys, thanks for fixing it already. Ciao, Johannes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] unzip: -d should create the dir
Hi,
On Wed, 15 Apr 2020, Lauri Kasanen wrote:
> The official Info-Zip unzip creates the dir if it doesn't exist.
This bit me recently, too. However,
> Signed-off-by: Lauri Kasanen
> ---
> archival/unzip.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/archival/unzip.c b/archival/unzip.c
> index 8c4cb98..45e0727 100644
> --- a/archival/unzip.c
> +++ b/archival/unzip.c
> @@ -646,8 +646,10 @@ int unzip_main(int argc, char **argv)
> }
>
> /* Change dir if necessary */
> - if (base_dir)
> + if (base_dir) {
> + mkdir(base_dir, 0777); /* Try to create, errors don't matter */
Doesn't Info-Zip do more like an `mkdir -p` than only a single level?
Ciao,
Johannes
> xchdir(base_dir);
> + }
>
> if (quiet <= 1) { /* not -qq */
> if (quiet == 0) {
> --
> 2.6.2
>
> ___
> busybox mailing list
> busybox@busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
>
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
