Libguestfs - Jan 2015 - Re: [PATCH 3/3] sparsify: Ignore read-only LVs (RHBZ#1185561).

virt-sparsify shouldn't die if sparsifying a filesystem that contains
read-only LVs.

https://bugzilla.redhat.com/show_bug.cgi?id=1185561

I thought about trying to make the LV writable temporarily, but I
suspect that if the sysadmin has made the LV read-only, then they
probably did it for a reason, so we shouldn't touch it.

Rich.

You can specify what comparison function is used; and instead of
having it raise 'Not_found', it returns a ~default value.
---
 mllib/common_utils.ml  | 5 +++++
 mllib/common_utils.mli | 4 ++++
 sysprep/main.ml        | 3 +--
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
index 7a8d8a2..d4994cf 100644
--- a/mllib/common_utils.ml
+++ b/mllib/common_utils.ml
@@ -241,6 +241,11 @@ let rec combine3 xs ys zs    | x::xs, y::ys, z::zs ->
(x, y, z) :: combine3 xs ys zs
   | _ -> invalid_arg "combine3"
 
+let rec assoc ?(cmp = compare) ~default x = function
+  | [] -> default
+  | (y, y') :: _ when cmp x y = 0 -> y'
+  | _ :: ys -> assoc ~cmp ~default x ys
+
 let istty chan    Unix.isatty (Unix.descr_of_out_channel chan)
 
diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
index adb436d..ccb2e5f 100644
--- a/mllib/common_utils.mli
+++ b/mllib/common_utils.mli
@@ -58,6 +58,10 @@ val mapi : (int -> 'a -> 'b) -> 'a list
-> 'b list
 val combine3 : 'a list -> 'b list -> 'c list -> ('a *
'b * 'c) list
 (** Like {!List.combine} but for triples.  All lists must be the same length.
*)
 
+val assoc : ?cmp:('a -> 'a -> int) -> default:'b ->
'a -> ('a * 'b) list -> 'b
+(** Like {!List.assoc} but with a user-defined comparison function, and
+    instead of raising [Not_found], it returns the [~default] value. *)
+
 val make_message_function : quiet:bool -> ('a, unit, string, unit)
format4 -> 'a
 (** Timestamped progress messages.  Used for ordinary messages when
     not [--quiet]. *)
diff --git a/sysprep/main.ml b/sysprep/main.ml
index 249800f..8df8db5 100644
--- a/sysprep/main.ml
+++ b/sysprep/main.ml
@@ -230,8 +230,7 @@ read the man page virt-sysprep(1).
     let mount_opts = !mount_opts in
     let mount_opts        List.map (string_split ":") (string_nsplit
";" mount_opts) in
-    let mount_opts mp -      try List.assoc mp mount_opts with Not_found ->
"" in
+    let mount_opts mp = assoc ~default:"" mp mount_opts in
 
     let msg fs = make_message_function ~quiet fs in
     msg (f_"Examining the guest ...");
-- 
2.1.0

---
 mllib/common_utils.ml  | 21 +++++++++++++++++++++
 mllib/common_utils.mli |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
index d4994cf..898be17 100644
--- a/mllib/common_utils.ml
+++ b/mllib/common_utils.ml
@@ -506,6 +506,27 @@ let compare_version v1 v2    in
   compare (split_version v1) (split_version v2)
 
+(* Annoying LVM2 returns a differing UUID strings for different
+ * function calls (sometimes containing or not containing '-'
+ * characters), so we have to normalize each string before
+ * comparison.  c.f. 'compare_pvuuids' in virt-filesystem.
+ *)
+let compare_lvm2_uuids uuid1 uuid2 +  let n1 = String.length uuid1 and n2 =
String.length uuid2 in
+  let rec loop i1 i2 +    if i1 = n1 && i2 = n2 then 0            (*
matching *)
+    else if i1 >= n1 then 1                 (* different lengths *)
+    else if i2 >= n2 then -1
+    else if uuid1.[i1] = '-' then loop (i1+1) i2 (* ignore '-'
characters *)
+    else if uuid2.[i2] = '-' then loop i1 (i2+1)
+    else (
+      let c = compare uuid1.[i1] uuid2.[i2] in
+      if c <> 0 then c                          (* not matching *)
+      else loop (i1+1) (i2+1)
+    )
+  in
+  loop 0 0
+
 (* Run an external command, slurp up the output as a list of lines. *)
 let external_command ~prog cmd    let chan = Unix.open_process_in cmd in
diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
index ccb2e5f..5d3149a 100644
--- a/mllib/common_utils.mli
+++ b/mllib/common_utils.mli
@@ -106,6 +106,9 @@ val display_long_options : unit -> 'a
 val compare_version : string -> string -> int
 (** Compare two version strings. *)
 
+val compare_lvm2_uuids : string -> string -> int
+(** Compare two LVM2 UUIDs, ignoring '-' characters. *)
+
 val external_command : prog:string -> string -> string list
 (** Run an external command, slurp up the output as a list of lines. *)
 
-- 
2.1.0

---
 sparsify/copying.ml  |  4 +++-
 sparsify/in_place.ml |  4 +++-
 sparsify/utils.ml    | 16 ++++++++++++++++
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/sparsify/copying.ml b/sparsify/copying.ml
index 8d77964..165dd6e 100644
--- a/sparsify/copying.ml
+++ b/sparsify/copying.ml
@@ -216,9 +216,11 @@ You can ignore this warning or change it to a hard failure
using the
     List.exists (fun fs' -> fs = g#canonical_device_name fs')
ignores
   in
 
+  let is_read_only_lv = is_read_only_lv g in
+
   List.iter (
     fun fs ->
-      if not (is_ignored fs) then (
+      if not (is_ignored fs) && not (is_read_only_lv fs) then (
         if List.mem fs zeroes then (
           if not quiet then
             printf (f_"Zeroing %s ...\n%!") fs;
diff --git a/sparsify/in_place.ml b/sparsify/in_place.ml
index 751129e..268784c 100644
--- a/sparsify/in_place.ml
+++ b/sparsify/in_place.ml
@@ -69,9 +69,11 @@ and perform g disk format ignores machine_readable quiet
zeroes      List.exists (fun fs' -> fs = g#canonical_device_name fs')
ignores
   in
 
+  let is_read_only_lv = is_read_only_lv g in
+
   List.iter (
     fun fs ->
-      if not (is_ignored fs) then (
+      if not (is_ignored fs) && not (is_read_only_lv fs) then (
         if List.mem fs zeroes then (
           if not quiet then
             printf (f_"Zeroing %s ...\n%!") fs;
diff --git a/sparsify/utils.ml b/sparsify/utils.ml
index 4de7d08..b541286 100644
--- a/sparsify/utils.ml
+++ b/sparsify/utils.ml
@@ -22,9 +22,25 @@ open Printf
 
 open Common_utils
 
+module G = Guestfs
+
 let prog = Filename.basename Sys.executable_name
 let error ?exit_code fs = error ~prog ?exit_code fs
 let warning fs = warning ~prog fs
 let info fs = info ~prog fs
 
 let quote = Filename.quote
+
+(* Return true if the filesystem is a read-only LV (RHBZ#1185561). *)
+let is_read_only_lv (g : G.guestfs) +  let lvs = Array.to_list (g#lvs_full ())
in
+  let romap = List.map (
+    fun { G.lv_uuid = lv_uuid; lv_attr = lv_attr } ->
+      lv_uuid, lv_attr.[1] = 'r'
+  ) lvs in
+  fun fs ->
+    if g#is_lv fs then (
+      let uuid = g#lvuuid fs in
+      assoc ~cmp:compare_lvm2_uuids ~default:false uuid romap
+    )
+    else false
-- 
2.1.0

On Wednesday 28 January 2015 14:25:38 Richard W.M. Jones
wrote:
> ---
>  sparsify/copying.ml  |  4 +++-
>  sparsify/in_place.ml |  4 +++-
>  sparsify/utils.ml    | 16 ++++++++++++++++
>  3 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/sparsify/copying.ml b/sparsify/copying.ml
> index 8d77964..165dd6e 100644
> --- a/sparsify/copying.ml
> +++ b/sparsify/copying.ml
> @@ -216,9 +216,11 @@ You can ignore this warning or change it to a hard
failure using the
>      List.exists (fun fs' -> fs = g#canonical_device_name fs')
ignores
>    in
>  
> +  let is_read_only_lv = is_read_only_lv g in
> +
>    List.iter (
>      fun fs ->
> -      if not (is_ignored fs) then (
> +      if not (is_ignored fs) && not (is_read_only_lv fs) then (
>          if List.mem fs zeroes then (
>            if not quiet then
>              printf (f_"Zeroing %s ...\n%!") fs;
> diff --git a/sparsify/in_place.ml b/sparsify/in_place.ml
> index 751129e..268784c 100644
> --- a/sparsify/in_place.ml
> +++ b/sparsify/in_place.ml
> @@ -69,9 +69,11 @@ and perform g disk format ignores machine_readable quiet
zeroes >      List.exists (fun fs' -> fs = g#canonical_device_name
fs') ignores
>    in
>  
> +  let is_read_only_lv = is_read_only_lv g in
> +
>    List.iter (
>      fun fs ->
> -      if not (is_ignored fs) then (
> +      if not (is_ignored fs) && not (is_read_only_lv fs) then (
>          if List.mem fs zeroes then (
>            if not quiet then
>              printf (f_"Zeroing %s ...\n%!") fs;
> diff --git a/sparsify/utils.ml b/sparsify/utils.ml
> index 4de7d08..b541286 100644
> --- a/sparsify/utils.ml
> +++ b/sparsify/utils.ml
> @@ -22,9 +22,25 @@ open Printf
>  
>  open Common_utils
>  
> +module G = Guestfs
> +
>  let prog = Filename.basename Sys.executable_name
>  let error ?exit_code fs = error ~prog ?exit_code fs
>  let warning fs = warning ~prog fs
>  let info fs = info ~prog fs
>  
>  let quote = Filename.quote
> +
> +(* Return true if the filesystem is a read-only LV (RHBZ#1185561). *)
> +let is_read_only_lv (g : G.guestfs) > +  let lvs = Array.to_list
(g#lvs_full ()) in
> +  let romap = List.map (
> +    fun { G.lv_uuid = lv_uuid; lv_attr = lv_attr } ->
> +      lv_uuid, lv_attr.[1] = 'r'
> +  ) lvs in
> +  fun fs ->
> +    if g#is_lv fs then (
> +      let uuid = g#lvuuid fs in
> +      assoc ~cmp:compare_lvm2_uuids ~default:false uuid romap
> +    )
> +    else false
This looks to me that it would go through all the LVs, even RW ones,
when is_read_only_lv is invoked, right?

Considering that we get a list of all the LVs anyway when doing:
> +  let is_read_only_lv = is_read_only_lv g in
wouldn't it be better to just get the list of UUIDs of RO LVs, and
looking for 'fs' in that? Considering that in most of the cases LVs
are RW, the list with RO LVs should be small if not empty, and thus
save checks.

-- 
Pino Toscano