251117 - fix error propagation in nsFileStream::Close()

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[[e-mail]]

http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsFileStreams.cpp#125

it's important for consumers to know whether closing the descriptor failed,
since closing can sometimes involve flushing/writing data to disk. see e.g.
nsSafeFileOutputStream. thanks to darin for spotting this.

Comment 1

[[e-mail]]

Attachment: v1

Comment 2

[Darin Fisher]

Comment on attachment 153025 [details] [diff] [review]
v1

>Index: netwerk/base/src/nsFileStreams.cpp

>+                rv = NS_BASE_STREAM_OSERROR;

does NS_ErrorAccordingToNSPR() need to be extended perhaps?
though NS_BASE_STREAM_OSERROR is fine by me.

r+sr=darin

Comment 3

[[e-mail]]

(In reply to comment #2)
> does NS_ErrorAccordingToNSPR() need to be extended perhaps?

i wasn't really sure about that... i think NS_ErrorAccordingToNSPR() only works
when nspr sets the error code on a thread (which you get with PR_GetError()),
and i couldn't tell if PR_Close() does that. can you shed some light? ;)

Comment 4

[[e-mail]]

adding darin to cc, please see comment 3

Comment 5

[Darin Fisher]

Under Unix at least, PR_Close calls _MD_close (see src/md/unix/unix.c), which
may call _MD_unix_map_close_error, which calls PR_SetError.

Comment 6

[[e-mail]]

ahh, nice. hmm, but then we'd have to add all those error mappings from [E* ->]
PR_* -> NS_ERROR_* into NS_ErrorAccordingToNSPR(), or at least the ones that
apply to closing a file. is the list in
http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/md/unix/unix_errors.c#44
complete (for unix at least)?

i can go ahead and do that if you want... what do you think? /me worries that
NS_ErrorAccordingToNSPR is pseudodeprecated, given the comment in nsError.h... :/

Comment 7

[Darin Fisher]

let's not worry about fixing up NS_ErrorAccordingToNSPR here.  it can be another
bug.  cvs blame says that dougt added that comment.  i'm not sure why... he
doesn't link to a bug or explain it in the checkin comment.  (checkin comment
says he is fixing an IRIX compilation problem!)  maybe he added that comment
because nsError.h is part of the Gecko SDK, and he didn't want
NS_ErrorAccordingToNSPR to be included in the SDK.  perhaps the function should
live elsewhere.  hmm...

Comment 8

[Darin Fisher]

again, NS_BASE_STREAM_OSERROR is fine by me for now.

Comment 9

[[e-mail]]

Comment on attachment 153025 [details] [diff] [review]
v1

heh. okey doke.

requesting approval... this is required for the safe file output patches to
work properly, and it would be great to have it in alpha for testing, since
there's some risk of (broken) consumers choking on the new failure-case
behavior.

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 153025 [details] [diff] [review]
v1

Agreed: safe-file is a big 1.8 change, and this is safe enough for 1.8a2.
a=shaver

Comment 11

[[e-mail]]

checked in.

Comment 12

[[e-mail]]

needed-aviary1.0 by proxy (bug 251091).