Adding ENOSPC and a few other POSIX error macros to nsresultForErrno()

RESOLVED FIXED in mozilla28

Status

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: ishikawa, Assigned: ishikawa)

Tracking

unspecified
mozilla28
All
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
While trying to fix "Bug 930397 - Failure to handle error during
writing to an almost full file system (TB saving an attachment/FF
downloading)", I noticed that the error of failing PR_Write() is not
propagated properly to callers in nsLocalFile::CopyToNative (File:
mozilla/xpcom/io/nsLocalFileCommon.cpp ) This makes it impossible for
the upper layer caller to display meaningful error message to the
user. Currently, a bland catch-all message ("The download cannot be
saved because an unknown error occurred.") is shown and users are left
with no clue to the cause of the error.

But then, we need a mapping of POSIX errno to NS_ERROR_* type macros
to pass proper error information to the callers.

In mozilla/xpcom/io/nsLocalFileCommon.cpp, I noticed that
nsDownload::CopyMoveFailed() used a macro

    #define NSRESULT_FOR_ERRNO() nsresultForErrno(errno)

defined in
FILE: mozilla/xpcom/io/nsLocalFile.h
to map the value of errno to NS_ERROR_* macros.

So I thought I could use this macro when write failed to write
expected number of octets to a local file (is the typical symptom of
failure to the filled up file system during writing.)

To my dismay, I found out that the macro, and nsresultForErrno()
function in turn, does not implement all the POSIX error values :-(

I do NOT NEED ALL the POSIX error macros, I only need macros for often
seen errors to be handled by nsresultForErrno(errno) for now.

Specifically, for an error scenario, in Bug 930397, I definitely need
the mapping of ENOSPC (e.g., defined under linux as follows.)

    #define	ENOSPC		28	/* No space left on device */

So I decided to add handling of some additional POSIX error macro
values to nsresultForErrno(). In adding ENOSPC to the values handled
nsresultForErrno(), A few more POSIX error macros that may be relevant
to FILE I?O error handling are added also.


A patch to the function follows.

TIA


PS: In doing the above, I noticed a rather unfortunate misguided usage
of NS_ERROR_FILE_TOO_BIG by createUnique() function: I reported the
bug and proposed a patch so that NS_ERROR_FILE_ALREADY_EXISTS is used
instead : Bug 931689 - Strange use of NS_ERROR_FILE_TOO_BIG by
nsLocalFile::CreateUnique: NS_FILE_ALREADY_EXISTS should be used
instead.

With the change in Bug 931689, I can universally convert POSIX EFBIG
to NS_ERROR_FILE_TOO_BIG for the whole the comm-central tree (and thus
the same in the mozilla source tree.)

PPS: this is valid for all POSIX-compliant OSs including MacOSX, I think.
(Or solaris and FreeBSD for that matter.)
(Assignee)

Comment 1

5 years ago
Created attachment 823226 [details] [diff] [review]
PATCH: Adding the processing of a few POSIX errors macros.

This is the patch.

The lack of handling of these important additional POSIX error macros may explain the lack of user-friendly detailed error handling in TB (and FF maybe, too.)

I hope this gets in soon. But I am not sure who the reviewer should be.

TIA
(Assignee)

Updated

5 years ago
See Also: → bug 931689
(Assignee)

Updated

5 years ago
Blocks: 931720
(Assignee)

Updated

5 years ago
Assignee: nobody → ishikawa
(Assignee)

Updated

5 years ago
Attachment #823226 - Flags: review?(benjamin)

Comment 2

5 years ago
Comment on attachment 823226 [details] [diff] [review]
PATCH: Adding the processing of a few POSIX errors macros.

r=me except for the comment about CreateUnique, which needs to continue returning NS_ERROR_FILE_TOO_BIG.
Attachment #823226 - Flags: review?(benjamin) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 825105 [details] [diff] [review]
FINAL PATCH: Adding the processing of a few POSIX errors macros.  (carrying over r= bsmedberg)

Thank you for the review.

I have changed the description to be a note for the newcomer. (I think a comment is necessary here to avoid future misunderstanding.)

If this is OK, then I will set checkin-needed keyword.

TIA
Attachment #823226 - Attachment is obsolete: true
Attachment #825105 - Flags: review?
Attachment #825105 - Flags: feedback?(benjamin)

Updated

5 years ago
Attachment #825105 - Flags: review?
Attachment #825105 - Flags: review+
Attachment #825105 - Flags: feedback?(benjamin)
(Assignee)

Comment 4

5 years ago
Thank you for the review.
I will put in the checkin-needed keyword.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/61d3b1f4614c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.