Page MenuHomeFreeBSD
Differential D2697

fdclose
ClosedPublic
Actions

Authored by oshogbo on May 31 2015, 7:14 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 10:05 PM
Unknown Object (File)
Fri, Nov 22, 11:16 PM
Unknown Object (File)
Wed, Nov 20, 3:16 AM
Unknown Object (File)
Tue, Nov 19, 11:16 PM
Unknown Object (File)
Tue, Nov 19, 10:56 PM
Unknown Object (File)
Tue, Nov 19, 2:58 AM
Unknown Object (File)
Tue, Nov 19, 1:56 AM
Unknown Object (File)
Mon, Nov 18, 11:35 PM
View All Files
Subscribers
imp
wblock

Details

Reviewers
jilles
jhb
allanjude
wblock
pjd
bdrewery
Group Reviewers
manpages
Commits
rS292154: MFstable/10 r291602:
rS291602: MFC r285140:
rS285140: Add fdclose(3) function.
Summary

After a quite long time I would like finally finish this function. :)
The change to the perl will be very simple (I believe there is no way to mix src and ports repositories in phabricator):

--- perl-5.20.2/perlio.c
+++ perl-5.20.2/perlio.c
@@ -3122,12 +3122,7 @@
     f->_file = -1;
     return 1;
 #  elif defined(__FreeBSD__)
