Transition from exception-raising Unix.getenv to Sys.getenv_opt with

explicit handling of failure cases. OCaml's stdlib has Sys.getenv_opt since 4.05. Some of the newer code already uses it, and some of the old code handled exceptions (so could nicely be transitioned to handling options instead). Some, however, did not handle failure at all. This commit remedies that. In most cases, getenv is used to query the PATH variable (before adding another directory to it, for example), in which case there is a nice default value of "". In some cases, the environment variable is required to be present to proceed, then there is a failure of some kind raised with the appropriate message. A test case was added to the quality-gate.sh script to prevent introduction of the exception-raising Unix.getenv into new code. Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
stephenchengCloud · Jul 4, 2024 · 99c4356 · 99c4356
1 parent e61e0ac
commit 99c4356
Show file tree

Hide file tree

Showing 15 changed files with 108 additions and 51 deletions.
diff --git a/ocaml/libs/stunnel/stunnel.ml b/ocaml/libs/stunnel/stunnel.ml
@@ -35,27 +35,38 @@ let stunnel_logger = ref ignore
 let timeoutidle = ref None
 
 let init_stunnel_path () =
- try cached_stunnel_path := Some (Unix.getenv "XE_STUNNEL")
- with Not_found ->
- let choices =
- [
- "/opt/xensource/libexec/stunnel/stunnel"
- ; "/usr/sbin/stunnel4"
- ; "/usr/sbin/stunnel"
- ; "/usr/bin/stunnel4"
- ; "/usr/bin/stunnel"
- ]
- in
- let rec choose l =
- match l with
- | [] ->
- raise Stunnel_binary_missing
- | p :: ps -> (
- try Unix.access p [Unix.X_OK] ; p with _ -> choose ps
+ cached_stunnel_path :=
+ Some
+ ( match Sys.getenv_opt "XE_STUNNEL" with
+ | Some x ->
+ x
+ | None ->
+ let choices =
+ [
+ "/opt/xensource/libexec/stunnel/stunnel"
+ ; "/usr/sbin/stunnel4"
+ ; "/usr/sbin/stunnel"
+ ; "/usr/bin/stunnel4"
+ ; "/usr/bin/stunnel"
+ ]
+ in
+
+ let choose l =
+ match
+ List.find_opt
+ (fun el ->
+ try Unix.access el [Unix.X_OK] ; true with _ -> false
+ )
+ l
+ with
+ | Some p ->
+ p
+ | None ->
+ raise Stunnel_binary_missing
+ in
+ let path = choose choices in
+ path
  )
- in
- let path = choose choices in
- cached_stunnel_path := Some path
 
 let stunnel_path () =
  if Option.is_none !cached_stunnel_path then
@@ -150,7 +161,8 @@ let debug_conf_of_bool verbose : string =
  if verbose then "debug=authpriv.7" else "debug=authpriv.5"
 
 let debug_conf_of_env () : string =
- (try Unix.getenv "debug_stunnel" with _ -> "") |> String.lowercase_ascii
+ Option.value (Sys.getenv_opt "debug_stunnel") ~default:""
+ |> String.lowercase_ascii
  |> fun x -> List.mem x ["yes"; "true"; "1"] |> debug_conf_of_bool
 
 let config_file ?(accept = None) config host port =

diff --git a/ocaml/networkd/lib/network_utils.ml b/ocaml/networkd/lib/network_utils.ml
@@ -112,7 +112,11 @@ let check_n_run ?(on_error = default_error_handler) ?(log = true) run_func
  try
  Unix.access script [Unix.X_OK] ;
  (* Use the same $PATH as xapi *)
- let env = [|"PATH=" ^ Sys.getenv "PATH"|] in
+ let env =
+ Option.fold ~none:[||]
+ ~some:(fun p -> [|"PATH=" ^ p|])
+ (Sys.getenv_opt "PATH")
+ in
  if log then
  info "%s %s" script (String.concat " " args) ;
  run_func env script args

diff --git a/ocaml/tapctl/tapctl.ml b/ocaml/tapctl/tapctl.ml
@@ -336,9 +336,12 @@ let canonicalise x =
  if not (Filename.is_relative x) then
  x
  else (* Search the PATH and XCP_PATH for the executable *)
- let paths = Astring.String.cuts ~sep:":" ~empty:false (Sys.getenv "PATH") in
+ let path_env_var = Option.value (Sys.getenv_opt "PATH") ~default:"" in
+ let paths = Astring.String.cuts ~sep:":" ~empty:false path_env_var in
  let xen_paths =
- try Astring.String.cuts ~sep:":" ~empty:false (Sys.getenv "XCP_PATH")
+ try
+ Astring.String.cuts ~sep:":" ~empty:false
+ (Option.value (Sys.getenv_opt "XCP_PATH") ~default:"")
  with _ -> []
  in
  let first_hit =
@@ -361,7 +364,9 @@ let canonicalise x =
 let tap_ctl = canonicalise "tap-ctl"
 
 let invoke_tap_ctl _ cmd args =
- let find x = try [x ^ "=" ^ Sys.getenv x] with _ -> [] in
+ let find x =
+ match Sys.getenv_opt x with Some v -> [x ^ "=" ^ v] | None -> []
+ in
  let env = Array.of_list (find "PATH" @ find "TAPDISK" @ find "TAPDISK2") in
  let stdout, _ = execute_command_get_output ~env tap_ctl (cmd :: args) in
  stdout

diff --git a/ocaml/xapi-idl/lib/coverage/enabled.ml b/ocaml/xapi-idl/lib/coverage/enabled.ml
@@ -9,7 +9,13 @@ module Bisect = struct
  let bisect_file = "BISECT_FILE"
 
  let dump jobid =
- let bisect_prefix = Unix.getenv bisect_file in
+ let bisect_prefix =
+ match Sys.getenv_opt bisect_file with
+ | Some x ->
+ x
+ | None ->
+ D.warn "No $BISECT_FILE default set: %s" __LOC__
+ in
  (* dump coverage information in same location as it would normally get
  dumped on exit, except also embed the jobid to make it easier to group.
  Relies on [open_temp_file] generating a unique filename given a
@@ -39,8 +45,7 @@ module Bisect = struct
  let init_env name =
  let ( // ) = Filename.concat in
  let tmpdir = Filename.get_temp_dir_name () in
- try ignore (Sys.getenv bisect_file)
- with Not_found ->
+ if Option.is_none (Sys.getenv_opt bisect_file) then
  Unix.putenv bisect_file (tmpdir // Printf.sprintf "bisect-%s-" name)
 
  let process body =

diff --git a/ocaml/xapi-idl/lib/xcp_service.ml b/ocaml/xapi-idl/lib/xcp_service.ml
@@ -364,24 +364,22 @@ let canonicalise x =
  if not (Filename.is_relative x) then
  x
  else (* Search the PATH and XCP_PATH for the executable *)
- let paths = split_c ':' (Sys.getenv "PATH") in
+ let paths =
+ split_c ':' (Option.value (Sys.getenv_opt "PATH") ~default:"")
+ in
  let first_hit =
- List.fold_left
- (fun found path ->
- match found with
- | Some _hit ->
- found
- | None ->
- let possibility = Filename.concat path x in
- if Sys.file_exists possibility then Some possibility else None
+ List.find_opt
+ (fun path ->
+ let possibility = Filename.concat path x in
+ Sys.file_exists possibility
  )
- None
  (paths @ !extra_search_path)
  in
  match first_hit with
  | None ->
  warn "Failed to find %s on $PATH ( = %s) or search_path option ( = %s)"
- x (Sys.getenv "PATH")
+ x
+ (Option.value (Sys.getenv_opt "PATH") ~default:"unset")
  (String.concat ":" !extra_search_path) ;
  x
  | Some hit ->

diff --git a/ocaml/xapi-storage-script/examples/volume/org.xen.xcp.storage.plainlvm/common.ml b/ocaml/xapi-storage-script/examples/volume/org.xen.xcp.storage.plainlvm/common.ml
@@ -126,9 +126,13 @@ let canonicalise x =
  if not (Filename.is_relative x) then
  x
  else (* Search the PATH and XCP_PATH for the executable *)
- let paths = Re_str.split colon (Sys.getenv "PATH") in
+ let paths =
+ Re_str.split colon (Option.value (Sys.getenv_opt "PATH") ~default:"")
+ in
  let xen_paths =
- try Re_str.split colon (Sys.getenv "XCP_PATH") with _ -> []
+ try
+ Re_str.split colon (Option.value (Sys.getenv_opt "XCP_PATH") ~default:"")
+ with _ -> []
  in
  let first_hit =
  List.fold_left
@@ -148,8 +152,8 @@ let canonicalise x =
  match first_hit with
  | None ->
  warn "Failed to find %s on $PATH ( = %s) or $XCP_PATH ( = %s)" x
- (Sys.getenv "PATH")
- (try Sys.getenv "XCP_PATH" with Not_found -> "unset") ;
+ (Option.value (Sys.getenv_opt "PATH") ~default:"unset")
+ (Option.value (Sys.getenv_opt "XCP_PATH") ~default:"unset") ;
  x
  | Some hit ->
  hit

diff --git a/ocaml/xapi/helpers.ml b/ocaml/xapi/helpers.ml
@@ -85,7 +85,11 @@ let call_script ?(log_output = Always) ?env ?stdin ?timeout script args =
  Unix.access script [Unix.X_OK] ;
  (* Use the same $PATH as xapi *)
  let env =
- match env with None -> [|"PATH=" ^ Sys.getenv "PATH"|] | Some env -> env
+ match env with
+ | None ->
+ [|"PATH=" ^ Option.value (Sys.getenv_opt "PATH") ~default:""|]
+ | Some env ->
+ env
  in
  let output, _ =
  match stdin with

diff --git a/ocaml/xapi/xapi_host.ml b/ocaml/xapi/xapi_host.ml
@@ -123,7 +123,7 @@ let bugreport_upload ~__context ~host:_ ~url ~options =
  if List.mem_assoc "http_proxy" options then
  List.assoc "http_proxy" options
  else
- try Unix.getenv "http_proxy" with _ -> ""
+ Option.value (Sys.getenv_opt "http_proxy") ~default:""
  in
  let cmd =
  Printf.sprintf "%s %s %s"

diff --git a/ocaml/xapi/xapi_support.ml b/ocaml/xapi/xapi_support.ml
@@ -29,7 +29,7 @@ let do_upload label file url options =
  if List.mem_assoc "http_proxy" options then
  List.assoc "http_proxy" options
  else
- try Unix.getenv "http_proxy" with _ -> ""
+ Option.value (Sys.getenv_opt "http_proxy") ~default:""
  in
  let env = Helpers.env_with_path [("URL", url); ("PROXY", proxy)] in
  match

diff --git a/ocaml/xapi/xha_scripts.ml b/ocaml/xapi/xha_scripts.ml
@@ -60,7 +60,13 @@ let ha_script_m = Mutex.create ()
 let call_script ?log_output script args =
  let path = ha_dir () in
  let script' = Filename.concat path script in
- let env = [|Printf.sprintf "PATH=%s:%s" (Sys.getenv "PATH") path|] in
+ let env =
+ [|
+ Printf.sprintf "PATH=%s:%s"
+ (Option.value (Sys.getenv_opt "PATH") ~default:"")
+ path
+ |]
+ in
  try
  Xapi_stdext_threads.Threadext.Mutex.execute ha_script_m (fun () ->
  Helpers.call_script ?log_output ~env script' args

diff --git a/ocaml/xe-cli/newcli.ml b/ocaml/xe-cli/newcli.ml
@@ -280,7 +280,9 @@ let parse_args =
  (List.filter (fun (k, v) -> not (set_keyword (k, v))) rcs)
  in
  let extras =
- let extra_args = try Sys.getenv "XE_EXTRA_ARGS" with Not_found -> "" in
+ let extra_args =
+ Option.value (Sys.getenv_opt "XE_EXTRA_ARGS") ~default:""
+ in
  let l = ref [] and pos = ref 0 and i = ref 0 in
  while !pos < String.length extra_args do
  if extra_args.[!pos] = ',' then (

diff --git a/ocaml/xe-cli/options.ml b/ocaml/xe-cli/options.ml
@@ -34,7 +34,7 @@ let parse_lines ls =
 
 let read_rc () =
  try
- let home = Sys.getenv "HOME" in
+ let home = Option.value (Sys.getenv_opt "HOME") ~default:"" in
  let rc_file = open_in (home ^ "/.xe") in
  let rec getlines cur =
  try

diff --git a/ocaml/xenopsd/cli/xn.ml b/ocaml/xenopsd/cli/xn.ml
@@ -1061,7 +1061,8 @@ let xenconsoles =
 let vncviewer_binary =
  let n = "vncviewer" in
  let dirs =
- Re.Str.split_delim (Re.Str.regexp_string ":") (Unix.getenv "PATH")
+ Re.Str.split_delim (Re.Str.regexp_string ":")
+ (Option.value (Sys.getenv_opt "PATH") ~default:"")
  in
  List.fold_left
  (fun result dir ->

diff --git a/ocaml/xsh/xsh.ml b/ocaml/xsh/xsh.ml
@@ -100,7 +100,11 @@ let _ =
  let host = Sys.argv.(1) in
  let cmd = Sys.argv.(2) in
  let session =
- try Sys.getenv "XSH_SESSION" with _ -> failwith "Session not provided"
+ match Sys.getenv_opt "XSH_SESSION" with
+ | Some x ->
+ x
+ | None ->
+ failwith "Session not provided"
  in
  let args =
  List.map

diff --git a/quality-gate.sh b/quality-gate.sh
@@ -93,11 +93,23 @@ ocamlyacc () {
  fi
 }
 
+unixgetenv () {
+ N=1
+ UNIXGETENV=$(git grep -P -r -o --count 'getenv(?!_opt)' -- **/*.ml | wc -l)
+ if [ "$UNIXGETENV" -eq "$N" ]; then
+ echo "OK found $UNIXGETENV usages of exception-raising Unix.getenv in OCaml files."
+ else
+ echo "ERROR expected $N usages of exception-raising Unix.getenv in OCaml files, got $UNIXGETENV" 1>&2
+ exit 1
+ fi
+}
+
 list-hd
 verify-cert
 mli-files
 structural-equality
 vtpm-unimplemented
 vtpm-fields
 ocamlyacc
+unixgetenv