portability for {Free,Net,Open,}BSD #505

agreppin · 2024-06-11T21:19:17Z

No description provided.

landley · 2024-06-15T01:19:05Z

Touches more files at once than I'm happy with. (The test suite plumbing tends to use SKIP rather than if around the tests, to avoid running different NUMBERS of tests on different targets, and silently skipping tests without saying "SKIP:")...

Came to the web interface to see if the 1/2 that got squished together by wget 505.patch provided better granularity, but doesn't look like it. (I can break it up easily enough, but then either I'm faking metadata or don't attribute it properly...)

I'll take a swing at it...

agreppin · 2024-06-15T09:24:26Z

please note that I only tested with this config:

CONFIG_TOYBOX_FORK=y
CONFIG_PRINTF=y
CONFIG_SORT_FLOAT=y
CONFIG_BASE64=y
CONFIG_HELP=y
CONFIG_GZIP=y
CONFIG_GUNZIP=y
CONFIG_ZCAT=y
CONFIG_TOYBOX=y
CONFIG_TOYBOX_LSM_NONE=y
CONFIG_TOYBOX_FLOAT=y
CONFIG_TOYBOX_HELP=y
CONFIG_TOYBOX_ZHELP=y
CONFIG_TOYBOX_DEBUG=y
CONFIG_TOYBOX_UID_SYS=100
CONFIG_TOYBOX_UID_USR=500

reactive-firewall

I did a partial Review in hopes that it helps @landley, and the changes to lib/protability.* are trivial, I'll recommend further review of scripts specificly in the case of toybox's long-term plans.

See specific comments inline.

🙇 Hope this helps.

reactive-firewall · 2025-08-16T18:33:53Z

lib/portability.c

+#ifdef __OpenBSD__
+#define syscall(...) -1
+#endif


🧹 NIT - Plea for a comment here.

Rational - It would be nice (but not strictly not necessary) to have a comment about why this portability trick is done for OpenBSD for those of us not yet initiated.

syscall(2) is not available anymore on OpenBSD for common mortals
https://undeadly.org/cgi?action=article;sid=20231213062827

reactive-firewall · 2025-08-16T18:42:11Z

lib/portability.c

👍 LGTM; Regarding lib/portability.c

I see no issue adding the || defined(__NetBSD__) checks (:speak_no_evil: as long as the NetBSD headers don't fall under GPL or something that might prevent inclusion, etc.) as they are straightforward.

See also 🧹 NIT suggestion inline regarding adding a helpful comment.

lib/portability.h

reactive-firewall · 2025-08-16T19:12:31Z

scripts/portability.sh


+case "$(uname)" in
+'NetBSD'|'OpenBSD')
+  unset ASAN;;


😕 does OpenBSD really have issues with address sanitation? Please explain for those of us who are uninitiated. (🙏 with small, gentle words please)

this is a quick hack to be able to run the tests. The base config of NetBSD, OpenBSD (and Alpine Linux / musl-libc), does not have a CC with -fsanitize=address.

🙇 Thanks for the explanation, I was unaware.

🙊 and I can almost hear the musl libc devs strictly claiming the lack of a check here is a bug too . 🙄

perhaps consider a comment:

Suggested change

unset ASAN;;

#TODO: add compiler check for '-fsanitize=address' (e.g., $CC -h 2>1 | grep 'args')

unset ASAN;; # otherwise assume no compiler support by default

reactive-firewall · 2025-08-16T19:12:34Z

scripts/portability.sh

 fi

+case ${OD-} in
+'') OD=$(command -v god || command -v ggod || echo od);; # use od from GNU coretutils


🙅 This change concerns me by deliberately introducing a reliance on "GNU coretutils" and places the original od command at the end of the chained-or-logic lending to a preference on GNU.

Perhaps at least consider maintaining od at the start of the logic. Otherwise please add a comment for why this logic is needed for those of us yet uninitiated.

not all /bin/od are equals, the test suite seems biased toward a specific implementation of od.

Suggested change

'') OD=$(command -v god || command -v ggod || echo od);; # use od from GNU coretutils

'') OD=$(command -v god || command -v ggod || echo od);; # use od from GNU coretutils for GNU biased tests

reactive-firewall · 2025-08-16T19:17:09Z

tests/zcat.test


 #testing "name" "command" "result" "infile" "stdin"

+HEAD=$(command -v ghead || echo head) # use head from GNU coreutils


