Libguestfs - Sep 2014 - [PATCH 1/3] sysprep: Replace --user-accounts option with --{remove, keep}-user-accounts.

The --user-accounts option, with its double-negative '-' prefix on
user names, is confusing.  Replace it with '--remove-user-accounts'
and '--keep-user-accounts' options.

This updates commit 128d474095bfabc2d724d0181155e364f1afbabf.
---
 sysprep/sysprep_operation_user_account.ml | 60 ++++++++++++++++---------------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/sysprep/sysprep_operation_user_account.ml
b/sysprep/sysprep_operation_user_account.ml
index 3ed1b6e..c514286 100644
--- a/sysprep/sysprep_operation_user_account.ml
+++ b/sysprep/sysprep_operation_user_account.ml
@@ -28,38 +28,26 @@ module G = Guestfs
 
 module StringSet = Set.Make (String)
 
-let users_included = ref StringSet.empty
-let users_excluded = ref StringSet.empty
-let set_users users +let remove_users = ref StringSet.empty
+let keep_users = ref StringSet.empty
+let add_users set users    let users = string_nsplit "," users in
   List.iter (
-    fun user ->
-      let op -        if string_prefix user "-" then
-          `Exclude (String.sub user 1 (String.length user - 1))
-        else
-          `Include user in
-      match op with
-      | `Include "" | `Exclude "" ->
-        eprintf (f_"%s: --user-accounts: empty user name\n")
-          prog;
-        exit 1
-      | `Include n ->
-        users_included := StringSet.add n !users_included;
-        users_excluded := StringSet.remove n !users_excluded
-      | `Exclude n ->
-        users_included := StringSet.remove n !users_included;
-        users_excluded := StringSet.add n !users_excluded
+    function
+    | "" ->
+      error ~prog (f_"user-accounts: empty user name")
+    | user ->
+      set := StringSet.add user !set
   ) users
 
 let check_remove_user user    (* If an user is explicitly excluded, keep it. *)
-  if StringSet.mem user !users_excluded then
+  if StringSet.mem user !keep_users then
     false
   (* If the list of included users is empty (thus no users were explicitly
    * included), or an user is explicitly included, remove it. *)
-  else if StringSet.is_empty !users_included
-          or StringSet.mem user !users_included then
+  else if StringSet.is_empty !remove_users
+      || StringSet.mem user !remove_users then
     true
   (* Any other case, not a reason to remove it. *)
   else
@@ -112,24 +100,38 @@ let op = {
 By default remove all the user accounts and their home directories.
 The \"root\" account is not removed.
 
-See the I<--user-accounts> parameter for a way to specify
+See the I<--remove-user-accounts> parameter for a way to specify
 how to remove only some users, or to not remove some others.");
     extra_args = [
-      { extra_argspec = "--user-accounts", Arg.String set_users,
s_"users" ^ " " ^ s_"Users to remove/keep";
+      { extra_argspec = "--remove-user-accounts", Arg.String
(add_users remove_users), s_"users" ^ " " ^ s_"Users to
remove";
         extra_pod_argval = Some "USERS";
         extra_pod_description = s_"\
-The user accounts to be removed (or not) from the guest.
+The user accounts to be removed from the guest.
 The value of this option is a list of user names separated by comma,
-where specifying an user means it is going to be removed,
-while prepending C<-> in front of it name means it is not removed.
+where specifying an user means it is going to be removed.
 For example:
 
- --user-accounts bob,eve
+ --remove-user-accounts bob,eve
 
 would only remove the user accounts C<bob> and C<eve>.
 
 This option can be specified multiple times."
       };
+
+      { extra_argspec = "--keep-user-accounts", Arg.String (add_users
keep_users), s_"users" ^ " " ^ s_"Users to keep";
+        extra_pod_argval = Some "USERS";
+        extra_pod_description = s_"\
+The user accounts to be kept in the guest.
+The value of this option is a list of user names separated by comma,
+where specifying an user means it is going to be kept.
+For example:
+
+ --keep-user-accounts mary
+
+would keep the user account C<mary>.
+
+This option can be specified multiple times."
+      };
     ];
     perform_on_filesystems = Some user_account_perform;
 }
-- 
2.0.4

---
 sysprep/main.ml               |  6 ++++++
 sysprep/sysprep_operation.ml  | 13 +++++++++++++
 sysprep/sysprep_operation.mli |  9 +++++++++
 3 files changed, 28 insertions(+)

diff --git a/sysprep/main.ml b/sysprep/main.ml
index 5d64538..4919783 100644
--- a/sysprep/main.ml
+++ b/sysprep/main.ml
@@ -221,6 +221,12 @@ read the man page virt-sysprep(1).
   let trace = !trace in
   let verbose = !verbose in
 
+  (* At this point we know which operations are enabled.  So call the
+   * not_enabled_check_args method of all *disabled* operations, so
+   * they have a chance to check for unused command line args.
+   *)
+  Sysprep_operation.not_enabled_check_args ?operations ();
+
   (* Parse the mount options string into a function that maps the
    * mountpoint to the mount options.
    *)
diff --git a/sysprep/sysprep_operation.ml b/sysprep/sysprep_operation.ml
index 19ccfe0..09231cb 100644
--- a/sysprep/sysprep_operation.ml
+++ b/sysprep/sysprep_operation.ml
@@ -43,6 +43,7 @@ type operation = {
   pod_description : string option;
   pod_notes : string option;
   extra_args : extra_arg list;
+  not_enabled_check_args : unit -> unit;
   perform_on_filesystems : filesystem_side_effects callback option;
   perform_on_devices : device_side_effects callback option;
 }
@@ -60,6 +61,7 @@ let defaults = {
   pod_description = None;
   pod_notes = None;
   extra_args = [];
+  not_enabled_check_args = (fun () -> ());
   perform_on_filesystems = None;
   perform_on_devices = None;
 }
@@ -272,6 +274,17 @@ let list_operations ()          op.heading
   ) !all_operations
 
