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

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 ch开发者_运维问答eck 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;
    }
} 

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.

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.

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.