Page MenuHomeFreeBSD

libc/stdio changes from Apple
AbandonedPublic

Authored by pfg on Jul 19 2014, 12:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 11, 5:29 AM
Unknown Object (File)
Sun, Dec 1, 9:29 PM
Unknown Object (File)
Sun, Dec 1, 9:29 PM
Unknown Object (File)
Sun, Dec 1, 9:29 PM
Unknown Object (File)
Sun, Dec 1, 9:29 PM
Unknown Object (File)
Sun, Dec 1, 9:29 PM
Unknown Object (File)
Sun, Dec 1, 9:28 PM
Unknown Object (File)
Sun, Dec 1, 9:28 PM
Subscribers

Details

Reviewers
jilles
theraven
ed
Group Reviewers
manpages
Summary

This is a bunch of changes from Apple's Libc.
I am specifically excluding xlocale() and collation changes
and I am focusing on UNIX03 and compliance issues.

Test Plan

This is an initial patch, I only know it compiles but it
may still have style issues and even mistakes.
Some of this, in particular the freopen changes will require
more testing. I will be running the gnulib tests on it.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2445
Build 2462: arc lint + arc unit

Event Timeline

pfg retitled this revision from to libc/stdio changes from Apple.
pfg updated this object.
pfg edited the test plan for this revision. (Show Details)
pfg added reviewers: theraven, jilles.
jilles requested changes to this revision.Jul 20 2014, 3:33 PM
jilles edited edge metadata.

Some parts are good, some parts need changes, some parts may be wrong. See per-file comments.

This should be multiple commits.

Note that I have only looked at the code. I have not tested it in any way.

lib/libc/stdio/fgetln.3
102

Perhaps the INT_MAX part should instead be in a BUGS section. It is unlikely to be fixed. Applications can use getline() instead.

lib/libc/stdio/fputs.c
67

This is wrong if the length exceeds INT_MAX. POSIX.1-2008tc1 suggests capping at INT_MAX if the length is supposed to be returned (this text is new in tc1).

Note that the standard text simply requires a non-negative integer if successful, so this is likely another bug in the testsuite :(

lib/libc/stdio/freopen.c
100 ↗(On Diff #763)

This errno change can probably committed separately, as it seems obviously correct to me.

135 ↗(On Diff #763)

Do we want to break compatibility in this way? An freopen() of stdin/stdout/stderr is common, and dangerous bugs may show up if those streams lose their common fd numbers.

155 ↗(On Diff #763)

We may want this (even though it is dead code with the above change), but only for fd >= 3.

lib/libc/stdio/wbuf.c
64 ↗(On Diff #763)

The prepwrite macro is used six more times; should those be adjusted as well?

This revision now requires changes to proceed.Jul 20 2014, 3:33 PM

Thank you.
Yes,the changes are meant be committed individually. This is all just work-in-progress.
I am undecided still on freopen(), I think it should probably be discussed in the lists.
I assume the rest is OK ... so I will do some more checking and I will commit them slowly.
FWIW, we pass the gnulib tests before and after the changes (with the exception of a
coredump in test-striconv, which seems unrelated).

lib/libc/stdio/fgetln.3
102

I will consider it for a different commit that touches only documentation, Thank you.

lib/libc/stdio/fputs.c
67

Good ... I do prefer to keep things simple :).

lib/libc/stdio/freopen.c
100 ↗(On Diff #763)

Committed revision 268926.

135 ↗(On Diff #763)

It is a good question. I guess AIX and whatever "big iron" UNIXes are left already do this. One possibility would be to bring it to current but no MFC. On the other hand we are not forced to comply with UNIX 03.

Some more replies to your comments.

lib/libc/stdio/fputs.c
67

Actually. I have a new patch with this:
return (iov.iov_len >= INT_MAX ? INT_MAX : iov.iov_len);

lib/libc/stdio/wbuf.c
64 ↗(On Diff #763)

Good catch: Apple also adds the adjustment on vfprintf.c. I added it to vfwprintf.c too. The two remaining checks according to preceeding comments in the code "may not be necessary".
Committed as revision 268930.

pfg edited edge metadata.
  1. Updating D442: libc/stdio changes from Apple #

These are the remaining *controversial* changes:

  • The fputs() change should be committed but not MFC'd.
  • The freopen() changes will have to be rethought (help welcome).
  • The fgetln() changes will not be committed: are here only for reference.
pfg edited edge metadata.
  1. Updating D442: libc/stdio changes from Apple #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #

Rescue what seems to be good from the freopen patch:
rewritten by ache@, pretty much in line with jilles@ comments.

Add a small enhancemente to fpurge.

Try to add ache as a reviewer:

Reviewers:
ache

  1. Updating D442: libc/stdio changes from Apple #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #

Nevermind about the fpurge change: it is a local change corresponding
to r236288.

  1. Updating D442: libc/stdio changes from Apple #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #

Cleanup: drop changes that have been committed or directly rejected.

These are the remaining changes.
I now think neither of them should be committed.
In the case of fputs() returning 0 upon failure seems more intuitive
than returning the length of the string. While Apple's behavior is
also standards compliant it can lead to compatibility issues.

I won't be merging these changes.

I am considering to apply this change to FreeBSD 11 as-is:

  • The fgetln change is a consequence of a limitation in our implementation: not sure if it classifies as a bug but there's nothing we can do at this time.
  • The fputs change is something the standard lets us do and is convenient for compatibility with Apple. A small change would be returning uio.uio_resid since that is an int already.

Minor update .. a while after.

Committed as r295631 (getln), r295632 (fputs).
No plans to MFC.
No idea why phabricator didn't catch it.