What's the correct way to do a 'catch all' error check on an fstream output operation?

Question

What's the correct way to check for a general error when sending data to an fstream?

UPDATE: My main concern regards some things I've been hearing about a delay between output and any data being physically written to the hard disk. My assumption was that the command "save_file_obj << save_str" would only send data to some kind of buffer and that the following check "if (save_file_obj.bad())" would not be any use in determining if there was an OS or hardware problem. I just wanted to know what was the definitive "catch all" way to send a string to a file and check to make certain that it was written to the disk, before carrying out any following actions such as closing the program.

I have the following code...

int Saver::output()
{
    save_file_handle.open(file_name.c_str());
    if (save_file_handle.is_open())
    {
        save_file_handle << save_str.c_str();

        if (save_file_handle.bad())
        {
            x_message("Error - failed to save file");
            return 0;
        }

        save_file_handle.close();

        if (save_file_handle.bad())
        {
            x_message("Error - failed to save file");
            return 0;
        }

        return 1;
    }
    else
    {
        x_message("Error - couldn't open save file");
        return 0;
    }
} 

Answer 1

Everything except for the check after the close seems reasonable. That said, I would restructure things slightly differently and throw an exception or use a bool, but that is simply a matter of preference:

bool Saver::output()
{
    std::fstream out(_filename.c_str(),std::ios::out);
    if ( ! out.is_open() ){
         LOG4CXX_ERROR(_logger,"Could not open \""<<filename<<"\"");
         return false;
    }

    out << _savestr << std::endl;
    if ( out.bad() ){
         LOG4CXX_ERROR(_logger,"Could not save to \""<<filename<<"\"");
         out.close();
         return false;
    }

    out.close();
    return true;
}

I should also point out that you don't need to use save_str.c_str(), since C++ iostreams (including fstream, ofstream, etc.) are all capable of outputting std::string objects. Also, if you construct the file stream object in the scope of the function, it will automatically be closed when it goes out of scope.

Answer 2

Are you absolutely sure that save_file_handle doesn't already have a file associated (open) with it? If it does then calling its open() method will fail and raise its ios::failbit error flag -- and any exceptions if set to do so.

The close() method can't fail unless the file isn't open, in which case the method will raise the ios::failbit error flag. At any rate, the destructor should close the file, and do so automatically if the save_file_handle is a stack variable as in your code.

int Saver::output()
{
    save_file_handle.open(file_name.c_str());
    if (save_file_handle.fail())
    {
        x_message("Error - file failed to previously close");
        return 0;
    }
    save_file_handle << save_str.c_str();

    if (save_file_handle.bad())
    {
        x_message("Error - failed to save file");
        return 0;
    }    
    return 1;
}

Alternatively, you can separate the error checking from the file-saving logic if you use ios::exceptions().

int Saver::output()
{
    ios_base::iostate old = save_file_handle.exceptions();
    save_file_handle.exceptions(ios::failbit | ios::badbit);
    try
    {
        save_file_handle.open(file_name.c_str());          
        save_file_handle << save_str.c_str();
    }
    catch (ofstream::failure e)
    {
        x_message("Error - couldn't save file");
        save_file_handle.exceptions(old);
        return 0;
    }
    save_file_handle.exceptions(old);
    return 1;
}

You might prefer to move the call to save_file_handle.exceptions(ios::failbit | ios::badbit) to the constructor(s). Then you can get rid of the statements that reset the exceptions flag.

Answer 3

A few points. Firstly:

save_file_handle

is a poor name for an instance of a C++ fstream. fstreams are not file handles and all this can do is confuse the reader.

Secondly, as Michael pints out, there is no need to convert a C++ string to a C-string. The only time you should really find yourself doing this is when interfacing with C-style APIS, and when using a few poorly designed C++ APIs, such as (unfortunately) fstream::open().

Thirdly, the canonical way to test if a stream operation worked is to test the operation itself. Streams have a conversion to void * which means you can write stuff like this:

if ( save_file_handle << save_str ) {
   // operation worked
}
else {
   // failed for some reason
}

Your code should always testv stream operations, whether for input or output.