Pino Toscano
2015-Jul-01 15:49 UTC
[Libguestfs] [PATCH 1/2] mllib: add and use last_part_of
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
Pino Toscano
2015-Jul-01 15:49 UTC
[Libguestfs] [PATCH 2/2] sysprep: rework and fix cron-spool operation (RHBZ#1229305)
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
Richard W.M. Jones
2015-Jul-01 16:36 UTC
Re: [Libguestfs] [PATCH 1/2] mllib: add and use last_part_of
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 inBest 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) inThat 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.0ACK 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/
Richard W.M. Jones
2015-Jul-01 16:36 UTC
Re: [Libguestfs] [PATCH 2/2] sysprep: rework and fix cron-spool operation (RHBZ#1229305)
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 withACK. 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
Pino Toscano
2015-Jul-01 16:47 UTC
Re: [Libguestfs] [PATCH 1/2] mllib: add and use last_part_of
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 inHmm 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
Possibly Parallel Threads
- [PATCH 1/3] mllib: add an optional filter for rm_rf_only_files
- Re: [PATCH 1/2] mllib: add and use last_part_of
- Re: [PATCH 1/2] mllib: add and use last_part_of
- [PATCH 2/3] mllib: add and use last_part_of
- [PATCH] sysprep: handle at jobs in cron-spool operation