CommandLineOption callback can easily go out of scope

[Joseph Wakeling]

The `CommandLineOption` class takes a callback as its first constructor parameter, defining what should be done with the data received from the given command line flag.

However, this callback is not used directly. Instead, the CommandLineOption class stores a wrapper callback, which invokes the original internally. In the `operator()(mir::Server&)` method, this wrapper callback is then added as an init (or pre-init) callback of the mir::Server instance.

In consequence, if the `CommandLineOption` instance goes out of scope before the init (or pre-init) callbacks are actually invoked, the inner callback will no longer be valid and a segfault will result.

This makes it to e.g. use `CommandLineOption` under the hood of other classes which may wish to encapsulate both the addition of a command line option and what to do with the result.

[Joseph Wakeling (webdrake)]

As an example of the use-case described, see e.g. https://github.com/WebDrake/unity-system-compositor/pull/1/commits/83f95752717168d49c4b8d1d67ec7d111fb1c50a.

Here it was possible to work around the scope issue by storing the `CommandLineOption` instance inside a longer-lived class, but it would be nice if such workarounds were not necessary.

[Alan Griffiths (alan-griffiths)]

CommandLineOption and the other "glue" classes were designed with the idea that they'd be passed to MirRunner::run_with() and exist for the lifetime of the server.

It is purely accidental that AddInitCallback (used in the example) doesn't need to exist beyond the initial call to operator().

I don't think it impossible to support your usage for this case: as you imply the wrapper callback could be curried by value. Vis:

    server.add_init_callback([&server, callback = self->callback]{ callback(server); });

But I'm not sure this can work for all these classes, or how useful it really is.

Specifically, why isn't the logic you pass to AddInitCallback invoked by the CommandLineOption? Vis:

    auto check_blacklist_option = pre_init(CommandLineOption(
        [](std::string const& blacklist_regex) { check_blacklist(blacklist_regex); },
        "blacklist", "Video blacklist regex to use", ""))};
    ...
    check_blacklist_option(*config);

AFAIC that would remove the need for usc::Blacklist - a check_blacklist() function would suffice.

[Joseph Wakeling (webdrake)]

Good point re the `usc::Blacklist` class. I do however have other use cases involving multiple different command-line options which all need to be handled before the real init callback can run (this will be useful in separating out the different components of the `SystemCompositor` init callback and the custom command line options it needs).