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

+namespace mir
+{
...
+class CookieFactory
...
+std::vector<uint8_t> get_random_data(unsigned size);

We reserve the general mir namespace for system setup. These could be in a "cookie" namespace.

~~~~

+class CookieFactory
+{
+public:
+ CookieFactory(std::vector<uint8_t> const& secret);
+ ~CookieFactory() noexcept;
+
+ MirCookie timestamp_to_cookie(uint64_t const& timestamp);
+
+ bool attest_timestamp(MirCookie const& cookie);
+
+private:
+ class CookieImpl;
+ std::unique_ptr<CookieImpl> impl;
+};

I'm still not convinced we should prefer the Cheshire Cat idiom (over Interface Class) here, but if we are taking this approach then impl should be const.

~~~~

+std::string mir_test_framework::udev_recordings_path()
+{
+ std::string bin_path = MIR_BUILD_PREFIX"/bin/udev_recordings";
+ std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
+
+ if (boost::filesystem::exists(bin_path))
+ return bin_path;
+ else if (boost::filesystem::exists(share_path))
+ return share_path;
+
+ BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings in standard search locations"));
+}

Can this really be the right behaviour? An installed test binary looks in the location where it was built? Even though a developer may be making changes there?

The older logic (based on the executable path) seems less surprising.

~~~~

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

This will throw if the secret is too small, but the behaviour and minimum size is not documented anywhere.

I suggest providing a static class member (minimum_secret_size) and documenting the requirement.