+let not_enabled_check_args ?operations () +  let enabled_ops +    match
operations with
+    | None -> !enabled_by_default_operations
+    | Some opset -> (* just the operation names listed *)
+      OperationSet.elements opset in
+  let all_ops = opset_of_oplist !all_operations in
+  let enabled_ops = opset_of_oplist enabled_ops in
+  let disabled_ops = OperationSet.diff all_ops enabled_ops in
+  OperationSet.iter (fun op -> op.not_enabled_check_args ()) disabled_ops
+
 let compare_operations { order = o1; name = n1 } { order = o2; name = n2 }   
let i = compare o1 o2 in
   if i <> 0 then i else compare n1 n2
diff --git a/sysprep/sysprep_operation.mli b/sysprep/sysprep_operation.mli
index c2057ed..d10ef1e 100644
--- a/sysprep/sysprep_operation.mli
+++ b/sysprep/sysprep_operation.mli
@@ -73,6 +73,11 @@ type operation = {
       You can decide the types of the arguments, whether they are
       mandatory etc. *)
 
+  not_enabled_check_args : unit -> unit;
+  (** If the operation is [not] enabled (or disabled), this function is
+      called after argument parsing and can be used to check that
+      no useless extra_args were passed by the user. *)
+
   perform_on_filesystems : filesystem_side_effects callback option;
   (** The function which is called to perform this operation, when
       enabled.
@@ -169,6 +174,10 @@ val remove_all_from_set : set -> set
 (** [remove_all_from_set set] removes from [set] all the available
     operations. *)
 
+val not_enabled_check_args : ?operations:set -> unit -> unit
+(** Call [not_enabled_check_args] on all operations in the set
+    which are {i not} enabled. *)
+
 val perform_operations_on_filesystems : ?operations:set -> verbose:bool
-> quiet:bool -> Guestfs.guestfs -> string ->
filesystem_side_effects -> unit
 (** Perform all operations, or the subset listed in the [operations] set. *)
 
-- 
2.0.4

You will see an error like this:

$ virt-sysprep --remove-user-accounts foo,bar -a /dev/null
virt-sysprep: error: user-accounts: --remove-user-accounts parameter was
used, but the "user-account" operation is not enabled
---
 sysprep/sysprep_operation_user_account.ml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sysprep/sysprep_operation_user_account.ml
b/sysprep/sysprep_operation_user_account.ml
index c514286..2d231cd 100644
--- a/sysprep/sysprep_operation_user_account.ml
+++ b/sysprep/sysprep_operation_user_account.ml
@@ -134,6 +134,11 @@ This option can be specified multiple times."
       };
     ];
     perform_on_filesystems = Some user_account_perform;
+    not_enabled_check_args = fun () ->
+      if not (StringSet.is_empty !keep_users) then
+        error ~prog (f_"user-accounts: --keep-user-accounts parameter was
used, but the \"user-account\" operation is not enabled");
+      if not (StringSet.is_empty !remove_users) then
+        error ~prog (f_"user-accounts: --remove-user-accounts parameter
was used, but the \"user-account\" operation is not enabled");
 }
 
 let () = register_operation op
-- 
2.0.4