> On 19 Dec 2022, at 09:37, Christian Lindig <christian.lin...@citrix.com>
> wrote:
>
>
>
>> On 16 Dec 2022, at 18:25, Edwin Török <edvin.to...@citrix.com> wrote:
>>
>> The configuration file can contain typos or various errors that could prevent
>> live update from succeeding (e.g. a flag only valid on a different version).
>> Unknown entries in the config file would be ignored on startup normally,
>> add a strict --config-test that live-update can use to check that the config
>> file
>> is valid *for the new binary*.
>
> Is the configuration tested, checked, or validated? If feel “check" or
> “validate" would convey better what is happening.
The rest of the code talks about validation, so I've renamed this to
config-validate.
>
>
>> For compatibility with running old code during live update recognize
>> --live --help as an equivalent to --config-test.
>>
>> Signed-off-by: Edwin Török <edvin.to...@citrix.com
>
> Acked-by: Christian Lindig <christian.lin...@citrix.com>
Thanks, I've pushed an update commit with this change here:
https://github.com/edwintorok/xen/compare/staging...private/edvint/review5,
in particular
https://github.com/edwintorok/xen/commit/f1a9153bb867bbb5df0f5e17b1ed3348e7ea79f8
Best regards,
--Edwin
>
>>>
>> ---
>> Changes since v2:
>> * repost of lost patch from 2021:
>> https://patchwork.kernel.org/project/xen-devel/patch/a53934dfa8ef984bffa858cc573cc7a6445bbdc0.1620755942.git.edvin.to...@citrix.com/
>> ---
>> tools/ocaml/xenstored/parse_arg.ml | 26 ++++++++++++++++++++++++++
>> tools/ocaml/xenstored/xenstored.ml | 11 +++++++++--
>> 2 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/ocaml/xenstored/parse_arg.ml
>> b/tools/ocaml/xenstored/parse_arg.ml
>> index 1a85b14ef5..b159b91f00 100644
>> --- a/tools/ocaml/xenstored/parse_arg.ml
>> +++ b/tools/ocaml/xenstored/parse_arg.ml
>> @@ -26,8 +26,14 @@ type config =
>> restart: bool;
>> live_reload: bool;
>> disable_socket: bool;
>> + config_test: bool;
>> }
>>
>> +let get_config_filename config_file =
>> + match config_file with
>> + | Some name -> name
>> + | None -> Define.default_config_dir ^ "/oxenstored.conf"
>
> I’d use Filename.concat.
>
>> +
>> let do_argv =
>> let pidfile = ref "" and tracefile = ref "" (* old xenstored compatibility
>> *)
>> and domain_init = ref true
>> @@ -38,6 +44,8 @@ let do_argv =
>> and restart = ref false
>> and live_reload = ref false
>> and disable_socket = ref false
>> + and config_test = ref false
>> + and help = ref false
>> in
>>
>> let speclist =
>> @@ -55,10 +63,27 @@ let do_argv =
>> ("-T", Arg.Set_string tracefile, ""); (* for compatibility *)
>> ("--restart", Arg.Set restart, "Read database on starting");
>> ("--live", Arg.Set live_reload, "Read live dump on startup");
>> + ("--config-test", Arg.Set config_test, "Test validity of config
>> file");
>
> I see the logic how this flag was named but feel starting with a verb
> (“validate”, “check”, “test”) leads to a clearer invocation pattern.
>
>> ("--disable-socket", Arg.Unit (fun () -> disable_socket := true),
>> "Disable socket");
>> + ("--help", Arg.Set help, "Display this list of options")
>> ] in
>> let usage_msg = "usage : xenstored [--config-file <filename>]
>> [--no-domain-init] [--help] [--no-fork] [--reraise-top-level] [--restart]
>> [--disable-socket]" in
>> Arg.parse speclist (fun _ -> ()) usage_msg;
>> + let () =
>> + if !help then begin
>> + if !live_reload then
>> + (*
>> + Transform --live --help into --config-test for backward compat
>> with
>> + running code during live update.
>> + Caller will validate config and exit
>> + *)
>> + config_test := true
>> + else begin
>> + Arg.usage_string speclist usage_msg |> print_endline;
>> + exit 0
>> + end
>> + end
>> + in
>> {
>> domain_init = !domain_init;
>> activate_access_log = !activate_access_log;
>> @@ -70,4 +95,5 @@ let do_argv =
>> restart = !restart;
>> live_reload = !live_reload;
>> disable_socket = !disable_socket;
>> + config_test = !config_test;
>> }
>> diff --git a/tools/ocaml/xenstored/xenstored.ml
>> b/tools/ocaml/xenstored/xenstored.ml
>> index 366437b396..1aaa3e995e 100644
>> --- a/tools/ocaml/xenstored/xenstored.ml
>> +++ b/tools/ocaml/xenstored/xenstored.ml
>> @@ -88,7 +88,7 @@ let default_pidfile = Paths.xen_run_dir ^ "/xenstored.pid"
>>
>> let ring_scan_interval = ref 20
>>
>> -let parse_config filename =
>> +let parse_config ?(strict=false) filename =
>> let pidfile = ref default_pidfile in
>> let options = [
>> ("merge-activate", Config.Set_bool Transaction.do_coalesce);
>> @@ -129,11 +129,12 @@ let parse_config filename =
>> ("xenstored-port", Config.Set_string Domains.xenstored_port); ] in
>> begin try Config.read filename options (fun _ _ -> raise Not_found)
>> with
>> - | Config.Error err -> List.iter (fun (k, e) ->
>> + | Config.Error err as e -> List.iter (fun (k, e) ->
>> match e with
>> | "unknown key" -> eprintf "config: unknown key %s\n" k
>> | _ -> eprintf "config: %s: %s\n" k e
>> ) err;
>> + if strict then raise e
>> | Sys_error m -> eprintf "error: config: %s\n" m;
>> end;
>> !pidfile
>> @@ -358,6 +359,12 @@ let tweak_gc () =
>> let () =
>> Printexc.set_uncaught_exception_handler Logging.fallback_exception_handler;
>> let cf = do_argv in
>> + if cf.config_test then begin
>> + let path = config_filename cf in
>> + let _pidfile:string = parse_config ~strict:true path in
>> + Printf.printf "Configuration valid at %s\n%!" path;
>> + exit 0
>> + end;
>> let pidfile =
>> if Sys.file_exists (config_filename cf) then
>> parse_config (config_filename cf)
>> --
>> 2.34.1
>>
>