Improve reporting of I/O, access errors for Emacs

Improve reporting of I/O, access errors for Emacs

[Paul Eggert]

I ran into some issues where Emacs was ignoring reasonably-serious I/O errors. I 
went through the C source code and looked for similar issues, and fixed 
everything I found. The basic idea is that Emacs should signal an error for 
serious and unexpected I/O errors (e.g., EIO, ENAMETOOLONG) while continuing to 
behave as before if the problem is merely a missing file (ENOENT) or is a 
similarly tame error. Because the patch is a bit involved and in some places 
affects MS-Windows code, I've posted it here for review:

https://bugs.gnu.org/37389

A half-dozen or so of the issues I found had simpler fixes, which I've already 
installed into master.

Re: Improve reporting of I/O, access errors for Emacs

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 12 Sep 2019 01:22:09 -0700
> 
> The basic idea is that Emacs should signal an error for serious and
> unexpected I/O errors (e.g., EIO, ENAMETOOLONG)

Why is that a good idea in general?  Emacs should do that when it
needs to operate on a file (like copy or rename or delete it), but
cannot due to some I/O error, that's a given.  But if Emacs just needs
to return some information about a file, and the file turns out to be
inaccessible, why do we need to signal an error instead of just
returning a failure indication?

IOW, I think your changes cause error signaling on too low a level,
where we cannot know whether signaling an error is or isn't a Good
Thing.  We should instead have such logic on higher levels, where the
semantics and the meaning of errors are clear.  Otherwise, we risk
breaking a lot of code which will suddenly need to deal with errors it
never saw before.

Re: Improve reporting of I/O, access errors for Emacs

[Paul Eggert]

On 9/12/19 11:04 AM, Eli Zaretskii wrote:
> We should instead have such logic on higher levels, where the
> semantics and the meaning of errors are clear.

If lower levels suppress the info, logic on higher levels cannot obtain 
the information and therefore cannot deal with the errors. So there must 
be some change to the lower levels, regardless of what changes might be 
advisable at higher levels.

> if Emacs just needs
> to return some information about a file, and the file turns out to be
> inaccessible, why do we need to signal an error

The same reason insert-file-contents signals an error when a file is 
inaccessible: the requested information is not available, and this is an 
exceptional condition.

I should mention that I've been trying out the new code and it works 
fine for me. All tests pass. The exceptional conditions in question are 
rare in practice; I found them only because I was trying out some stress 
tests that didn't work right.

Re: Improve reporting of I/O, access errors for Emacs

[Eli Zaretskii]

> Cc: Emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 12 Sep 2019 11:27:28 -0700
> 
> On 9/12/19 11:04 AM, Eli Zaretskii wrote:
> > We should instead have such logic on higher levels, where the
> > semantics and the meaning of errors are clear.
> 
> If lower levels suppress the info, logic on higher levels cannot obtain 
> the information and therefore cannot deal with the errors. So there must 
> be some change to the lower levels, regardless of what changes might be 
> advisable at higher levels.

I wasn't questioning changes on lower levels, I was questioning
signaling errors there.

> > if Emacs just needs
> > to return some information about a file, and the file turns out to be
> > inaccessible, why do we need to signal an error
> 
> The same reason insert-file-contents signals an error when a file is 
> inaccessible: the requested information is not available, and this is an 
> exceptional condition.

The analogy is incorrect: insert-file-contents is the level where I
think we can signal errors, because that level knows what these errors
mean from the application point of view: the application wanted to
have a file in a buffer, but that couldn't be done.  Same with
copy-file, delete-file, etc.

But your changes signal errors from stuff like emacs_readlinkat,
file_name_case_insensitive_p, and file_directory_p.  On that level, we
have no idea whether an error is critical or not.  We should just set
errno and return an error indication, and then let the caller deal
with that.

> I should mention that I've been trying out the new code and it works 
> fine for me.

That's too small a sample, so it doesn't prove much in my book, sorry.

Re: Improve reporting of I/O, access errors for Emacs

[Paul Eggert]

