[Libguestfs] [PATCH 1/2] mllib: add and use last_part_of

[Pino Toscano]

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