Libguestfs - Jul 2016 - Re: [PATCH 1/3] mllib: add checking for btrfs subvolume

This patcheset fixes errors in virt-sysprep and virt-sparsify.

Here we have a common functionality: is_btrfs_subvolume.
Doesn't it make sense to turn it into guestfs API?

Also I found an issue. In 'virt-sysprep fs-uuids',
the uuids for ALL filesystems are regenerated
as many times as many roots are in guest.
Is it done intentionally?

Maxim Perevedentsev (3):
  mllib: add checking for btrfs subvolume
  sparsify: fix btrfs subvolume processing in is_read_only_lv
  sysprep: fix btrfs subvolume processing in fs-uuids

 mllib/common_utils.ml                 | 10 ++++++++++
 mllib/common_utils.mli                |  3 +++
 sparsify/utils.ml                     |  3 ++-
 sysprep/sysprep_operation_fs_uuids.ml | 16 +++++++++-------
 4 files changed, 24 insertions(+), 8 deletions(-)

--
1.8.3.1

This is needed to skip btrfs subvolumes from output
of list_filesystems where device is needed.
---
 mllib/common_utils.ml  | 10 ++++++++++
 mllib/common_utils.mli |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
index 77b9acd..30fc5cd 100644
--- a/mllib/common_utils.ml
+++ b/mllib/common_utils.ml
@@ -922,3 +922,13 @@ let inspect_mount_root g ?mount_opts_fn root  
 let inspect_mount_root_ro    inspect_mount_root ~mount_opts_fn:(fun _ ->
"ro")
+
+let is_btrfs_subvolume g fs +  let device = g#mountable_device fs in
+  let subvol +    try
+      g#mountable_subvolume fs
+    with Guestfs.Error msg as exn ->
+      if g#last_errno () = Guestfs.Errno.errno_EINVAL then ""
+      else raise exn in
+  device <> "" && subvol <> ""
diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
index 5b0b9bb..d2ed30c 100644
--- a/mllib/common_utils.mli
+++ b/mllib/common_utils.mli
@@ -330,3 +330,6 @@ val inspect_mount_root : Guestfs.guestfs ->
?mount_opts_fn:(string -> string) ->
 val inspect_mount_root_ro : Guestfs.guestfs -> string -> unit
 (** Like [inspect_mount_root], but mounting every mount point as
     read-only. *)
+
+val is_btrfs_subvolume : Guestfs.guestfs -> string -> bool
+(** Checks if a filesystem is a btrfs subvolume. *)
-- 
1.8.3.1

Calling guestfs_is_lv on btrfs subvolume throws an error.
Here we workaround it by returning 'false' for subvolumes.
---
 sparsify/utils.ml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sparsify/utils.ml b/sparsify/utils.ml
index 9a49504..8d99db1 100644
--- a/sparsify/utils.ml
+++ b/sparsify/utils.ml
@@ -34,7 +34,8 @@ let is_read_only_lv (g : G.guestfs)        if lv_attr.[1] =
'r' then Some lv_uuid else None
   ) lvs in
   fun fs ->
-    if g#is_lv fs then (
+    (* Btrfs subvolumes are NOT read-only LVs *)
+    if not (is_btrfs_subvolume g fs) && g#is_lv fs then (
       let uuid = g#lvuuid fs in
       List.exists (fun u -> compare_lvm2_uuids uuid u = 0) ro_uuids
     )
-- 
1.8.3.1

guestfs_set_uuid wants device as argument.
Moreover, btrfstune -U is unable to set uuid for subvolumes,
only unmounted partitions are supported.

Here we skip btrfs subvolumes.
---
 sysprep/sysprep_operation_fs_uuids.ml | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/sysprep/sysprep_operation_fs_uuids.ml
