Page MenuHomeFreeBSD

Revert echo(1) diet changes.
Needs ReviewPublic

Authored by delphij on Aug 2 2020, 6:40 PM.

Details

Summary

The C startup code calls exit(), which in turn called
__cxa_thread_call_dtors that called fprintf(), so stdio is
linked regardless of our effort.

MFC after: 2 weeks

Sidenote: This was introduced in rS121010 and I think the goal
was to reduce the size of statically linked binary of /bin/echo.
In rS308432 a call to err() was introduced, but rewriting it
as errexit() would not remove err.c from the statically linked
binary either.

The space saving was negligible (7944 -> 7528 for FreeBSD/amd64
dynamically linked, or 766656 -> 766416 statically linked).

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 32740
Build 30182: arc lint + arc unit

Event Timeline

delphij created this revision.

This looks like half-done. You are mixing write(2) and stdio .

If going this direction, why not revert r121010 and convert write() to printf() ?

delphij retitled this revision from Remove home-grown err(3) whose purpose was to avoid stdio. to Revert echo(1) diet changes..Aug 3 2020, 12:48 AM
delphij edited the summary of this revision. (Show Details)
delphij edited the summary of this revision. (Show Details)
bin/echo/echo.c
52

(For reviewer) Previous version contained an /* ARGSUSED */ comment here due to the unused argc argument, which is mainly for lint which is no longer being used against the tree so my intention is to not bring it back.

78

The \c support in /bin/echo is not really standard [1] compliant as far as I understand (POSIX says \c can appear anywhere and echo should stop there, while our implementation only recognizes trailing \c's). I'm leaving it as-is for now as it's beyond scope of this change.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/echo.html

This revision is now accepted and ready to land.Aug 3 2020, 7:51 AM
cem added inline comments.
bin/echo/echo.c
87–89

We could just do fputs() here.

bin/echo/echo.c
87–89

I believe both clang and gcc do this automatically in present days.

jilles requested changes to this revision.Aug 3 2020, 8:01 PM

I found a reason to touch this code, but otherwise I'm a bit surprised about a change here.

Even if using writev() saves little code size, it is likely to be a bit faster than stdio due to less memory copies (although when performance is important, /bin/echo is unlikely to be used).

bin/echo/echo.c
78

The \c support is indeed not POSIX-compliant, but I'm not sure it's worth it to fix. I expect obscure breakage for little gain.

For example, dash's echo builtin interprets all XSI backslash escapes and this causes a constant trickle of bug reports by surprised (in a negative way) users.

87–89

Or even better fwrite() since the length is already known.

138–139

This error handling is incomplete (partial writes are not handled), but replacing it with no error handling at all is a regression.

This revision now requires changes to proceed.Aug 3 2020, 8:01 PM
delphij marked 3 inline comments as done.

Address jilles's comments.

delphij marked an inline comment as done.

Detect error at the end of echo(1) after performing an explicit
fflush().

Inspired by NetBSD.

Thanks for the comments, please take another look.

bin/echo/echo.c
87–89

Thanks, I'm taking fwrite() route.

delphij added inline comments.
bin/echo/echo.c
78

Let's leave the non-standard behavior as-is for now.

Both sh(1)'s built-in echo and GNU echo(1)'s behavior is XSI compliant only when -e is specified, which is in my opinion more reasonable but by itself violated POSIX's "Implementations shall not support any options." requirement.

bin/echo/echo.c
87

I am not sure that what you do there is correct, if your intent is to handle errors.
Error from fwrite(3) should 'set the stream' error indicator', but does not neither guarantee that all passed data was buffered nor that subsequent operations of the file detect the error.

IMO you should check for fwrite() result and also check for short writes.

93

If you do fflush(), you should check for returned error as well.