😕 Similar to before in scripts/portability.sh this preference for GNU variants concerns me. Please consider maintaining the preference for the original variant in the chained-or-logic. Otherwise please enlighten us uninitiated about why NetBSD would require this.

I don't remember now, seems a similar issue ... some tests fails with head and not with ghead.

🤔 Odd; unless ghead expands the 2.gz file before reporting the first 10 bytes (e.g., $HEAD -c 10 2.gz) I don't see why a BSD-Like head would behave different from ghead in this use-case.

perhaps a -o pipefail somewhere or similar side-efect issue 🤷

🤷 LGTM; Using a variable is maintainable enough.

main.c

reactive-firewall · 2025-10-07T04:01:56Z

main.c

👍 LGTM; Regarding main.c the no-op implementation for uselocale is straight forward, and targeted at NetBSD.

agreppin · 2025-10-06T04:50:42Z

Thanks for the review, I will rework that PR, when time permits.

Co-authored-by: Mr. Walls <reactive-firewall@users.noreply.github.com>

reactive-firewall

👍 LGTM; See suggested NIT suggestion/comments inline.

@landley this is probably ready for you to review.

🙇 I hope this helps.

reactive-firewall · 2025-10-07T03:58:49Z

lib/portability.h

👍 again LGTM; Regarding lib/portability.h the changes are now well styled and trivial.

reactive-firewall · 2025-10-07T04:01:56Z

main.c

👍 LGTM; Regarding main.c the no-op implementation for uselocale is straight forward, and targeted at NetBSD.

reactive-firewall · 2025-10-07T04:12:13Z

scripts/portability.sh


+case "$(uname)" in
+'NetBSD'|'OpenBSD')
+  unset ASAN;;


🙇 Thanks for the explanation, I was unaware.

🙊 and I can almost hear the musl libc devs strictly claiming the lack of a check here is a bug too . 🙄

perhaps consider a comment:

Suggested change

unset ASAN;;

#TODO: add compiler check for '-fsanitize=address' (e.g., $CC -h 2>1 | grep 'args')

unset ASAN;; # otherwise assume no compiler support by default

reactive-firewall · 2025-10-07T04:25:17Z

tests/zcat.test


 #testing "name" "command" "result" "infile" "stdin"

+HEAD=$(command -v ghead || echo head) # use head from GNU coreutils


🤔 Odd; unless ghead expands the 2.gz file before reporting the first 10 bytes (e.g., $HEAD -c 10 2.gz) I don't see why a BSD-Like head would behave different from ghead in this use-case.

perhaps a -o pipefail somewhere or similar side-efect issue 🤷

🤷 LGTM; Using a variable is maintainable enough.

portability for {Free,Net,Open,}BSD

f595efb

agreppin force-pushed the portability branch from 4eb5205 to f595efb Compare June 15, 2024 09:42

reactive-firewall reviewed Aug 16, 2025

View reviewed changes

agreppin and others added 3 commits October 6, 2025 07:25

Update main.c

67eac52

Co-authored-by: Mr. Walls <reactive-firewall@users.noreply.github.com>

Update lib/portability.h

b303e8e

Co-authored-by: Mr. Walls <reactive-firewall@users.noreply.github.com>

Update lib/portability.h

1ec3eb7

Co-authored-by: Mr. Walls <reactive-firewall@users.noreply.github.com>

reactive-firewall reviewed Oct 7, 2025

View reviewed changes

	unset ASAN;;
	#TODO: add compiler check for '-fsanitize=address' (e.g., $CC -h 2>1 \| grep 'args')
	unset ASAN;; # otherwise assume no compiler support by default

	'') OD=$(command -v god \|\| command -v ggod \|\| echo od);; # use od from GNU coretutils
	'') OD=$(command -v god \|\| command -v ggod \|\| echo od);; # use od from GNU coretutils for GNU biased tests


		#testing "name" "command" "result" "infile" "stdin"

		HEAD=$(command -v ghead \|\| echo head) # use head from GNU coreutils

portability for {Free,Net,Open,}BSD #505

Are you sure you want to change the base?

portability for {Free,Net,Open,}BSD #505

Uh oh!

Conversation

agreppin commented Jun 11, 2024

Uh oh!

landley commented Jun 15, 2024

Uh oh!

agreppin commented Jun 15, 2024

Uh oh!

reactive-firewall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agreppin commented Oct 6, 2025

Uh oh!

reactive-firewall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants