Libguestfs - Jul 2015 - [PATCH 3/3] sysprep: rework and fix cron-spool operation (RHBZ#1229305)

This way it is possible to use rm_rf_only_files, but not removing
specific files.
---
 mllib/common_utils.ml  | 8 +++++++-
 mllib/common_utils.mli | 5 ++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
index 516cff3..3737b4c 100644
--- a/mllib/common_utils.ml
+++ b/mllib/common_utils.ml
@@ -640,13 +640,19 @@ let rmdir_on_exit   * without removing the actual
directory structure.  Also if 'dir' is
  * not a directory or doesn't exist, ignore it.
  *
+ * The optional filter is used to filter out files which will be
+ * removed: files returning true are not removed.
+ *
  * XXX Could be faster with a specific API for doing this.
  *)
-let rm_rf_only_files (g : Guestfs.guestfs) dir +let rm_rf_only_files (g :
Guestfs.guestfs) ?filter dir    if g#is_dir dir then (
     let files = Array.map (Filename.concat dir) (g#find dir) in
     let files = Array.to_list files in
     let files = List.filter g#is_file files in
+    let files = match filter with
+    | None -> files
+    | Some f -> List.filter (fun x -> not (f x)) files in
     List.iter g#rm files
   )
 
diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
index bf0cba6..a5b0a68 100644
--- a/mllib/common_utils.mli
+++ b/mllib/common_utils.mli
@@ -140,12 +140,15 @@ val unlink_on_exit : string -> unit
 val rmdir_on_exit : string -> unit
 (** Remove a temporary directory on exit (using [rm -rf]). *)
 
-val rm_rf_only_files : Guestfs.guestfs -> string -> unit
+val rm_rf_only_files : Guestfs.guestfs -> ?filter:(string -> bool) ->
string -> unit
 (** Using the libguestfs API, recursively remove only files from the
     given directory.  Useful for cleaning [/var/cache] etc in sysprep
     without removing the actual directory structure.  Also if [dir] is
     not a directory or doesn't exist, ignore it.
 
+    The optional [filter] is used to filter out files which will be
+    removed: files returning true are not removed.
+
     XXX Could be faster with a specific API for doing this. *)
 
 val truncate_recursive : Guestfs.guestfs -> string -> unit
-- 
2.1.0

Collect this small snippet to get the part of a string after the last
occurrency of a character; replace with it the current snippets doing
the same.

Should be just code motion.
---
 customize/password.ml                     | 4 +---
 mllib/common_utils.ml                     | 5 +++++
 mllib/common_utils.mli                    | 3 +++
 sysprep/sysprep_operation_user_account.ml | 4 +---
 v2v/convert_linux.ml                      | 4 +---
 v2v/utils.ml                              | 9 ++-------
 6 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/customize/password.ml b/customize/password.ml