On 9/12/19 12:34 PM, Eli Zaretskii wrote:
> We should just set
> errno and return an error indication, and then let the caller deal
> with that.

I don't see how that could work. Callers are typically written in Lisp, 
and so can't see errno.

For example, suppose Lisp code calls (file-symlink-p "/a/b/c") and Emacs 
cannot tell whether /a/b/c is a symbolic link because of an I/O error, 
or because of a timeout in network access, or because /a is a symlink 
into an unreadable directory, or whatever. If file-symlink-p merely 
returns nil, there's no way for the Lisp code to know that there was an 
exceptional situation and the Lisp code will continue based on the 
incorrect assumption that /a/b/c was not a symlink.

Instead of signaling these exceptional conditions, we could extend each 
  primitive with a new optional argument saying what to do on failure. 
For example, (file-symlink-p "/a/b/c" t) could signal an exception or 
return a special value for these exceptional cases, whereas 
(file-symlink-p "/a/b/c" nil) would simply return nil. However, such an 
extension would require changes to Lisp code if it is intended to be 
reliable, and most Lisp code would not be changed and would continue to 
be unreliable in these exceptional situations. In contrast, the proposed 
patch means  existing Lisp code will gain reliability without needing to 
be changed.

> That's too small a sample, so it doesn't prove much in my book, sorry.

I've looked at and experimented with a reasonable amount of Elisp code, 
and haven't run into problems with the proposed patch. What's your 
contrary intuition based on? Can you give a realistic example of the 
problems you see from the patch?

Re: Improve reporting of I/O, access errors for Emacs

[Eli Zaretskii]

> Cc: Emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 12 Sep 2019 13:32:16 -0700
> 
> On 9/12/19 12:34 PM, Eli Zaretskii wrote:
> > We should just set
> > errno and return an error indication, and then let the caller deal
> > with that.
> 
> I don't see how that could work. Callers are typically written in Lisp, 
> and so can't see errno.

There's some kind of misunderstanding here: the 3 functions I
mentioned are not Lisp-callable, they are called, directly or
indirectly, by Lisp primitives written in C.  My point was that in the
code of the primitive itself, we have all the information to decide
whether or not to signal an error; but on lower levels, we don't have
enough context to make such decisions, so we should only return an
error indication from those lower levels.

> For example, suppose Lisp code calls (file-symlink-p "/a/b/c") and Emacs 
> cannot tell whether /a/b/c is a symbolic link because of an I/O error, 
> or because of a timeout in network access, or because /a is a symlink 
> into an unreadable directory, or whatever. If file-symlink-p merely 
> returns nil, there's no way for the Lisp code to know that there was an 
> exceptional situation and the Lisp code will continue based on the 
> incorrect assumption that /a/b/c was not a symlink.

I agree that file-symlink-p, being a primitive, can decide to signal
an error in case of I/O errors.  But I wasn't talking about the level
of primitives, I was talking about lower levels.  E.g.,
file_directory_p has no way of making intelligent decisions regarding
whether its caller should or shouldn't signal an error in some case,
because file_directory_p can be called in a variety of use cases and
contexts, about whose details it knows nothing at all.

> > That's too small a sample, so it doesn't prove much in my book, sorry.
> 
> I've looked at and experimented with a reasonable amount of Elisp code, 
> and haven't run into problems with the proposed patch. What's your 
> contrary intuition based on? Can you give a realistic example of the 
> problems you see from the patch?

See above: it sounds wrong to delegate to very low levels an authority
of failing the entire higher-level application, since those low levels
don't have enough context information to make such decisions.

Re: Improve reporting of I/O, access errors for Emacs

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

On Thu, 12 Sep 2019 at 09:23, Paul Eggert <eggert@cs.ucla.edu> wrote:

>
> A half-dozen or so of the issues I found had simpler fixes, which I've
> already
> installed into master.
>

Hi Paul,

