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

[Pino Toscano]

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