index 25ce901..cb97804 100644
--- a/customize/password.ml
+++ b/customize/password.ml
@@ -97,9 +97,7 @@ let rec set_linux_passwords ?password_crypto (g :
Guestfs.guestfs) root password
   let users = Array.to_list (g#aug_ls "/files/etc/shadow") in
   List.iter (
     fun userpath ->
-      let user -        let i = String.rindex userpath '/' in
-        String.sub userpath (i+1) (String.length userpath -i-1) in
+      let user = last_part_of userpath '/' in
       try
         (* Each line is: "user:[!!]password:..."
          * !! at the front of the password field means the account is locked.
diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
index 3737b4c..8601009 100644
--- a/mllib/common_utils.ml
+++ b/mllib/common_utils.ml
@@ -738,3 +738,8 @@ let guest_arch_compatible guest_arch    | x, y when x = y
-> true
   | "x86_64",
("i386"|"i486"|"i586"|"i686") -> true
   | _ -> false
+
+(** Return the last part of a string, after the specified separator. *)
+let last_part_of str sep +  let i = String.rindex str sep in
+  String.sub str (i+1) (String.length str - (i+1))
diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
index a5b0a68..7d0b38f 100644
--- a/mllib/common_utils.mli
+++ b/mllib/common_utils.mli
@@ -178,3 +178,6 @@ val mkdir_p : string -> int -> unit
 val guest_arch_compatible : string -> bool
 (** Are guest arch and host_cpu compatible, in terms of being able
     to run commands in the libguestfs appliance? *)
+
+val last_part_of : string -> char -> string
+(** Return the last part of a string, after the specified separator. *)
diff --git a/sysprep/sysprep_operation_user_account.ml
b/sysprep/sysprep_operation_user_account.ml
index 3f3b142..fe460b8 100644
--- a/sysprep/sysprep_operation_user_account.ml
+++ b/sysprep/sysprep_operation_user_account.ml
@@ -68,9 +68,7 @@ let user_account_perform g root side_effects          let uid
= userpath ^ "/uid" in
         let uid = g#aug_get uid in
         let uid = int_of_string uid in
-        let username -          let i = String.rindex userpath '/' in
-          String.sub userpath (i+1) (String.length userpath -i-1) in
+        let username = last_part_of userpath '/' in
         if uid >= uid_min && uid <= uid_max
            && check_remove_user username then (
           changed := true;
diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index 7967c0f..cadd515 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -795,10 +795,8 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source         *)
       let mkinitrd_kv          let modpath = kernel.ki_modpath in
-        let len = String.length modpath in
         try
-          let i = String.rindex modpath '/' in
-          String.sub modpath (i+1) (len - (i+1))
+          last_part_of modpath '/'
         with
           Not_found ->
             invalid_arg (sprintf "invalid module path: %s" modpath)
in
diff --git a/v2v/utils.ml b/v2v/utils.ml
index 1f8c357..4d6b08d 100644
--- a/v2v/utils.ml
+++ b/v2v/utils.ml
@@ -176,9 +176,7 @@ let find_virtio_win_drivers virtio_win          let paths =
List.filter (g#is_file ~followsymlinks:false) paths in
         List.map (
           fun path ->
-            let i = String.rindex path '/' in
-            let len = String.length path in
-            let basename = String.sub path (i+1) (len - (i+1)) in
+            let basename = last_part_of path '/' in
             (path, sprintf "%s:%s" virtio_win path,
              basename,
              fun () -> g#read_file path)
@@ -198,10 +196,7 @@ let find_virtio_win_drivers virtio_win            let
lc_path = String.lowercase path in
           let lc_basename = String.lowercase basename in
 
-          let extension -            let i = String.rindex lc_basename
'.' in
-            let len = String.length lc_basename in
-            String.sub lc_basename (i+1) (len - (i+1)) in
+          let extension = last_part_of lc_basename '.' in
 
           (* Skip files without specific extensions. *)
           if extension <> "cat" && extension <>
"inf" &&
-- 
2.1.0

When cleaning the directories with cron/at jobs, remove only files
there, as subdirectories might be used by other systems; for example
in Debian under /var/spool/cron/ there is the atjobs subdirectory with
the actual at queue.

Make sure to not remove .SEQ files anymore, as they represent the at job
counter which is needed by the at daemon. Instead, reset these files to
0.

Furthermore, add also the path to the .SEQ location in Debian-based
systems.
---
 sysprep/sysprep_operation_cron_spool.ml | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/sysprep/sysprep_operation_cron_spool.ml
b/sysprep/sysprep_operation_cron_spool.ml
index 687a7e9..888b97a 100644
--- a/sysprep/sysprep_operation_cron_spool.ml
+++ b/sysprep/sysprep_operation_cron_spool.ml
@@ -18,19 +18,32 @@
 
 open Sysprep_operation
 open Common_gettext.Gettext
+open Common_utils
 
 module G = Guestfs
 
 let cron_spool_perform (g : Guestfs.guestfs) root side_effects -  Array.iter
g#rm_rf (g#glob_expand "/var/spool/cron/*");
+  let is_seq path +    let basename +      try last_part_of path '/'
+      with Not_found -> path in
+    basename = ".SEQ" in
+  let reset f +    if g#is_file f then
+      (* This should overwrite the file in-place, as it's a very
+       * small buffer which will be handled using internal_write.
+       * This way, existing attributes like SELinux labels are
+       * preserved.
+       *)
+      g#write f "00000\n" in
+
+  rm_rf_only_files g ~filter:is_seq "/var/spool/cron/";
+  reset "/var/spool/cron/atjobs/.SEQ";
   Array.iter g#rm (g#glob_expand "/var/spool/atjobs/*");
-  Array.iter g#rm (g#glob_expand "/var/spool/atjobs/.SEQ");
+  reset "/var/spool/atjobs/.SEQ";
   Array.iter g#rm (g#glob_expand "/var/spool/atspool/*");
-  Array.iter
-    (fun path -> if not (g#is_dir path) then g#rm path)
-    (g#glob_expand "/var/spool/at/*");
-  Array.iter g#rm (g#glob_expand "/var/spool/at/.SEQ");
-  Array.iter g#rm (g#glob_expand "/var/spool/at/spool/*")
+  rm_rf_only_files g ~filter:is_seq "/var/spool/at/";
+  reset "/var/spool/at/.SEQ"
 
 let op = {
   defaults with
-- 
2.1.0

On Wed, Jul 01, 2015 at 03:12:54PM +0200, Pino Toscano
wrote:
> Collect this small snippet to get the part of a string after the last
> occurrency of a character; replace with it the current snippets doing
> the same.
> 
> Should be just code motion.
I see a future where we get a customer bug that some utility fails
with a mysterious 'Not_found' exception and no stack trace.

At the very least we should document that last_part_of can raise
Not_found (because of String.rindex).  Even better would be if
last_part_of returns None | Some substring, which would force all the
callers to check for this case and deal with it.

I know the code may already have this bug, but best to kill it now.

Rich.

>  customize/password.ml                     | 4 +---
>  mllib/common_utils.ml                     | 5 +++++
>  mllib/common_utils.mli                    | 3 +++
>  sysprep/sysprep_operation_user_account.ml | 4 +---
>  v2v/convert_linux.ml                      | 4 +---
>  v2v/utils.ml                              | 9 ++-------
>  6 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/customize/password.ml b/customize/password.ml
> index 25ce901..cb97804 100644
> --- a/customize/password.ml
> +++ b/customize/password.ml
> @@ -97,9 +97,7 @@ let rec set_linux_passwords ?password_crypto (g :
Guestfs.guestfs) root password
>    let users = Array.to_list (g#aug_ls "/files/etc/shadow") in
>    List.iter (
>      fun userpath ->
> -      let user > -        let i = String.rindex userpath '/' in
> -        String.sub userpath (i+1) (String.length userpath -i-1) in
> +      let user = last_part_of userpath '/' in
>        try
>          (* Each line is: "user:[!!]password:..."
>           * !! at the front of the password field means the account is
locked.
> diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
> index 3737b4c..8601009 100644
> --- a/mllib/common_utils.ml
> +++ b/mllib/common_utils.ml
> @@ -738,3 +738,8 @@ let guest_arch_compatible guest_arch >    | x, y
when x = y -> true
>    | "x86_64",
("i386"|"i486"|"i586"|"i686") -> true
>    | _ -> false
> +
> +(** Return the last part of a string, after the specified separator. *)
> +let last_part_of str sep > +  let i = String.rindex str sep in
> +  String.sub str (i+1) (String.length str - (i+1))
> diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
> index a5b0a68..7d0b38f 100644
> --- a/mllib/common_utils.mli
> +++ b/mllib/common_utils.mli
> @@ -178,3 +178,6 @@ val mkdir_p : string -> int -> unit
>  val guest_arch_compatible : string -> bool
>  (** Are guest arch and host_cpu compatible, in terms of being able
>      to run commands in the libguestfs appliance? *)
> +
> +val last_part_of : string -> char -> string
> +(** Return the last part of a string, after the specified separator. *)
> diff --git a/sysprep/sysprep_operation_user_account.ml
b/sysprep/sysprep_operation_user_account.ml
> index 3f3b142..fe460b8 100644
> --- a/sysprep/sysprep_operation_user_account.ml
> +++ b/sysprep/sysprep_operation_user_account.ml
> @@ -68,9 +68,7 @@ let user_account_perform g root side_effects >        
let uid = userpath ^ "/uid" in
>          let uid = g#aug_get uid in
>          let uid = int_of_string uid in
> -        let username > -          let i = String.rindex userpath
'/' in
> -          String.sub userpath (i+1) (String.length userpath -i-1) in
> +        let username = last_part_of userpath '/' in
>          if uid >= uid_min && uid <= uid_max
>             && check_remove_user username then (
>            changed := true;
> diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
> index 7967c0f..cadd515 100644
> --- a/v2v/convert_linux.ml
> +++ b/v2v/convert_linux.ml
> @@ -795,10 +795,8 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source >         *)
>        let mkinitrd_kv >          let modpath = kernel.ki_modpath in
> -        let len = String.length modpath in
>          try
> -          let i = String.rindex modpath '/' in
> -          String.sub modpath (i+1) (len - (i+1))
> +          last_part_of modpath '/'
>          with
>            Not_found ->
>              invalid_arg (sprintf "invalid module path: %s"
modpath) in
> diff --git a/v2v/utils.ml b/v2v/utils.ml
> index 1f8c357..4d6b08d 100644
> --- a/v2v/utils.ml
> +++ b/v2v/utils.ml
> @@ -176,9 +176,7 @@ let find_virtio_win_drivers virtio_win >         
let paths = List.filter (g#is_file ~followsymlinks:false) paths in
>          List.map (
>            fun path ->
> -            let i = String.rindex path '/' in
> -            let len = String.length path in
> -            let basename = String.sub path (i+1) (len - (i+1)) in
> +            let basename = last_part_of path '/' in
>              (path, sprintf "%s:%s" virtio_win path,
>               basename,
>               fun () -> g#read_file path)
> @@ -198,10 +196,7 @@ let find_virtio_win_drivers virtio_win >           
let lc_path = String.lowercase path in
>            let lc_basename = String.lowercase basename in
>  
> -          let extension > -            let i = String.rindex
lc_basename '.' in
> -            let len = String.length lc_basename in
> -            String.sub lc_basename (i+1) (len - (i+1)) in
> +          let extension = last_part_of lc_basename '.' in
>  
>            (* Skip files without specific extensions. *)
>            if extension <> "cat" && extension
<> "inf" &&
> -- 
> 2.1.0
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Wed, Jul 01, 2015 at 03:12:53PM +0200, Pino Toscano
wrote:
> This way it is possible to use rm_rf_only_files, but not removing
> specific files.
> ---
>  mllib/common_utils.ml  | 8 +++++++-
>  mllib/common_utils.mli | 5 ++++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
> index 516cff3..3737b4c 100644
> --- a/mllib/common_utils.ml
> +++ b/mllib/common_utils.ml
> @@ -640,13 +640,19 @@ let rmdir_on_exit >   * without removing the
actual directory structure.  Also if 'dir' is
>   * not a directory or doesn't exist, ignore it.
>   *
> + * The optional filter is used to filter out files which will be
> + * removed: files returning true are not removed.
> + *
>   * XXX Could be faster with a specific API for doing this.
>   *)
> -let rm_rf_only_files (g : Guestfs.guestfs) dir > +let rm_rf_only_files
(g : Guestfs.guestfs) ?filter dir >    if g#is_dir dir then (
>      let files = Array.map (Filename.concat dir) (g#find dir) in
>      let files = Array.to_list files in
>      let files = List.filter g#is_file files in
> +    let files = match filter with
> +    | None -> files
> +    | Some f -> List.filter (fun x -> not (f x)) files in
>      List.iter g#rm files
>    )
>  
> diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
> index bf0cba6..a5b0a68 100644
> --- a/mllib/common_utils.mli
> +++ b/mllib/common_utils.mli
> @@ -140,12 +140,15 @@ val unlink_on_exit : string -> unit
>  val rmdir_on_exit : string -> unit
>  (** Remove a temporary directory on exit (using [rm -rf]). *)
>  
> -val rm_rf_only_files : Guestfs.guestfs -> string -> unit
> +val rm_rf_only_files : Guestfs.guestfs -> ?filter:(string -> bool)
-> string -> unit
>  (** Using the libguestfs API, recursively remove only files from the
>      given directory.  Useful for cleaning [/var/cache] etc in sysprep
>      without removing the actual directory structure.  Also if [dir] is
>      not a directory or doesn't exist, ignore it.
>  
> +    The optional [filter] is used to filter out files which will be
> +    removed: files returning true are not removed.
> +
>      XXX Could be faster with a specific API for doing this. *)
>  
>  val truncate_recursive : Guestfs.guestfs -> string -> unit
> -- 
> 2.1.0
ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Wed, Jul 01, 2015 at 03:12:55PM +0200, Pino Toscano
wrote:
> When cleaning the directories with cron/at jobs, remove only files
> there, as subdirectories might be used by other systems; for example
> in Debian under /var/spool/cron/ there is the atjobs subdirectory with
> the actual at queue.
> 
> Make sure to not remove .SEQ files anymore, as they represent the at job
> counter which is needed by the at daemon. Instead, reset these files to
> 0.
> 
> Furthermore, add also the path to the .SEQ location in Debian-based
> systems.
> ---
>  sysprep/sysprep_operation_cron_spool.ml | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/sysprep/sysprep_operation_cron_spool.ml
b/sysprep/sysprep_operation_cron_spool.ml
> index 687a7e9..888b97a 100644
> --- a/sysprep/sysprep_operation_cron_spool.ml
> +++ b/sysprep/sysprep_operation_cron_spool.ml
> @@ -18,19 +18,32 @@
>  
>  open Sysprep_operation
>  open Common_gettext.Gettext
> +open Common_utils
>  
>  module G = Guestfs
>  
>  let cron_spool_perform (g : Guestfs.guestfs) root side_effects > - 
Array.iter g#rm_rf (g#glob_expand "/var/spool/cron/*");
> +  let is_seq path > +    let basename > +      try last_part_of path
'/'
> +      with Not_found -> path in
> +    basename = ".SEQ" in
> +  let reset f > +    if g#is_file f then
> +      (* This should overwrite the file in-place, as it's a very
> +       * small buffer which will be handled using internal_write.
> +       * This way, existing attributes like SELinux labels are
> +       * preserved.
> +       *)
> +      g#write f "00000\n" in
> +
> +  rm_rf_only_files g ~filter:is_seq "/var/spool/cron/";
> +  reset "/var/spool/cron/atjobs/.SEQ";
>    Array.iter g#rm (g#glob_expand "/var/spool/atjobs/*");
> -  Array.iter g#rm (g#glob_expand "/var/spool/atjobs/.SEQ");
> +  reset "/var/spool/atjobs/.SEQ";
>    Array.iter g#rm (g#glob_expand "/var/spool/atspool/*");
> -  Array.iter
> -    (fun path -> if not (g#is_dir path) then g#rm path)
> -    (g#glob_expand "/var/spool/at/*");
> -  Array.iter g#rm (g#glob_expand "/var/spool/at/.SEQ");
> -  Array.iter g#rm (g#glob_expand "/var/spool/at/spool/*")
> +  rm_rf_only_files g ~filter:is_seq "/var/spool/at/";
> +  reset "/var/spool/at/.SEQ"
ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top