b/sysprep/sysprep_operation_fs_uuids.ml
index ed4e81a..f125eb0 100644
--- a/sysprep/sysprep_operation_fs_uuids.ml
+++ b/sysprep/sysprep_operation_fs_uuids.ml
@@ -30,13 +30,15 @@ let rec fs_uuids_perform g root side_effects    List.iter
(function
   | _, "unknown" -> ()
   | dev, typ ->
-    let new_uuid = Common_utils.uuidgen () in
-    try
-      g#set_uuid dev new_uuid
-    with
-      G.Error msg ->
-        warning (f_"cannot set random UUID on filesystem %s type %s:
%s")
-          dev typ msg
+    if not (is_btrfs_subvolume g dev) then (
+      let new_uuid = Common_utils.uuidgen () in
+      try
+        g#set_uuid dev new_uuid
+      with
+        G.Error msg ->
+          warning (f_"cannot set random UUID on filesystem %s type %s:
%s")
+            dev typ msg
+    )
   ) fses
 
 let op = {
-- 
1.8.3.1

On Thu, Jul 07, 2016 at 06:04:14PM +0300, Maxim Perevedentsev
wrote:
> This is needed to skip btrfs subvolumes from output
> of list_filesystems where device is needed.
> ---
>  mllib/common_utils.ml  | 10 ++++++++++
>  mllib/common_utils.mli |  3 +++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
> index 77b9acd..30fc5cd 100644
> --- a/mllib/common_utils.ml
> +++ b/mllib/common_utils.ml
> @@ -922,3 +922,13 @@ let inspect_mount_root g ?mount_opts_fn root >  
>  let inspect_mount_root_ro >    inspect_mount_root ~mount_opts_fn:(fun _
-> "ro")
> +
> +let is_btrfs_subvolume g fs > +  let device = g#mountable_device fs in
> +  let subvol > +    try
> +      g#mountable_subvolume fs
> +    with Guestfs.Error msg as exn ->
> +      if g#last_errno () = Guestfs.Errno.errno_EINVAL then ""
> +      else raise exn in
> +  device <> "" && subvol <> ""
Firstly I think device <> "" is always true.

Secondly it wasn't obvious to me until I thought about it that you're
testing if subvol is not equal to the value ("") returned three lines
earlier.

How about this, which is type safe and a lot simpler:

let is_btrfs_subvolume g fs   let device = g#mountable_device fs in
  try
    ignore (g#mountable_subvolume fs); true
  with Guestfs.Error msg as exn ->
    if g#last_errno () = Guestfs.Errno.errno_EINVAL then false
    else raise exn

Apart from that, this is just a translation of fish/options.c:
display_mountpoints_on_failure into OCaml, so ACK if that was fixed.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Thu, Jul 07, 2016 at 06:04:16PM +0300, Maxim Perevedentsev
wrote:
> guestfs_set_uuid wants device as argument.
> Moreover, btrfstune -U is unable to set uuid for subvolumes,
> only unmounted partitions are supported.
> 
> Here we skip btrfs subvolumes.
> ---
>  sysprep/sysprep_operation_fs_uuids.ml | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/sysprep/sysprep_operation_fs_uuids.ml
b/sysprep/sysprep_operation_fs_uuids.ml
> index ed4e81a..f125eb0 100644
> --- a/sysprep/sysprep_operation_fs_uuids.ml
> +++ b/sysprep/sysprep_operation_fs_uuids.ml
> @@ -30,13 +30,15 @@ let rec fs_uuids_perform g root side_effects >   
List.iter (function
>    | _, "unknown" -> ()
>    | dev, typ ->
> -    let new_uuid = Common_utils.uuidgen () in
> -    try
> -      g#set_uuid dev new_uuid
> -    with
> -      G.Error msg ->
> -        warning (f_"cannot set random UUID on filesystem %s type %s:
%s")
> -          dev typ msg
> +    if not (is_btrfs_subvolume g dev) then (
> +      let new_uuid = Common_utils.uuidgen () in
> +      try
> +        g#set_uuid dev new_uuid
> +      with
> +        G.Error msg ->
> +          warning (f_"cannot set random UUID on filesystem %s type
%s: %s")
> +            dev typ msg
> +    )
>    ) fses
This patch is fine, because we want to avoid settings UUIDs on btrfs
subvolumes.

However I'm not convinced that this means my statement before about
set_uuid should take a Mountable instead of a Device is wrong.  It
could still be changed to take a Mountable, and either we'd have to
document that setting UUID on a btrfs subvolume makes no sense, or we
could have set_uuid refuse to work on a subvolume (only on the "main"
btrfs subvolume, if there is such a thing).

Anyway, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Thu, Jul 07, 2016 at 06:04:15PM +0300, Maxim Perevedentsev
wrote:
> Calling guestfs_is_lv on btrfs subvolume throws an error.
> Here we workaround it by returning 'false' for subvolumes.
> ---
>  sparsify/utils.ml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sparsify/utils.ml b/sparsify/utils.ml
> index 9a49504..8d99db1 100644
> --- a/sparsify/utils.ml
> +++ b/sparsify/utils.ml
> @@ -34,7 +34,8 @@ let is_read_only_lv (g : G.guestfs) >        if
lv_attr.[1] = 'r' then Some lv_uuid else None
>    ) lvs in
>    fun fs ->
> -    if g#is_lv fs then (
> +    (* Btrfs subvolumes are NOT read-only LVs *)
> +    if not (is_btrfs_subvolume g fs) && g#is_lv fs then (
>        let uuid = g#lvuuid fs in
>        List.exists (fun u -> compare_lvm2_uuids uuid u = 0) ro_uuids
I really think it would be better for is_lv to take a Mountable.

Then in daemon/lvm.c we can just return false for any mountables which
are passed in where mountable->im_type != MOUNTABLE_DEVICE.

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

On Fri, Jul 08, 2016 at 01:58:07PM +0300, Maxim Perevedentsev
wrote:
> On 07/07/2016 06:59 PM, Richard W.M. Jones wrote:
> >
> >However I'm not convinced that this means my statement before about
> >set_uuid should take a Mountable instead of a Device is wrong.  It
> >could still be changed to take a Mountable, and either we'd have to
> >document that setting UUID on a btrfs subvolume makes no sense, or we
> >could have set_uuid refuse to work on a subvolume (only on the
"main"
> >btrfs subvolume, if there is such a thing).
> >
> >Anyway, ACK.
> >
> >Rich.
> >
> As I understand, one can now only set uuid on a mountable, which is
> - unmounted (especially for btrfs)
> - not a btrfs subvolume
> So you will get code like
> 
> if (mountable->type != MOUTABLE_DEVICE) {
>     reply_with_error();
>     return -1;
> }
> do_something(mountable->device).
> 
> The only thing we get is a more correct error.
> But likely there are many places where one can pass the result of
> list_filesystems.
> Probably it makes sense to change all of them to correctly process
> subvolumes,
> but the patch might be extremely large.
> 
> Implicitly using btrfs device instead of subvolume is even worse, IMO.
> 
> Do I miss something?
Something to solve one day.  The current patch is fine.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top