At the end of init_callproc in callproc.c, we check that PATH_GAME is
file_accessible_directory_p. On Windows, PATH_GAME begins with a literal
'%emacs_dir%', so this never works, but sets errno to ERROR_ACCESS_DENIED.
Therefore, on every invocation of Emacs, I get this warning:

  Warning: game dir '%emacs_dir%/var/games/emacs': Permission denied

Passing PATH_GAME through w32_relocate first makes it try the correct
filename (in my environment that's "c:\projects\emacs/var/games/emacs"
uninstalled or "c:\msys64\mingw64/var/games/emacs" installed). Those
directories don't exist and this still sets errno to ERROR_ACCESS_DENIED on
Windows. Whether or not the relocation bug gets fixed, you should probably
silently ignore ERROR_ACCESS_DENIED (treat it as ENOENT) there, on Windows.
(In my local copy of the repo, I've just replaced that whole block with
"Vshared_game_score_directory = Qnil", since that's what I'll end up with,
whatever happens.)

[-- Attachment #2: Type: text/html, Size: 1613 bytes --]

Re: Improve reporting of I/O, access errors for Emacs

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Mon, 16 Sep 2019 12:07:09 +0100
> Cc: Emacs Development <Emacs-devel@gnu.org>
> 
> At the end of init_callproc in callproc.c, we check that PATH_GAME is file_accessible_directory_p. On
> Windows, PATH_GAME begins with a literal '%emacs_dir%', so this never works, but sets errno to
> ERROR_ACCESS_DENIED. Therefore, on every invocation of Emacs, I get this warning:
> 
>   Warning: game dir '%emacs_dir%/var/games/emacs': Permission denied

Thanks, should be fixed now.

Re: Improve reporting of I/O, access errors for Emacs

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

On Mon, 16 Sep 2019 at 15:53, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Richard Copley <rcopley@gmail.com>
> > Date: Mon, 16 Sep 2019 12:07:09 +0100
> > Cc: Emacs Development <Emacs-devel@gnu.org>
> >
> > At the end of init_callproc in callproc.c, we check that PATH_GAME is
> file_accessible_directory_p. On
> > Windows, PATH_GAME begins with a literal '%emacs_dir%', so this never
> works, but sets errno to
> > ERROR_ACCESS_DENIED. Therefore, on every invocation of Emacs, I get this
> warning:
> >
> >   Warning: game dir '%emacs_dir%/var/games/emacs': Permission denied
>
> Thanks, should be fixed now.
>

LGTM, thanks.

[-- Attachment #2: Type: text/html, Size: 1135 bytes --]

Re: Improve reporting of I/O, access errors for Emacs

[Paul Eggert]

On 9/13/19 12:37 AM, Eli Zaretskii wrote:
> My point was that in the
> code of the primitive itself, we have all the information to decide
> whether or not to signal an error; but on lower levels, we don't have
> enough context to make such decisions, so we should only return an
> error indication from those lower levels.

OK, thanks for clarifying. I reworked the patch along the lines that you 
suggested, and installed it into master.

Re: Improve reporting of I/O, access errors for Emacs

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

On Wed, 18 Sep 2019 at 03:26, Paul Eggert <eggert@cs.ucla.edu> wrote:

> On 9/13/19 12:37 AM, Eli Zaretskii wrote:
> > My point was that in the
> > code of the primitive itself, we have all the information to decide
> > whether or not to signal an error; but on lower levels, we don't have
> > enough context to make such decisions, so we should only return an
> > error indication from those lower levels.
>
> OK, thanks for clarifying. I reworked the patch along the lines that you
> suggested, and installed it into master.
>

The Windows build is broken again. It seems likely one of the recent
commits affecting file error codes is the culprit. The build fails here:

rm -f bootstrap-emacs.pdmp
./temacs --batch  -l loadup --temacs=pbootstrap
emacs: Testing file: Permission denied,
c:/projects/emacs/share/emacs/27.0.50/etc/NEWS
make[1]: *** [Makefile:817: bootstrap-emacs.pdmp] Error 1
make[1]: Leaving directory '/c/projects/emacs/src'

[-- Attachment #2: Type: text/html, Size: 1345 bytes --]