-    /* There may be a better way on FreeBSD:
-        - we could insert a dummy func in the _close function entry
-	f->_close = (int (*)(void *)) dummy_close;
-     */
-    f->_file = -1;
-    return 1;
+    return fdclose(f, NULL) == 0 ? 1 : 0;
 #  elif defined(__OpenBSD__)
     /* There may be a better way on OpenBSD:
         - we could insert a dummy func in the _close function entry

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

oshogbo updated this revision to Diff 5843.May 31 2015, 7:14 PM
oshogbo retitled this revision from to fdclose.
oshogbo updated this object.
oshogbo edited the test plan for this revision. (Show Details)
oshogbo added reviewers: pjd, jilles, jhb, bdrewery, allanjude.
oshogbo set the repository for this revision to rS FreeBSD src repository - subversion.
Herald added a reviewer: manpages. · View Herald TranscriptMay 31 2015, 7:14 PM
Herald added a subscriber: imp. · View Herald Transcript
jilles edited edge metadata.May 31 2015, 10:06 PM

The code looks good to me.

lib/libc/stdio/fclose.3
72 ↗(On Diff #5843)

Calling the argument fdp instead of fd may make this clearer.

Try:

If
.Fa fd
is not
.Dv NULL ,
the file descriptor will be written to it.

73 ↗(On Diff #5843)

The words "non-standard close method" may be unclear. Perhaps say "If the stream does not have an associated file descriptor", possibly with examples like fmemopen(), funopen() and open_memstream().

These words also occur under ERRORS below.

If the stream is not open, the behaviour is not defined and need not be described in the man page.

97 ↗(On Diff #5843)

This change seems unnecessary. It is still the case that there are two situations: success and error.

110 ↗(On Diff #5843)

errno should keep its .Va markup

141 ↗(On Diff #5843)

This should be .Sh HISTORY.

oshogbo updated this revision to Diff 5862.Jun 1 2015, 9:28 AM
oshogbo edited edge metadata.
oshogbo removed rS FreeBSD src repository - subversion as the repository for this revision.

Update man page as jilles suggest.

wblock added a subscriber: wblock.Jun 1 2015, 5:57 PM
wblock added inline comments.
lib/libc/stdio/fclose.3
78 ↗(On Diff #5862)

Needs a comma after "descriptor".

81 ↗(On Diff #5862)

"Such stream" should be "Such a stream", but it's a little confusing, maybe "This type of stream", or maybe it does not even need to be said. "functions like" is confusing, probably should be "such as". Finally, switch to active rather than passive voice by changing "could" to "can" or "is":

Streams are created with functions such as
or
This type of stream is created with functions such as
or even
This type of stream is created with

95 ↗(On Diff #5862)

Several things:

  1. Not necessary to use "the blah function". After the first time, just say the name of the function: "blah returns..." (no leading "The").
  2. "returns no value" is somewhat ambiguous, could be misread to mean it returns null. "does not return a value" might be more clear.

So:

.Fn fcloseall
does not return a value.

110 ↗(On Diff #5862)

"Shall" implies a recommendation. Probably better as "will".

136 ↗(On Diff #5862)

(Missing an "it" after "makes").
Use a full stop rather than an emdash:

This is intentional.
It makes it easier to make sure programs written under

jhb accepted this revision.Jun 1 2015, 6:22 PM
jhb edited edge metadata.

The code changes look fine to me (I'll defer to others on the manpage bits).

This revision is now accepted and ready to land.Jun 1 2015, 6:22 PM
oshogbo updated this revision to Diff 5869.Jun 1 2015, 6:57 PM
oshogbo edited edge metadata.
oshogbo marked 3 inline comments as done.

Update man page as wblock suggest. Thanks!

This revision now requires review to proceed.Jun 1 2015, 6:57 PM
oshogbo marked 6 inline comments as done.Jun 1 2015, 7:02 PM

@wblock

"Shall" implies a recommendation. Probably better as "will".

I see this is a problematic one.
In first version i had "will" but Bruce suggest me that I should changed it.

I don't like the tense given by "will" in man pages. POSIX says "shall
fail" in similar contexts, and "will fail" is a mistranslation of this
("shall" is a technical term that doesn't suggest future tense).

oshogbo updated this revision to Diff 5870.Jun 1 2015, 7:15 PM
oshogbo edited edge metadata.

Missed one suggested by @wblock.

wblock added inline comments.Jun 2 2015, 5:12 PM
lib/libc/stdio/fclose.3
38 ↗(On Diff #5869)

No comma after "descriptor" here. (This might be due to phabricator "helping" by mixing up diffs. I'll try to include some context to make this easier.)

38 the file descriptor will be written to it.

41 ↗(On Diff #5869)

Typo: s/arguemnt/argument/

42 ↗(On Diff #5869)

Needs a comma after this "descriptor", where the pause is located:

42 If the stream does not have an associated file descriptor,

99 ↗(On Diff #5869)

Start the new sentence on a new line:

99  This is intentional.

100 It makes it easier to make sure programs written under

wblock added a comment.Jun 2 2015, 5:16 PM
In D2697#51201, @oshogbo wrote:

@wblock

"Shall" implies a recommendation. Probably better as "will".

I see this is a problematic one.
In first version i had "will" but Bruce suggest me that I should changed it.

I don't like the tense given by "will" in man pages. POSIX says "shall
fail" in similar contexts, and "will fail" is a mistranslation of this
("shall" is a technical term that doesn't suggest future tense).

All right, let's eliminate the stilted language altogether:

Original:

The
​.Fn fdclose
​function shall fail if:

New:

.Fn fdclose
fails if:

oshogbo updated this revision to Diff 5888.EditedJun 2 2015, 6:12 PM

wblock suggestions.

I also add missing .El.

oshogbo updated this revision to Diff 5889.Jun 2 2015, 6:15 PM
oshogbo marked 4 inline comments as done.

Compromise about shall and will :)

wblock added inline comments.Jun 2 2015, 9:47 PM
lib/libc/stdio/fclose.3
41 ↗(On Diff #5889)

Oops, I missed the other typo:
s/diffrent/different/

oshogbo updated this revision to Diff 5911.Jun 3 2015, 2:19 PM
oshogbo marked an inline comment as done.

s/diffrent/different/

wblock accepted this revision.Jun 3 2015, 3:27 PM
wblock added a reviewer: wblock.

Man page looks good. Please test with igor -R fclose.3 | less -RS and mandoc -Tlint fclose.3 before commit.

This revision is now accepted and ready to land.Jun 3 2015, 3:27 PM
Closed by commit rS285140: Add fdclose(3) function. (authored by oshogbo). · Explain WhyJul 4 2015, 4:42 PM
This revision was automatically updated to reflect the committed changes.
oshogbo marked an inline comment as done.

Revision Contents
Changeset List

PathSize
head/
include/
1 line
lib/
libc/
stdio/
4 lines
82 lines
75 lines

Diff 6691

View Options

head/include/stdio.h

Loading...
View Options

head/lib/libc/stdio/Symbol.map

Loading...
View Options

head/lib/libc/stdio/fclose.3

Loading...
View Options

head/lib/libc/stdio/fclose.c

Loading...