Code review comment for lp:~marcustomlinson/unity-scopes-api/reg_file_system_monitor

Minor educational rant below :-)

static const int c_buffer_len = 1024 * (c_event_size + 16);

I'm not too keen on the magic numbers here. I had to read the inotify() man page to figure out what's going on.

A better way to size the read buffer might be to call select() to wait until the descriptor becomes ready for reading. Then use ioctl(fd, FIONREAD, &avail) to get the number of available bytes.
Then use avail to size the buffer for reading.

Minor comment:

int length = read(fd_, buffer, c_buffer_len);

This is a little brittle. Personally, I prefer

int length = read(fd_, buffer, sizeof(buffer));

It doesn't matter all that much here but, in situations where there are several buffers involved, this is a little bit more robust. (Note that this will *not* work if the buffer is dynamically allocated and declared as char* or char[].)

Still better:

string s;
s.reserve(avail);
s.size(avail);
int length = read(fd_, &s[0], s.size());

No more magic numbers that way, and it gets extremely unlikely that a future code modification will pass a mismatched buffer and buffer size to read().

Doubleplusgood:

A Buffer class that's instantiated with a file descriptor and buries all the range checking inside its methods. But that would be overkill here, so please don't do that! :-)

    if (fd_ < 0)
    {
        throw ResourceException("DirWatcher(): inotify_init() failed");
    }

The error message isn't great, because it doesn't say why it broke. Throw a unity::SyscallException instead. That exception takes errno and adds it to the output from e.what().

if (length < 0)
{
    throw ResourceException("DirWatcher::watch_thread(): failed to read from inotify fd");
}

Same here: knowing the errno in this case would be nice, so SyscallException is better, and adding the value of fd to the message would be a nice touch.

When this exception is thrown, it doesn't have anywhere to go because the enclosing try-catch swallows it. So, in effect, all the exception does is cause the thread to exit and all the love of the exception is actually lost :-)

Seeing that a read failure is tantamount to an assertion failure (I can't see any reasonable circumstances under which this might fail), it might be easier to just write a message with the fd and errno to stderr and throw 99 to get out of there :-)

The exception handler sets thread_state_ to Failed, but the Failed state is never used elsewhere. Moreover, the next call to remove_watch() will overwrite the Failed state with the Stopping state.

So, as is, the Failed state has no purpose. If there is a Failed state, add_watch() and remove_watch() should probably throw something when called in the Failed state. Of course, what they throw could be an exception that was deposited by the thread into an exception_ptr member, which would allow the exception caught by the watcher thread to eventually make its way back to the caller :-) The destructor could look at the state and print the exception_ptr what() string to cerr. That would deal with the situation where no call to add_watch() or remove_watch() was made after the watcher thread fell over.