[PATCH 0/3] fix btrfs subvolume procession in tools
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
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 Thursday 07 July 2016 16:54:20 Richard W.M. Jones wrote:
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
This is what I was writing too, but you were faster :) I think this will fail to build as "device" is unused, so the first line should be: (* Make sure we can determine the device of the mountable. *) ignore (g#mountable_device fs);
Apart from that, this is just a translation of fish/options.c: display_mountpoints_on_failure into OCaml, so ACK if that was fixed.
Indeed, LGTM with the above fix. Thanks, -- Pino Toscano
On Thu, Jul 07, 2016 at 06:04:04PM +0200, Pino Toscano wrote:
On Thursday 07 July 2016 16:54:20 Richard W.M. Jones wrote:
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
This is what I was writing too, but you were faster :) I think this will fail to build as "device" is unused, so the first line should be:
(* Make sure we can determine the device of the mountable. *) ignore (g#mountable_device fs);
Actually I believe Maxim can just drop the 'let device = ...' line completely. 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
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
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
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: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 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? -- Your sincerely, Maxim Perevedentsev
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
participants (3)
-
Maxim Perevedentsev -
Pino Toscano -
Richard W.M. Jones