Re: [PATCH v4 10/11] tools/ocaml/xenstored: validate config file before live update

[Edwin Torok <edwin.torok@xxxxxxxxx>]

> On 19 Dec 2022, at 09:37, Christian Lindig <christian.lindig@xxxxxxxxxx> 
> wrote:
> 
> 
> 
>> On 16 Dec 2022, at 18:25, Edwin Török <edvin.torok@xxxxxxxxxx> 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.torok@xxxxxxxxxx
> 
> Acked-by: Christian Lindig <christian.lindig@xxxxxxxxxx>


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.torok@xxxxxxxxxx/
>> ---
>> 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
>> 
>