Refactor feature processing logic in Smint

The previous feature processing logic was a bit ad hoc with various random conversion functions. Abstract them into one module and refactor a few functions to make their uses more explicit. Also try to encourage users, when processing features, to convert them into `Feature.t` and use the provided functions in this module rather than writing `List.mem` directly. Signed-off-by: Vincent Liu <[email protected]>
xapi-project · Nov 4, 2024 · c07bac3 · c07bac3
1 parent f673d81
commit c07bac3
Show file tree

Hide file tree

Showing 15 changed files with 171 additions and 156 deletions.
diff --git a/ocaml/tests/test_sm_features.ml b/ocaml/tests/test_sm_features.ml
@@ -20,7 +20,7 @@ type sm_data_sequence = {
     (* Text feature list we get back as part of sr_get_driver_info. *)
     raw: string list
   ; (* SMAPIv1 driver info. *)
-    smapiv1_features: Smint.feature list
+    smapiv1_features: Smint.Feature.t list
   ; (* SMAPIv2 driver info. *)
     smapiv2_features: string list
   ; (* SM object created in the database. *)
@@ -40,7 +40,6 @@ let string_of_sm_object sm =
     )
 
 let test_sequences =
-  let open Smint in
   [
     (* Test NFS driver features as of Clearwater. *)
     {
@@ -164,14 +163,14 @@ module ParseSMAPIv1Features = Generic.MakeStateless (struct
   module Io = struct
     type input_t = string list
 
-    type output_t = Smint.feature list
+    type output_t = Smint.Feature.t list
 
     let string_of_input_t = Test_printers.(list string)
 
-    let string_of_output_t = Test_printers.(list Smint.string_of_feature)
+    let string_of_output_t = Test_printers.(list Smint.Feature.string_of)
   end
 
-  let transform = Smint.parse_capability_int64_features
+  let transform = Smint.Feature.parse_capability_int64
 
   let tests =
     `QuickAndAutoDocumented
@@ -183,16 +182,16 @@ end)
 
 module CreateSMAPIv2Features = Generic.MakeStateless (struct
   module Io = struct
-    type input_t = Smint.feature list
+    type input_t = Smint.Feature.t list
 
     type output_t = string list
 
-    let string_of_input_t = Test_printers.(list Smint.string_of_feature)
+    let string_of_input_t = Test_printers.(list Smint.Feature.string_of)
 
     let string_of_output_t = Test_printers.(list string)
   end
 
-  let transform = List.map Smint.string_of_feature
+  let transform = List.map Smint.Feature.string_of
 
   let tests =
     `QuickAndAutoDocumented

diff --git a/ocaml/xapi/sm.ml b/ocaml/xapi/sm.ml
@@ -117,7 +117,7 @@ let sr_detach ~dbg dconf driver sr =
 let sr_probe ~dbg dconf driver sr_sm_config =
   with_dbg ~dbg ~name:"sr_probe" @@ fun di ->
   let dbg = Debug_info.to_string di in
-  if List.mem_assoc Sr_probe (features_of_driver driver) then
+  if List.mem_assoc Feature.Sr_probe (features_of_driver driver) then
     Locking_helpers.Named_mutex.execute serialize_attach_detach (fun () ->
         debug "sr_probe" driver
           (sprintf "sm_config=[%s]"

diff --git a/ocaml/xapi/sm_exec.ml b/ocaml/xapi/sm_exec.ml
@@ -551,8 +551,8 @@ let parse_sr_get_driver_info driver (xml : Xml.xml) =
   let strings =
     XMLRPC.From.array XMLRPC.From.string (safe_assoc "capabilities" info)
   in
-  let features = Smint.parse_capability_int64_features strings in
-  let text_features = List.map Smint.string_of_feature features in
+  let features = Smint.Feature.parse_capability_int64 strings in
+  let text_features = List.map Smint.Feature.string_of features in
   (* Parse the driver options *)
   let configuration =
     List.map

diff --git a/ocaml/xapi/smint.ml b/ocaml/xapi/smint.ml
@@ -21,97 +21,94 @@ open D
 
 type vdi_info = {vdi_info_uuid: string option; vdi_info_location: string}
 
-(** Very primitive first attempt at a set of backend features *)
-type capability =
-  | Sr_create
-  | Sr_delete
-  | Sr_attach
-  | Sr_detach
-  | Sr_scan
-  | Sr_probe
-  | Sr_update
-  | Sr_supports_local_caching
-  | Sr_stats
-  | Sr_metadata
-  | Sr_trim
-  | Sr_multipath
-  | Vdi_create
-  | Vdi_delete
-  | Vdi_attach
-  | Vdi_detach
-  | Vdi_mirror
-  | Vdi_clone
-  | Vdi_snapshot
-  | Vdi_resize
-  | Vdi_activate
-  | Vdi_activate_readonly
-  | Vdi_deactivate
-  | Vdi_update
-  | Vdi_introduce
-  | Vdi_resize_online
-  | Vdi_generate_config
-  | Vdi_attach_offline
-  | Vdi_reset_on_boot
-  | Vdi_configure_cbt
-  | Large_vdi  (** Supports >2TB VDIs *)
-  | Thin_provisioning
-  | Vdi_read_caching
-
-type feature = capability * int64
-
-let string_to_capability_table =
-  [
-    ("SR_CREATE", Sr_create)
-  ; ("SR_DELETE", Sr_delete)
-  ; ("SR_ATTACH", Sr_attach)
-  ; ("SR_DETACH", Sr_detach)
-  ; ("SR_SCAN", Sr_scan)
-  ; ("SR_PROBE", Sr_probe)
-  ; ("SR_UPDATE", Sr_update)
-  ; ("SR_SUPPORTS_LOCAL_CACHING", Sr_supports_local_caching)
-  ; ("SR_METADATA", Sr_metadata)
-  ; ("SR_TRIM", Sr_trim)
-  ; ("SR_MULTIPATH", Sr_multipath)
-  ; ("SR_STATS", Sr_stats)
-  ; ("VDI_CREATE", Vdi_create)
-  ; ("VDI_DELETE", Vdi_delete)
-  ; ("VDI_ATTACH", Vdi_attach)
-  ; ("VDI_DETACH", Vdi_detach)
-  ; ("VDI_MIRROR", Vdi_mirror)
-  ; ("VDI_RESIZE", Vdi_resize)
-  ; ("VDI_RESIZE_ONLINE", Vdi_resize_online)
-  ; ("VDI_CLONE", Vdi_clone)
-  ; ("VDI_SNAPSHOT", Vdi_snapshot)
-  ; ("VDI_ACTIVATE", Vdi_activate)
-  ; ("VDI_ACTIVATE_READONLY", Vdi_activate_readonly)
-  ; ("VDI_DEACTIVATE", Vdi_deactivate)
-  ; ("VDI_UPDATE", Vdi_update)
-  ; ("VDI_INTRODUCE", Vdi_introduce)
-  ; ("VDI_GENERATE_CONFIG", Vdi_generate_config)
-  ; ("VDI_ATTACH_OFFLINE", Vdi_attach_offline)
-  ; ("VDI_RESET_ON_BOOT", Vdi_reset_on_boot)
-  ; ("VDI_CONFIG_CBT", Vdi_configure_cbt)
-  ; ("LARGE_VDI", Large_vdi)
-  ; ("THIN_PROVISIONING", Thin_provisioning)
-  ; ("VDI_READ_CACHING", Vdi_read_caching)
-  ]
-
-let capability_to_string_table =
-  List.map (fun (k, v) -> (v, k)) string_to_capability_table
-
-let string_of_capability c = List.assoc c capability_to_string_table
-
-let string_of_feature (c, v) =
-  Printf.sprintf "%s/%Ld" (string_of_capability c) v
-
-let has_capability (c : capability) fl = List.mem_assoc c fl
-
-let capability_of_feature : feature -> capability = fst
-
-let known_features = List.map fst string_to_capability_table
-
-let parse_string_int64_features features =
-  let scan feature =
+module Feature = struct
+  (** Very primitive first attempt at a set of backend features *)
+  type capability =
+    | Sr_create
+    | Sr_delete
+    | Sr_attach
+    | Sr_detach
+    | Sr_scan
+    | Sr_probe
+    | Sr_update
+    | Sr_supports_local_caching
+    | Sr_stats
+    | Sr_metadata
+    | Sr_trim
+    | Sr_multipath
+    | Vdi_create
+    | Vdi_delete
+    | Vdi_attach
+    | Vdi_detach
+    | Vdi_mirror
+    | Vdi_clone
+    | Vdi_snapshot
+    | Vdi_resize
+    | Vdi_activate
+    | Vdi_activate_readonly
+    | Vdi_deactivate
+    | Vdi_update
+    | Vdi_introduce
+    | Vdi_resize_online
+    | Vdi_generate_config
+    | Vdi_attach_offline
+    | Vdi_reset_on_boot
+    | Vdi_configure_cbt
+    | Large_vdi  (** Supports >2TB VDIs *)
+    | Thin_provisioning
+    | Vdi_read_caching
+
+  type t = capability * int64
+
+  let string_to_capability_table =
+    [
+      ("SR_CREATE", Sr_create)
+    ; ("SR_DELETE", Sr_delete)
+    ; ("SR_ATTACH", Sr_attach)
+    ; ("SR_DETACH", Sr_detach)
+    ; ("SR_SCAN", Sr_scan)
+    ; ("SR_PROBE", Sr_probe)
+    ; ("SR_UPDATE", Sr_update)
+    ; ("SR_SUPPORTS_LOCAL_CACHING", Sr_supports_local_caching)
+    ; ("SR_METADATA", Sr_metadata)
+    ; ("SR_TRIM", Sr_trim)
+    ; ("SR_MULTIPATH", Sr_multipath)
+    ; ("SR_STATS", Sr_stats)
+    ; ("VDI_CREATE", Vdi_create)
+    ; ("VDI_DELETE", Vdi_delete)
+    ; ("VDI_ATTACH", Vdi_attach)
+    ; ("VDI_DETACH", Vdi_detach)
+    ; ("VDI_MIRROR", Vdi_mirror)
+    ; ("VDI_RESIZE", Vdi_resize)
+    ; ("VDI_RESIZE_ONLINE", Vdi_resize_online)
+    ; ("VDI_CLONE", Vdi_clone)
+    ; ("VDI_SNAPSHOT", Vdi_snapshot)
+    ; ("VDI_ACTIVATE", Vdi_activate)
+    ; ("VDI_ACTIVATE_READONLY", Vdi_activate_readonly)
+    ; ("VDI_DEACTIVATE", Vdi_deactivate)
+    ; ("VDI_UPDATE", Vdi_update)
+    ; ("VDI_INTRODUCE", Vdi_introduce)
+    ; ("VDI_GENERATE_CONFIG", Vdi_generate_config)
+    ; ("VDI_ATTACH_OFFLINE", Vdi_attach_offline)
+    ; ("VDI_RESET_ON_BOOT", Vdi_reset_on_boot)
+    ; ("VDI_CONFIG_CBT", Vdi_configure_cbt)
+    ; ("LARGE_VDI", Large_vdi)
+    ; ("THIN_PROVISIONING", Thin_provisioning)
+    ; ("VDI_READ_CACHING", Vdi_read_caching)
+    ]
+
+  let capability_to_string_table =
+    List.map (fun (k, v) -> (v, k)) string_to_capability_table
+
+  let known_features = List.map fst string_to_capability_table
+
+  let string_of_capability c = List.assoc c capability_to_string_table
+
+  let string_of (c, v) = Printf.sprintf "%s/%Ld" (string_of_capability c) v
+
+  let capability_of : t -> capability = fst
+
+  let string_int64_of_string_opt feature =
     match String.split_on_char '/' feature with
     | [] ->
         None
@@ -129,15 +126,31 @@ let parse_string_int64_features features =
     | feature :: _ ->
         error "SM.feature: unknown feature %s" feature ;
         None
-  in
-  features
-  |> List.filter_map scan
-  |> List.sort_uniq (fun (x, _) (y, _) -> compare x y)
 
-let parse_capability_int64_features strings =
-  List.map
-    (function c, v -> (List.assoc c string_to_capability_table, v))
-    (parse_string_int64_features strings)
+  let of_string_int64_opt (c, v) =
+    List.assoc_opt c string_to_capability_table |> Option.map (fun c -> (c, v))
+
+  (** [has_capability c fl] will test weather the required capability [c] is present 
+  in the feature list [fl]. Callers should use this function to test if a feature
+    is available rather than directly using membership functions on a feature list
+    as this function might have special logic for some features. *)
+  let has_capability (c : capability) fl = List.mem_assoc c fl
+
+  (** [parse_string_int64 features] takes a [features] list in its plain string
+  forms such as "VDI_MIRROR/2" and parses them into the form of (VDI_MIRROR, 2).
+  If the number is malformated, default to (VDI_MIRROR, 1). It will also deduplicate
+  based on the capability ONLY, and randomly choose a verion, based on the order
+  it appears in the input list.
+  *)
+  let parse_string_int64 features =
+    List.filter_map string_int64_of_string_opt features
+    |> List.sort_uniq (fun (x, _) (y, _) -> compare x y)
+
+  (** [parse_capability_int64 features] is similar to [parse_string_int64_features features]
+  but parses the input list into a [t list] *)
+  let parse_capability_int64 features =
+    parse_string_int64 features |> List.filter_map of_string_int64_opt
+end
 
 type sr_driver_info = {
     sr_driver_filename: string
@@ -147,7 +160,7 @@ type sr_driver_info = {
   ; sr_driver_copyright: string
   ; sr_driver_version: string
   ; sr_driver_required_api_version: string
-  ; sr_driver_features: feature list
+  ; sr_driver_features: Feature.t list
   ; sr_driver_text_features: string list
   ; sr_driver_configuration: (string * string) list
   ; sr_driver_required_cluster_stack: string list

diff --git a/ocaml/xapi/storage_mux.ml b/ocaml/xapi/storage_mux.ml
@@ -37,7 +37,7 @@ type plugin = {
     processor: processor
   ; backend_domain: string
   ; query_result: query_result
-  ; features: Smint.feature list
+  ; features: Smint.Feature.t list
 }
 
 let plugins : (sr, plugin) Hashtbl.t = Hashtbl.create 10
@@ -53,7 +53,7 @@ let debug_printer rpc call =
 let register sr rpc d info =
   with_lock m (fun () ->
       let features =
-        Smint.parse_capability_int64_features info.Storage_interface.features
+        Smint.Feature.parse_capability_int64 info.Storage_interface.features
       in
       Hashtbl.replace plugins sr
         {
@@ -88,7 +88,7 @@ let sr_has_capability sr capability =
   with_lock m (fun () ->
       match Hashtbl.find_opt plugins sr with
       | Some x ->
-          Smint.has_capability capability x.features
+          Smint.Feature.has_capability capability x.features
       | None ->
           false
   )
@@ -648,7 +648,9 @@ module Mux = struct
         | None ->
             failwith "DP not found"
       in
-      if (not read_write) && sr_has_capability sr Smint.Vdi_activate_readonly
+      if
+        (not read_write)
+        && sr_has_capability sr Smint.Feature.Vdi_activate_readonly
       then (
         info "The VDI was attached read-only: calling activate_readonly" ;
         C.VDI.activate_readonly (Debug_info.to_string di) dp sr vdi vm

diff --git a/ocaml/xapi/storage_smapiv1.ml b/ocaml/xapi/storage_smapiv1.ml
@@ -607,7 +607,9 @@ module SMAPIv1 : Server_impl = struct
                     ~key:"content_id"
             ) ;
             (* If the backend doesn't advertise the capability then do nothing *)
-            if List.mem_assoc Smint.Vdi_activate (Sm.features_of_driver _type)
+            if
+              List.mem_assoc Smint.Feature.Vdi_activate
+                (Sm.features_of_driver _type)
             then
               Sm.vdi_activate ~dbg device_config _type sr self read_write
             else
@@ -638,7 +640,9 @@ module SMAPIv1 : Server_impl = struct
                     ~value:Uuidx.(to_string (make ()))
             ) ;
             (* If the backend doesn't advertise the capability then do nothing *)
-            if List.mem_assoc Smint.Vdi_deactivate (Sm.features_of_driver _type)
+            if
+              List.mem_assoc Smint.Feature.Vdi_deactivate
+                (Sm.features_of_driver _type)
             then
               Sm.vdi_deactivate ~dbg device_config _type sr self
             else

diff --git a/ocaml/xapi/xapi_dr_task.ml b/ocaml/xapi/xapi_dr_task.ml
@@ -101,7 +101,8 @@ let create ~__context ~_type ~device_config ~whitelist =
   (* Check if licence allows disaster recovery. *)
   Pool_features.assert_enabled ~__context ~f:Features.DR ;
   (* Check that the SR type supports metadata. *)
-  if not (List.mem_assoc Smint.Sr_metadata (Sm.features_of_driver _type)) then
+  if not (List.mem_assoc Smint.Feature.Sr_metadata (Sm.features_of_driver _type))
+  then
     raise
       (Api_errors.Server_error
          ( Api_errors.operation_not_allowed

diff --git a/ocaml/xapi/xapi_host.ml b/ocaml/xapi/xapi_host.ml
@@ -2247,7 +2247,7 @@ let enable_local_storage_caching ~__context ~host ~sr =
   let shared = Db.SR.get_shared ~__context ~self:sr in
   let has_required_capability =
     let caps = Sm.features_of_driver ty in
-    List.mem_assoc Smint.Sr_supports_local_caching caps
+    List.mem_assoc Smint.Feature.Sr_supports_local_caching caps
   in
   debug "shared: %b. List.length pbds: %d. has_required_capability: %b" shared
     (List.length pbds) has_required_capability ;

diff --git a/ocaml/xapi/xapi_pool.ml b/ocaml/xapi/xapi_pool.ml
@@ -3104,7 +3104,7 @@ let enable_local_storage_caching ~__context ~self:_ =
       (fun (_, _, srrec) ->
         (not srrec.API.sR_shared)
         && List.length srrec.API.sR_PBDs = 1
-        && List.mem_assoc Smint.Sr_supports_local_caching
+        && List.mem_assoc Smint.Feature.Sr_supports_local_caching
              (Sm.features_of_driver srrec.API.sR_type)
       )
       hosts_and_srs