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

[Christian Lindig <christian.lindig@xxxxxxxxxx>]

> 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.


> 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>


> >
> ---
> 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
>