cr-service: Refactor RPC config parsing

[rst0git]

When an additional configuration file is specified via RPC, this file is parsed twice: first at an early stage to load options such as --log-file, --work-dir, and --images-dir; and again after all RPC options and configuration files have been evaluated.

This allows users to overwrite options specified via RPC by the container runtime (e.g., --tcp-established). However, processing the RPC config file twice leads to silently duplicating the values of repeatable options such as --action-script.

To address this problem, we adjust the order of options parsing so that the RPC config file is evaluated only once. This does not introduce any functional changes.

Fixes #2727

[avagin]

Could you explain why we need to parse it twice? Why do we need to handle log-file, workdir before parsing other options? Why we can't handle these option on the second run?

[rst0git]

@avagin The only information I found are the following commit messages:

cr-service: add support for configuration files in RPC mode

Most CRIU options are correctly used by just writing the new values to
the corresponding fields of the opts structure. For the RPC case there
are, however, a few options (output, work_dir, imgs_dir) which need
special handling.
So the RPC configuration file is parsed twice. First time to get output,
work_dir and imgs_dir. Once those are read and correctly used, the RPC
code overwrites all options again by values set by the RPC interface. At
the end the RPC configuration file is read a second time and finally
overwrites the values set via RPC.

Fix RPC configuration file handling

While writing runc test cases to verify that runc correctly uses RPC
configuration files it became clear that some things were not working as
they are supposed to. Looking closer at the code to set log files
via RPC configuration files I discovered that the code seems wrong (at
least I did not understand it any more (or the intentions behind it)).

@adrianreber Do you remember by any chance why we need to parse the RPC configuration file twice?

[avagin]

are, however, a few options (output, work_dir, imgs_dir) which need

I think we need to rework how these options are handled.

[rst0git]

I think we need to rework how these options are handled.

@avagin I've updated the pull request with these changes.

[adrianreber]

Concerning this line from the commit:

This does not introduce any functional changes.

This is not true. Probably better to remove that line. Because you will be loosing early log messages with this change. Output from a couple of functions (check_caps(), kerndat_init() and more) will be written to another log file (or maybe nowhere) if the log file is changed via an additional configuration file. This means the user will now miss some debug output.

[rst0git]

Thanks for the comments Adrian!

Because you will be loosing early log messages with this change.

Early log messages are still preserved with this change. We handle these in log_init() with flush_early_log_buffer().

[adrianreber]

Thanks for the comments Adrian!

Because you will be loosing early log messages with this change.

Early log messages are still preserved with this change. We handle these in log_init().

That is good to know. Maybe add that as a comment in the code or commit.