Code review comment for lp:~mir-team/mir/attestable-timestamps-server

+#include <nettle/hmac.h>
...
+ struct hmac_sha1_ctx ctx;

Surely we don't want to introduce this dependency to user code? (See [1].)

There is no point in this class heirarchy if you are putting the derived class definition in the same header.

An option to consider here would the Named Constructor Idiom.

~~~~

+// Using the NVI Idiom

Nobody takes NVPI seriously - it is a solution in search of a problem.

The principle options to consider are Interface and Cheshire Cat (a.k.a. Pimpl). For a comparison of the different options see[2].

~~~~~

+ CookieFactoryNettle(std::vector<uint8_t> const& secret);

This will throw if the secret is too small, but the behaviour and minimum size is not documented in a doc comment on the function. (Having a non-doc comment in the private section of the class doesn't help the user.)

(But as I don't think this class should be made public anyway the documentation should go on the corresponding factory function.)

~~~~

> > The older logic (based on the executable path) seems less surprising.
> >
> > ~~~~
>
> Not sure what the correct answer is here. What would be the best solution?

I think to first check relative to the executable path (as it did before this MP), then to check the install location.

E.g. if running from a build directory it should pick up the one from e.g. <build>/bin/udev_recordings/ if not it picks up the installed version.

~~~~

[1] http://wiki.hsr.ch/Prog3/files/overload72-FINAL_DesigningHeaderFiles.pdf

[2] http://www.twonine.co.uk/articles/SeparatingInterfaceAndImplementation.pdf