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.
>
> ~~~~

Done.

>
> +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.
>
> ~~~~

Move to an NVI idiom:
http://www.gotw.ca/publications/mill18.htm
>
> +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.
>
> ~~~~
Not sure what the correct answer is here. What would be the best solution?

>
> + 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.

Done.