8:25 a.m.
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.
8:25 a.m.
New subject: [PATCH 1/3] mllib: Add a better List.assoc function.
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
8:25 a.m.
New subject: [PATCH 2/3] mllib: Add function for comparing LVM2 UUIDs, ignoring '-' characters.
---
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
8:25 a.m.
New subject: [PATCH 3/3] sparsify: Ignore read-only LVs (RHBZ#1185561).
---
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
10:15 a.m.
New subject: [PATCH 3/3] sparsify: Ignore read-only LVs (RHBZ#1185561).
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
2:39 p.m.
New subject: [PATCH 3/3] sparsify: Ignore read-only LVs (RHBZ#1185561).
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
3400
days inactive
3400
days old
5 comments
2 participants
participants (2)
-
Pino Toscano -
Richard W.M. Jones