There are several ways. Some non-hypothetical ones below:

* No one notices the change or can't be bothered :

if( !Stdio.read_file("/etc/foo") ) webbapp_report_fatal("Unable to read /etc/foo");

In this case it will go from reporting the error to the web application user to producing a white page, a page with missing content or a 500 error.

* Someone notices but just want to prevent the rest of the framework from failing:

catch { if( !Stdio.read_file("/etc/foo") ) webbapp_report_fatal("Unable to open /etc/foo"); };

Now you won't just get a broken web page, you will also not get anything in the logs that could tell you forgot to start the framework with sufficient permissions.

I could take the first problem as a necessary evil if it lead to improvements, but I can't see any improvement here. You go from easy error management where you can check errno if you want details, to a situation where you have to check the return code and set up a catch, and then decode what kind of error the catch received. And the code gets uglier.

Also the change that breaks sprintf("%O","new\nline");, is that reversed?

The benefits are there in the CVS message

revision 1.204 date: 2004/09/01 17:17:08; author: mast; state: Exp; lines: +78 -30 Made read_file, read_bytes, write_file and append_file easier to use by letting them throw on all I/O errors (with the exception that read_file and read_bytes still returns zero if the file doesn't exist at all). For instance, it's not necessary to check the return value from write_file and append_file to ensure that the whole string was written. ----------------------------

and I agree with them. I don't think this breaks much code, if any, only makes the flaws in that code more visible, should it occur. I know that I never check errno after performing Pike file I/O.

I can live with write_file and append_file throwing. It's already documented to do that and you can not use if (Stdio.write_file(...)) as is anyway.

It's the read_file and read_bytes changes I'm against. They only make things worse.

The new behaviour is to throw when there's a permanent error (eg permission denied). There's lots of code that assumes that read_{file,bytes} return zero only when the file doesn't exist. This is now also the implemented behaviour.

On Tue, Jan 09, 2007 at 10:00:03AM +0000, Peter Bortas @ Pike developers forum wrote:

File does not exist is pretty permanent.

no, if the file does not exist, the next line of the code could create it, if it's needed or just go on without it.

if the file does exist and there is no read permission than that is not the same as a file not existing and should be treated differently. fixing it most likely requires user intervention too.

a program that treats non-existing and unreadable files the same is broken. therefore i like this difference of throwing en error on permisson problems because then i can get away with if(read_file()) in simple programs where i don't mind the crash.

I can't think of any offhand where it matters if the file is unreadable or if there is insufficient permissions.

unreadable or insufficient permissions are the same kind of problem, but non-existing is a different kind of problem.

non-existing in most cases is non-critical, whereas unreadable or insufficient permission may be critical.

greetings, martin.

Catching up, and I'd like to give my view of this since I'm responsible for it.

There is lots of code that simply assumes that a zero from read_bytes et al means the file doesn't exist. You can tell since the code happily continues with e.g. some kind of default, or that it simply doesn't process the file at all (think files in a queue). This is very broken behavior for other sorts of I/O errors, as people already have mentioned.

I ran across this on code which happily ignored input because of a permission problem. Nothing was reported, and the system just idled when it was supposed to be doing things. It was of course all too time consuming to figure out what the problem actually was.

My aim with the change was to make the functions as useful as possible, considering they are convenience functions. They should only return for errors that the caller typically wants to handle and throw the rest. My experience, and also all the code I've seen that are using these functions, tells that the error one wants to handle is when the file doesn't exist, and nothing else.

So, the intention is that very little code using these functions should need to be changed. It only starts to behave better since exceptional errors (i.e. installation errors or hardware failures) get reported instead of being essentially ignored. I can only see two cases where changes are necessary:

1. Code that checks errno after getting a zero return value. (I can't recall I've ever seen that.)

2. Code that regularly encounters other (i.e. exceptional, imo) I/O errors but has so far lumped them together with the common nonexisting file error. Such code could probably be improved anyway to report these errors in different ways since they are of different severity.