[PATCH 0/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
On Wed, Jan 28, 2015 at 05:15:06PM +0100, Pino Toscano wrote:
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.
Yup, makes sense. I'll rework the patch. Rich. -- 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
participants (2)
-
Pino Toscano -
Richard W.M. Jones