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

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                     |  5 +++--
 mllib/common_utils.ml                     |  7 +++++++
 mllib/common_utils.mli                    |  3 +++
 sysprep/sysprep_operation_user_account.ml |  5 +++--
 v2v/convert_linux.ml                      | 10 +++-------
 v2v/utils.ml                              | 16 ++++++++++------
 6 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/customize/password.ml b/customize/password.ml
index 25ce901..d91c4b5 100644
--- a/customize/password.ml
+++ b/customize/password.ml
@@ -98,8 +98,9 @@ let rec set_linux_passwords ?password_crypto (g :
Guestfs.guestfs) root password
   List.iter (
     fun userpath ->
       let user -        let i = String.rindex userpath '/' in
-        String.sub userpath (i+1) (String.length userpath -i-1) in
+        match last_part_of userpath '/' with
+        | Some x -> x
+        | None -> error "password: missing '/' in %s"
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..083c5d5 100644
--- a/mllib/common_utils.ml
+++ b/mllib/common_utils.ml
@@ -738,3 +738,10 @@ 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 +  try
+    let i = String.rindex str sep in
+    Some (String.sub str (i+1) (String.length str - (i+1)))
+  with Not_found -> None
diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
index a5b0a68..550f37f 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 option
+(** 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..78c60d0 100644
--- a/sysprep/sysprep_operation_user_account.ml
+++ b/sysprep/sysprep_operation_user_account.ml
@@ -69,8 +69,9 @@ let user_account_perform g root side_effects          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
+          match last_part_of userpath '/' with
+          | Some x -> x
+          | None -> error "user-accounts: missing '/' in
%s" 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..f5a716f 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -795,13 +795,9 @@ 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))
-        with
-          Not_found ->
-            invalid_arg (sprintf "invalid module path: %s" modpath)
in
+        match last_part_of modpath '/' with
+        | Some x -> x
+        | None -> invalid_arg (sprintf "invalid module path: %s"
modpath) in
 
       if g#is_file ~followsymlinks:true "/sbin/dracut" then (
         (* Dracut. *)
diff --git a/v2v/utils.ml b/v2v/utils.ml
index 2aeba45..976fe85 100644
--- a/v2v/utils.ml
+++ b/v2v/utils.ml
@@ -176,9 +176,11 @@ 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 +              match last_part_of path '/'
with
+              | Some x -> x
+              | None ->
+                error "v2v/find_virtio_win_drivers: missing '/' in
%s" path in
             (path, sprintf "%s:%s" virtio_win path,
              basename,
              fun () -> g#read_file path)
@@ -199,9 +201,11 @@ let find_virtio_win_drivers virtio_win            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
+            match last_part_of lc_basename '.' with
+            | Some x -> x
+            | None ->
+              error "v2v/find_virtio_win_drivers: missing '.' in
%s"
+                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 | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/sysprep/sysprep_operation_cron_spool.ml
b/sysprep/sysprep_operation_cron_spool.ml
index 687a7e9..063f75a 100644
--- a/sysprep/sysprep_operation_cron_spool.ml
+++ b/sysprep/sysprep_operation_cron_spool.ml
@@ -18,19 +18,33 @@
 
 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 +      match last_part_of path '/'
with
+      | Some x -> x
+      | None -> 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 05:49:06PM +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.
> ---
>  customize/password.ml                     |  5 +++--
>  mllib/common_utils.ml                     |  7 +++++++
>  mllib/common_utils.mli                    |  3 +++
>  sysprep/sysprep_operation_user_account.ml |  5 +++--
>  v2v/convert_linux.ml                      | 10 +++-------
>  v2v/utils.ml                              | 16 ++++++++++------
>  6 files changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/customize/password.ml b/customize/password.ml
> index 25ce901..d91c4b5 100644
> --- a/customize/password.ml
> +++ b/customize/password.ml
> @@ -98,8 +98,9 @@ let rec set_linux_passwords ?password_crypto (g :
Guestfs.guestfs) root password
>    List.iter (
>      fun userpath ->
>        let user > -        let i = String.rindex userpath '/' in
> -        String.sub userpath (i+1) (String.length userpath -i-1) in
> +        match last_part_of userpath '/' with
> +        | Some x -> x
> +        | None -> error "password: missing '/' in %s"
userpath in
Best to translate these strings:

  | None -> error (f_"password: missing '/' in %s")
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..083c5d5 100644
> --- a/mllib/common_utils.ml
> +++ b/mllib/common_utils.ml
> @@ -738,3 +738,10 @@ 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 > +  try
> +    let i = String.rindex str sep in
> +    Some (String.sub str (i+1) (String.length str - (i+1)))
> +  with Not_found -> None
> diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
> index a5b0a68..550f37f 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 option
> +(** 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..78c60d0 100644
> --- a/sysprep/sysprep_operation_user_account.ml
> +++ b/sysprep/sysprep_operation_user_account.ml
> @@ -69,8 +69,9 @@ let user_account_perform g root side_effects >        
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
> +          match last_part_of userpath '/' with
> +          | Some x -> x
> +          | None -> error "user-accounts: missing '/' in
%s" 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..f5a716f 100644
> --- a/v2v/convert_linux.ml
> +++ b/v2v/convert_linux.ml
> @@ -795,13 +795,9 @@ 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))
> -        with
> -          Not_found ->
> -            invalid_arg (sprintf "invalid module path: %s"
modpath) in
> +        match last_part_of modpath '/' with
> +        | Some x -> x
> +        | None -> invalid_arg (sprintf "invalid module path:
%s" modpath) in
That should probably be 'error' actually, but the bug already exists :-(
>        if g#is_file ~followsymlinks:true "/sbin/dracut" then (
>          (* Dracut. *)
> diff --git a/v2v/utils.ml b/v2v/utils.ml
> index 2aeba45..976fe85 100644
> --- a/v2v/utils.ml
> +++ b/v2v/utils.ml
> @@ -176,9 +176,11 @@ 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 > +              match last_part_of path
'/' with
> +              | Some x -> x
> +              | None ->
> +                error "v2v/find_virtio_win_drivers: missing
'/' in %s" path in
>              (path, sprintf "%s:%s" virtio_win path,
>               basename,
>               fun () -> g#read_file path)
> @@ -199,9 +201,11 @@ let find_virtio_win_drivers virtio_win >           
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
> +            match last_part_of lc_basename '.' with
> +            | Some x -> x
> +            | None ->
> +              error "v2v/find_virtio_win_drivers: missing '.'
in %s"
> +                lc_basename in
>  
>            (* Skip files without specific extensions. *)
>            if extension <> "cat" && extension
<> "inf" &&
> -- 
> 2.1.0
ACK with (f_...) around the first error parameter.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Wed, Jul 01, 2015 at 05:49:07PM +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 | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/sysprep/sysprep_operation_cron_spool.ml
b/sysprep/sysprep_operation_cron_spool.ml
> index 687a7e9..063f75a 100644
> --- a/sysprep/sysprep_operation_cron_spool.ml
> +++ b/sysprep/sysprep_operation_cron_spool.ml
> @@ -18,19 +18,33 @@
>  
>  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 > +      match last_part_of
path '/' with
> +      | Some x -> x
> +      | None -> 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
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 Wednesday 01 July 2015 17:36:20 Richard W.M. Jones
wrote:
> On Wed, Jul 01, 2015 at 05:49:06PM +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.
> > ---
> >  customize/password.ml                     |  5 +++--
> >  mllib/common_utils.ml                     |  7 +++++++
> >  mllib/common_utils.mli                    |  3 +++
> >  sysprep/sysprep_operation_user_account.ml |  5 +++--
> >  v2v/convert_linux.ml                      | 10 +++-------
> >  v2v/utils.ml                              | 16 ++++++++++------
> >  6 files changed, 29 insertions(+), 17 deletions(-)
> > 
> > diff --git a/customize/password.ml b/customize/password.ml
> > index 25ce901..d91c4b5 100644
> > --- a/customize/password.ml
> > +++ b/customize/password.ml
> > @@ -98,8 +98,9 @@ let rec set_linux_passwords ?password_crypto (g :
Guestfs.guestfs) root password
> >    List.iter (
> >      fun userpath ->
> >        let user > > -        let i = String.rindex userpath
'/' in
> > -        String.sub userpath (i+1) (String.length userpath -i-1) in
> > +        match last_part_of userpath '/' with
> > +        | Some x -> x
> > +        | None -> error "password: missing '/' in
%s" userpath in
> 
> Best to translate these strings:
> 
>   | None -> error (f_"password: missing '/' in %s")
userpath in
Hmm I thought that, being them really internal errors (basically
something like a "polished assert"), it would be better to not bother
translating them.
> 
> >        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..083c5d5 100644
> > --- a/mllib/common_utils.ml
> > +++ b/mllib/common_utils.ml
> > @@ -738,3 +738,10 @@ 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 > > +  try
> > +    let i = String.rindex str sep in
> > +    Some (String.sub str (i+1) (String.length str - (i+1)))
> > +  with Not_found -> None
> > diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
> > index a5b0a68..550f37f 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 option
> > +(** 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..78c60d0 100644
> > --- a/sysprep/sysprep_operation_user_account.ml
> > +++ b/sysprep/sysprep_operation_user_account.ml
> > @@ -69,8 +69,9 @@ let user_account_perform g root side_effects >
>          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
> > +          match last_part_of userpath '/' with
> > +          | Some x -> x
> > +          | None -> error "user-accounts: missing '/'
in %s" 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..f5a716f 100644
> > --- a/v2v/convert_linux.ml
> > +++ b/v2v/convert_linux.ml
> > @@ -795,13 +795,9 @@ 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))
> > -        with
> > -          Not_found ->
> > -            invalid_arg (sprintf "invalid module path: %s"
modpath) in
> > +        match last_part_of modpath '/' with
> > +        | Some x -> x
> > +        | None -> invalid_arg (sprintf "invalid module path:
%s" modpath) in
> 
> That should probably be 'error' actually, but the bug already
exists :-(
Ah ok, I thought there was some reason for it to be an exception...

Thanks,
-- 
Pino Toscano