Add security logging base class

[artivis]

Add security logging plugin base class and accompanying basic classes as defined in DDS-Sec specs.

Note: whereas specs define some long-based enum I went ahead and used long-based enum struct instead.

Signed-off-by: artivis jeremie.deray@canonical.com

[kyrofa]

Fast RTPS seems to use four spaces for indentation. This only seems to be a single space, and elsewhere in the PR there are two. We should probably follow their established format. Try to make the code look like it was written by them.

[artivis]

Yeah I will take care of 'fixing' all indentations...

[kyrofa]

This section seems to outline the same information as 9.6, but with different names. I suspect we can remove this struct and use LoggingLevel instead, which seems to be what RTI has done. We should say something on the RTI forums or ping eProsima to make sure we're right, and to raise awareness of the error.

[artivis]

👍

[kyrofa]

Check other classes in Fast RTPS, make sure we're following the same format (I think these are indented).

[kyrofa]

Could probably flesh out the descriptions using the spec.

[artivis]

Will document properly once the actual implementation is done.

[kyrofa]

Should we handle the case where a log file wasn't provided, and distribute is false? Does that seem like an error?

[artivis]

I don't think that's an error but maybe we should warn somehow.

[kyrofa]

Is there a use-case for having this public instead of protected?

[artivis]

Options must be set before calling enable. This seemed like a handy way to assert that options are indeed set. But maybe it is not necessary at all.

[kyrofa]

I think it'd be easier to maintain if this were a standalone function in an anonymous namespace as opposed to a lambda.

[kyrofa]

First of all, I suggest getting rid of the while and just using the predicate, e.g.

has_data_.wait(lock, [&]{ return !queue_.empty(); });

However, this design requires all users of this class to utilize the hack that you have in the BuiltinLogging class' destructor, pushing an empty message on to force a thread wakeup. I recommend a few things that could help this limitation.

First of all, add a stop_ atomic bool member variable (default false, obviously). Then update the predicate here to be:

has_data_.wait(lock, [&]{ return !stop_.load() && !queue_.empty(); });

Second, add another public member function for stopping:

void ConcurrentQueue::stop()
{
    stop_ = true;
    has_data_.notify_all();
}

Finally, add a destructor that looks something like:

ConcurrentQueue::~ConcurrentQueue()
{
    stop();
}

These together will cause all clients waiting on data from this to wake up and realize that no further data will be forthcoming once the queue is destroyed without hack needed. Sadly, in your case, given that you own the thread and the queue in the same scope, without a refactor you'll need to use the stop() function before waiting for the thread to join.

[artivis]

Removed the while as suggested although it is equivalent.

While the current schema may look hacky, it allows for a desirable feature, the possibility to log all messages stored in the queue prior to the 'stop' call. Given that the logging is the last security object to be destroyed, other security plugins may still have messages to log upon destruction, which is possible here.

Furthermore, I don't like adding a stop mechanism to the queue, it seems very odd to 'stop' a container...
We could abstract the queue and thread in a class but that only make sense if it is re-used somewhere else. I'll leave it as is for now but re-visit it later on see if there is a better design.

[kyrofa]

If you follow my advice about the queue's destructor and notification, you should be able to do away with this member variable entirely and just use the invalid return value as the signal that the thread should shutdown.

[kyrofa]

I suggest going the other way and extracting this whole logic into a map.

[kyrofa]

I assume DDS support will also be streamable? Or do you plan on streaming to an sstream or something? Because that would also work for the file logging. Do we really gain much by having this be a template?