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

> Sorry about the long list here, most of it is about fitting in with the design
> styles that have grown in Mir and you would not yet be familiar with. I've
> voted "Needs Fixing" but many of the points might be addressed by discussion.
>
> ~~~~
>

No problem at all :), Thanks for going through in depth!

> src/server/default_server_configuration.cpp
>
> There's a lot of code added here (to generate a seed) that probably doesn't
> belong in a translation unit that is about constructing the right objects to
> configure the system. I'd prefer to see it put in a separate file. (Is there
> any reason it can't be part of the new library?)
>
> ~~~~

This is true, Im not 100% sure why it cant be in the library it self. Ill have to talk to Chris about that.

>
> +void fill_vector_with_random_data(std::vector<uint8_t>& buffer)
>
> Could we not use a return value instead of an out parameter?
>
> ~~~~

Changed!

>
> +std::string mir_test_framework::udev_recordings_path()
> +{
> + std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
> + std::string bin_path = MIR_BUILD_PREFIX"/bin/udev_recordings";
> +
> + if (boost::filesystem::exists(share_path))
> + return share_path;
> + else if (boost::filesystem::exists(bin_path))
> + return bin_path;
> +
> + BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings
> in standard search locations"));
> +}
>
> Can this be right? A development test binary looks in the location where it
> would be installed? Even though a developer may be making changes to the
> version in the build path.
>
> The older logic (based on the executable path) seems less surprising.
>
> ~~~~

Yes... Hmm so my overall thought here is do we need to always install the file to run the tests? Or should we ever be able to run the tests on a build directory? If we just want the test to run off the install location im 100% fine with that. All else I can think about is setting an env variable so we can run it to the build directory (and the test can set it).

Either way im happy to change it.

>
> +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;
> +};
>
> What is the reason for preferring the Cheshire Cat idiom over Interface Class?
> This makes it far harder to substitute alternative implementations (e.g. for
> testing that cookies actually are validated when necessary).
>
> I can also imagine a desire to provide a different verification model in a
> (hypothetical) downstream server.
>
> ~~~~~

Not sure the reasoning here, Ill have to talk to Chris about it. Wouldnt be to hard to switch over to just an interface.
>
> +void mir::Server::set_cookie_secret(std::vector<uint8_t> const& secret)
> +{
> + verify_setting_allowed(self->server_config);
> + self->cookie_factory = std::make_shared<CookieFactory>(secret);
> +}
>
> It is a little surprising that this sets the cookie factory, not the secret. I
> can't immediately think of anything this breaks but...
>
> Between this and the previous, I think this would fit better into Mir's design
> patterns if:
>
> 1. CookieFactory were an interface and CookieImpl a derived class.
> 2. the configuration point were override_the_cookie_factory
>
> ~~~~~

Right, well if we switch to an interface this should be simpler to make clearer. Have to talk to Chris about it was well :).
>
> === added file 'tests/acceptance-tests/test_mir_cookie.cpp'
>
> This reads more like a unit test
>
> ~~~~~
test_cookie_factory sound any better? (Im bad at names)
>
> + mir_discover_tests_with_fd_leak_detection(mir_acceptance_tests LD_PRELOAD
> =libumockdev-preload.so.0 G_SLICE=always-malloc G_DEBUG=gc-friendly)
>
> The acceptance tests (before and after this MP) don't need libumockdev-
> preload.so.0 (which is convenient when running them by hand) why change that?
>
> ~~~~~

This was done because the client code has a test that needs this. Seems I didnt split this as well as I could have. Ill remove it for now!

>
> AFAICS the_cookie_factory() isn't used. I've not looked in the parent branch -
> I assume use (and real acceptance tests) is coming soon?

This is used in the client side of the code, this branch shows what the changes will come (but is broken and I was waiting for the server code to merge to get a client side branch up):

https://code.launchpad.net/~mir-team/mir/attestable-timestamps-client/+merge/270470

The main start of the test is 1142 in the diff.

Thank you!