10:04 a.m.
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
10:04 a.m.
New subject: [PATCH 1/3] mllib: add checking for btrfs subvolume
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
10:54 a.m.
New subject: [PATCH 1/3] mllib: add checking for btrfs subvolume
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
11:04 a.m.
New subject: [PATCH 1/3] mllib: add checking for btrfs subvolume
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
11:36 a.m.
New subject: [PATCH 1/3] mllib: add checking for btrfs subvolume
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
10:04 a.m.
New subject: [PATCH 2/3] sparsify: fix btrfs subvolume processing in is_read_only_lv
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
11:01 a.m.
New subject: [PATCH 2/3] sparsify: fix btrfs subvolume processing in is_read_only_lv
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
10:04 a.m.
New subject: [PATCH 3/3] sysprep: fix btrfs subvolume processing in fs-uuids
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
10:59 a.m.
New subject: [PATCH 3/3] sysprep: fix btrfs subvolume processing in fs-uuids
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
5:58 a.m.
New subject: [PATCH 3/3] sysprep: fix btrfs subvolume processing in fs-uuids
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
7:17 a.m.
New subject: [PATCH 3/3] sysprep: fix btrfs subvolume processing in fs-uuids
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
3093
days inactive
3094
days old
10 comments
3 participants
participants (3)
-
Maxim Perevedentsev -
Pino Toscano -
Richard W.M. Jones