this is a spinoff from bug 246675, where we ended up floundering around with rv-propagation issues. * darin thinks that nsISafeFileInputStream should just have a Finish() method. <dwitte> heh <darin> no writeSucceeded madness <dwitte> what would that do? <darin> call Finish to indicate that you are done writing, and want the file to be moved to the target location. <darin> if you do not call Finish, then the file will not be saved, and the tempfile will be removed. * dwitte likes <darin> the caller can then also check the return value of Finish to find out if the file was saved properly <dwitte> actually...yeah. that would solve the problem of trying to pack too much into Close <darin> right <darin> and the caller could then ignore errors from Write if they wanted because we'd still trap them in Finish

sounds good. just to be clear, Finish would presumably be called after Close(), right? (so that the file descriptor is closed and the file can be renamed)

actually, Finish would do the closing itself. It will take care of checking all the writes succeeded, closing the descriptor, and (re)moving the temp file. Most importantly, the rv from Finish will reflect whether everything succeeded, for convenience to the consumer (rather than it having to check several rv's, which is more ambiguous from an interface usage perspective).

as promised. one notable difference here is that we now save the nsresult from a failed Write() call, and propagate it directly back to the consumer in Finish(). does this look ok?

Comment on attachment 153024 [details] [diff] [review] v1 Looks good, although i wonder if a failure in close() might be temporary, like when the disk is still writing or something. And shouldn't the two consumers be updated to retry saving when writing failed? r=mvl with that fixed (or good arguments against it :) )

> Looks good, although i wonder if a failure in close() might be temporary, like > when the disk is still writing or something. From "man close" on Fedora Core 2: Not checking the return value of close is a common but nevertheless serious programming error. It is quite possible that errors on a pre- vious write(2) operation are first reported at the final close. Not checking the return value when closing the file may lead to silent loss of data. This can especially be observed with NFS and disk quotas. A successful close does not guarantee that the data has been success- fully saved to disk, as the kernel defers writes. It is not common for a filesystem to flush the buffers when the stream is closed. If you need to be sure that the data is physically stored use fsync(2). (It will depend on the disk hardware at this point.) Perhaps we should call PR_Sync before calling PR_Close. > And shouldn't the two consumers be updated to retry saving when writing failed? Yeah, perhaps... but, I think that's outside the scope of this bug.

Comment on attachment 153024 [details] [diff] [review] v1 >Index: netwerk/base/src/nsFileStreams.cpp > nsSafeFileOutputStream::Write(const char *buf, PRUint32 count, PRUint32 *result) > { > nsresult rv = nsFileOutputStream::Write(buf, count, result); >+ if (NS_SUCCEEDED(mWriteResult)) { >+ if (NS_FAILED(rv)) >+ mWriteResult = rv; >+ else if (count != *result) >+ mWriteResult = NS_ERROR_FAILURE; >+ } > return rv; > } good idea to verify |count == *result|, but perhaps it'd be good to add a NS_WARNING in the failure case. NS_ERROR_UNEXPECTED might be better than NS_ERROR_FAILURE too. sr=darin (thanks dwitte!)

feel free to take the PR_Sync issue to another bug.

Comment on attachment 153024 [details] [diff] [review] v1 thanks darin, i'll make those changes. requesting approval for this patch, which revises the API of the safe file streams code we added around a week ago. it'd be nice to have this change in before alpha, so we ship with the updated interface, before consumers start using it.

Comment on attachment 153024 [details] [diff] [review] v1 We have another alpha for this, and 1.8a2 already has one foot out the door. Also, Asa would kill me.

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

+ mWriteResult = NS_ERROR_FAILURE; would another error code be better? like, NS_ERROR_LOSS_OF_SIGNIFICANT_DATA or NS_BASE_STREAM_OSERROR?

also... could you add a comment in the idl file that Close should never be called on a safefileoutputstream, unless you want the data thrown away?

+ * to |write| succeeded and the stream was successfully closed. If the + * stream is closed by calling |close| directly, or the stream goes away, + * the original file will not be overwritten, and the temporary file will + * be deleted. that's what's in the patch at the moment... is that okay? and about the rv code... hmm, we don't really know that it's an OS error. i like the NS_ERROR_LOSS... one though. darin, any objections?

hmm, maybe i'll shift that comment sentence into the comment block before the iface definition, it might belong better there.

(In reply to comment #13) > that's what's in the patch at the moment... is that okay? oh, sorry, missed that. but yeah, I do think it fits better in the comment before the interface

this is what i checked in (and what should go on the aviary1.0 branch).