std::ostream wrapper for network logging device

Question

I want to be able to write logging data out to a network device with the same ease as using std::cout, so I implemented an ostream wrapper. The wrapper uses the C file IO functions to simulate writing to the device. I am looking for feedback on whether or not I have implemented the ostream API correctly.

device_ostream.hpp:

#ifndef DEVICE_OSTREAM_HPP__
#define DEVICE_OSTREAM_HPP__
#include <ostream>
#define BUF_SIZE 100
class device_buffer : public std::streambuf
{
public:
 device_buffer();
 ~device_buffer();
 int overflow(int ch = EOF);
 int sync();
private:
 char buffer_[BUF_SIZE];
 FILE* fp_;
};
class device_ostream : public std::ostream {
public:
 device_ostream() : std::ostream(new device_buffer) {}
 ~device_ostream() {
 delete rdbuf();
 }
};
#endif // DEVICE_OSTREAM_HPP__

device_ostream.cpp:

#include "device_ostream.hpp"
device_buffer::device_buffer() : fp_(0) {
 memset(buffer_, 0, sizeof(buffer_));
 setp(buffer_, buffer_+BUF_SIZE-1);
 fp_=fopen("test.bin","wb"); // simulate our device
}
device_buffer::~device_buffer() {
 if(fp_)
 fclose(fp_); // simulate disconnection from device
}
// ostream fwk notification that buffer is now full
// so write out data now
int device_buffer::overflow(int ch) {
 if(ch != EOF) {
 *pptr() = ch;
 pbump(1);
 }
 return sync();
}
// function to sync up with hardware device. ie we actually write out data here
int device_buffer::sync() {
 if(fp_) {
 fwrite(buffer_, 1, size_t(pptr() - pbase()), fp_);
 // reset put ptr
 setp(pbase(), epptr());
 return 0; // success
 }
 else
 return EOF; // return EOF on device failure
}

Code to exercise:

#include "device_ostream.hpp"
int main () {
 device_ostream os;
 os.write("WRITE", 5);
 os << "Hi there, how are you today???" << "\n";
 os << "Number: " << 33 << std::endl; // std::endl causes sync to be called even if buffer not full yet
 os << "ABCDEFGHIJKLMNOPQRSTUVWXYZ" << std::endl;
 os << "abcdefghijklmnopqrstuvwxyz" << std::endl;
 os << "ABCDEFGHIJKLMNOPQRSTUVWXYZ" << std::endl;
 if(os)
 os << "abcdefghijklmnopqrstuvwxyz" << std::endl;
 return 0;
}

• glampert: That BUF_SIZE should definitely be a member constant of device_buffer.

Answer 1

You should not do this:

class device_ostream : public std::ostream {
public:
 device_ostream() : std::ostream(new device_buffer) {}
 ~device_ostream() {
 delete rdbuf();
 }
};

You do not know if the user of the class has reset the buffer while you were not watching.

device_ostream data;
....
MyImprovedStreamBuffer myBuffer;
data.rdbuf(&myBuffer); // Your destructor will not play well with that.

I would make it an internal member.

class device_ostream : public std::ostream
{
 device_buffer buffer;
 public:
 device_ostream()
 : std::ostream(nullptr)
 {
 rdbuf(&buffer);
 }
};

You should validate the return code of C functions.

fwrite(buffer_, 1, size_t(pptr() - pbase()), fp_);

Does not guarantee that it will write everything you asked for. Personally I would use a lower level write function so I can accurately check the error state and compensate accordingly.

std::size_t written = 0;
std::size_t size = pptr() - pbase();
while(written != size)
{
 std::size output = write(fp_, buffer_ + written, size - written);
 if (output == -1)
 {
 if (errno == EINTR || errno == EAGAIN)
 {
 continue; // Ignore the error and try again.
 }
 // Something bad happened.
 abort(1); // or set the state of your stream to bad in
 // some way (I used abort() simply because I am 
 // to lazy to look up what to do. (you should).
 }
 written += output;
}

The value returned by overflow is incorrect.

According to the specs:

In case of success, the character put is returned, converted to a value of type int_type using member traits_type::to_int_type. Otherwise, it returns the end-of-file value (EOF) either if called with this value as argument c or to signal a failure (some implementations may throw an exception instead).

On success you return 0.

The value returned by sync is incorrect.

According to the specs:

Returns zero, which indicates success. A value of -1 would indicate failure.

On failure you return EOF. Yes I get that it is -1. But your making assumptions. Follow specs to the letter.

Things like this scare me:

 else
 return EOF; // return EOF on device failure

I have been bitten a couple of times were macros expand to more than a single statement. Which renders the block incorrect. As a result I always use '{}' for sub blocks. I recommend it for everything it will prevent those impossible to find errors.

Since we now live in 2014 (nearly 2015). You should be using the modern version of the compiler. So add override to functions that override virtual functions.

int overflow(int ch = EOF) override;
int sync() override;

Definitely worth overriding the method streamsize xsputn (const char* s, streamsize n);

Answer 2

I think you're on the right track. However, there's a lot of missing functionality (Perhaps it would be more helpful to think in terms of std::fstream). For example, you should to be able to set the following options:

1. Set an IP address
2. Set a port number
3. Set a socket type (TCP/UDP)

An example usage of what I'm talking about would look something like this:

int main( int argc, const char* argv[] ) {
 // set ip address, port number, and socket type
 device_ostream os( "127.0.0.1", 5601, device_ostream::udp ); 
 os << "Hello, World\n";
 os.flush();
}

I know that you're looking for a review of the code you posted, but I would suggest you take a look at boost asio. They've already solved many of the design challenges that you'll have to solve at some point. It's also not that difficult to use.