| Message ID | a9414ef542c7c5c7f1423efdf1a117431ae569b6.1671214525.git.edwin.torok@cloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | OCaml fixes | expand |
> On 16 Dec 2022, at 18:25, Edwin Török <edvin.torok@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. > 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@citrix.com Acked-by: Christian Lindig <christian.lindig@citrix.com> > > > --- > Changes since v2: > * repost of lost patch from 2021: https://patchwork.kernel.org/project/xen-devel/patch/a53934dfa8ef984bffa858cc573cc7a6445bbdc0.1620755942.git.edvin.torok@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 >
> On 19 Dec 2022, at 09:37, Christian Lindig <christian.lindig@citrix.com> wrote: > > > >> On 16 Dec 2022, at 18:25, Edwin Török <edvin.torok@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.torok@citrix.com > > Acked-by: Christian Lindig <christian.lindig@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.torok@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 >> >
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" + 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"); ("--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)
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*. 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@citrix.com> --- Changes since v2: * repost of lost patch from 2021: https://patchwork.kernel.org/project/xen-devel/patch/a53934dfa8ef984bffa858cc573cc7a6445bbdc0.1620755942.git.edvin.torok@citrix.com/ --- tools/ocaml/xenstored/parse_arg.ml | 26 ++++++++++++++++++++++++++ tools/ocaml/xenstored/xenstored.ml | 11 +++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-)