6:01 a.m.
Pre-modularised virt-v2v allows you to use either --compress or -oo
compressed which basically added the -c option to qemu-img convert,
compressing the qcow2 output. Since we switched over to nbdcopy this
option is no longer available. The alternative is to set up the
"compress" driver in qemu-nbd. However that didn't work because
nbdcopy used to ignore the target minimum block size, and the compress
driver requires you to obey the 64k minimum when writing, else it
fails. Now that nbdcopy 1.13.5 actually fixes this problem, we can
reimplement -oo compressed.
This is a minimal fix for https://bugzilla.redhat.com/show_bug.cgi?id=2047660
I only implemented this for small selection of output modes that go to
local files.
Old virt-v2v also implemented this option (at least, in theory) for -o
glance, -o rhv and -o vdsm, but all of these modes are deprecated to
some degree. -o openstack which replaces glance only allows raw
format uploads, and -o rhv-upload which replaces -o rhv/vdsm does its
own thing completely.
Rich.
6:01 a.m.
New subject: [PATCH v2v 1/3] qemu-nbd: Implement output compression for qcow2 files
---
lib/qemuNBD.ml | 11 +++++++++--
lib/qemuNBD.mli | 5 +++++
output/output.ml | 38 +++++++++++++++++++++++++++++++++++---
output/output.mli | 1 +
4 files changed, 50 insertions(+), 5 deletions(-)
diff --git a/lib/qemuNBD.ml b/lib/qemuNBD.ml
index ae21b17c7d..bbb65f4155 100644
--- a/lib/qemuNBD.ml
+++ b/lib/qemuNBD.ml
@@ -55,14 +55,16 @@ type cmd = {
disk : string;
mutable snapshot : bool;
mutable format : string option;
+ mutable imgopts : bool;
}
-let create disk = { disk; snapshot = false; format = None }
+let create disk = { disk; snapshot = false; format = None; imgopts = false }
let set_snapshot cmd snap = cmd.snapshot <- snap
let set_format cmd format = cmd.format <- format
+let set_image_opts cmd imgopts = cmd.imgopts <- imgopts
-let run_unix socket { disk; snapshot; format } =
+let run_unix socket { disk; snapshot; format; imgopts } =
assert (disk <> "");
(* Create a temporary directory where we place the PID file. *)
@@ -85,6 +87,11 @@ let run_unix socket { disk; snapshot; format } =
(* -s adds a protective overlay. *)
if snapshot then List.push_back args "-s";
+ (* --image-opts reinterprets the filename parameter as a set of
+ * image options.
+ *)
+ if imgopts then List.push_back args "--image-opts";
+
if have_selinux && qemu_nbd_has_selinux_label_option () then (
List.push_back args "--selinux-label";
List.push_back args "system_u:object_r:svirt_socket_t:s0"
diff --git a/lib/qemuNBD.mli b/lib/qemuNBD.mli
index e10d3106c7..afe9d944e1 100644
--- a/lib/qemuNBD.mli
+++ b/lib/qemuNBD.mli
@@ -43,6 +43,11 @@ val set_snapshot : cmd -> bool -> unit
val set_format : cmd -> string option -> unit
(** Set the format [--format] parameter. *)
+val set_image_opts : cmd -> bool -> unit
+(** Set whether the [--image-opts] parameter is used. This changes
+ the meaning of the [filename] parameter to a set of image options.
+ Consult the qemu-nbd man page for more details. *)
+
val run_unix : string -> cmd -> string * int
(** Start qemu-nbd command listening on a Unix domain socket,
waiting for the process to start up.
diff --git a/output/output.ml b/output/output.ml
index 5c6670b99c..5fe8555a7a 100644
--- a/output/output.ml
+++ b/output/output.ml
@@ -69,7 +69,7 @@ let error_if_disk_count_gt dir n =
if Sys.file_exists socket then
error (f_"this output module doesn't support copying more than %d
disks") n
-let output_to_local_file ?(changeuid = fun f -> f ())
+let output_to_local_file ?(changeuid = fun f -> f ()) ?(compressed = false)
output_alloc output_format filename size socket =
(* Check nbdkit is installed and has the required plugin. *)
if not (Nbdkit.is_installed ()) then
@@ -78,6 +78,10 @@ let output_to_local_file ?(changeuid = fun f -> f ())
error (f_"nbdkit-file-plugin is not installed or not working");
let nbdkit_config = Nbdkit.config () in
+ (* Only allow compressed with -of qcow2. *)
+ if compressed && output_format <> "qcow2" then
+ error (f_"‘-oo compressed’ is only allowed when the output format is a local
qcow2-format file, ie ‘-of qcow2’");
+
let g = open_guestfs () in
let preallocation =
match output_alloc with
@@ -103,9 +107,37 @@ let output_to_local_file ?(changeuid = fun f -> f ())
On_exit.kill pid
| "qcow2" ->
- let cmd = QemuNBD.create filename in
+ let cmd =
+ if not compressed then (
+ let cmd = QemuNBD.create filename in
+ QemuNBD.set_format cmd (Some "qcow2");
+ cmd
+ )
+ else (
+ (* Check nbdcopy is new enough. This assumes that
+ * the version of libnbd is the same as the version
+ * of nbdcopy, but parsing this is easier. We can
+ * remove this check when we build-depend on
+ * libnbd >= 1.14.
+ *)
+ let () =
+ let version = NBD.create () |> NBD.get_version in
+ let version = String.nsplit "." version in
+ let version = List.map int_of_string version in
+ if version < [1; 13; 5] then
+ error (f_"-oo compressed option requires nbdcopy >= 1.13.5")
in
+
+ let qemu_quote str = String.replace str "," ",," in
+ let image_opts = [ "driver=compress";
+ "file.driver=qcow2";
+ "file.file.driver=file";
+ "file.file.filename=" ^ qemu_quote filename ] in
+ let image_opts = String.concat "," image_opts in
+ let cmd = QemuNBD.create image_opts in
+ QemuNBD.set_image_opts cmd true;
+ cmd
+ ) in
QemuNBD.set_snapshot cmd false;
- QemuNBD.set_format cmd (Some "qcow2");
let _, pid = QemuNBD.run_unix socket cmd in
On_exit.kill pid
diff --git a/output/output.mli b/output/output.mli
index 8d3d686594..c1f0f53d8e 100644
--- a/output/output.mli
+++ b/output/output.mli
@@ -84,6 +84,7 @@ val error_if_disk_count_gt : string -> int -> unit
called. *)
val output_to_local_file : ?changeuid:((unit -> unit) -> unit) ->
+ ?compressed:bool ->
Types.output_allocation ->
string -> string -> int64 -> string ->
unit
--
2.37.0.rc2
1:51 a.m.
New subject: [PATCH v2v 1/3] qemu-nbd: Implement output compression for qcow2 files
On 07/01/22 13:01, Richard W.M. Jones wrote:
---
lib/qemuNBD.ml | 11 +++++++++--
lib/qemuNBD.mli | 5 +++++
output/output.ml | 38 +++++++++++++++++++++++++++++++++++---
output/output.mli | 1 +
4 files changed, 50 insertions(+), 5 deletions(-)
(Can you update your git diff order so that *.mli be put before *.ml?)
diff --git a/lib/qemuNBD.ml b/lib/qemuNBD.ml
index ae21b17c7d..bbb65f4155 100644
--- a/lib/qemuNBD.ml
+++ b/lib/qemuNBD.ml
@@ -55,14 +55,16 @@ type cmd = {
disk : string;
mutable snapshot : bool;
mutable format : string option;
+ mutable imgopts : bool;
}
-let create disk = { disk; snapshot = false; format = None }
+let create disk = { disk; snapshot = false; format = None; imgopts = false }
let set_snapshot cmd snap = cmd.snapshot <- snap
let set_format cmd format = cmd.format <- format
+let set_image_opts cmd imgopts = cmd.imgopts <- imgopts
-let run_unix socket { disk; snapshot; format } =
+let run_unix socket { disk; snapshot; format; imgopts } =
assert (disk <> "");
(* Create a temporary directory where we place the PID file. *)
@@ -85,6 +87,11 @@ let run_unix socket { disk; snapshot; format } =
(* -s adds a protective overlay. *)
if snapshot then List.push_back args "-s";
+ (* --image-opts reinterprets the filename parameter as a set of
+ * image options.
+ *)
+ if imgopts then List.push_back args "--image-opts";
+
if have_selinux && qemu_nbd_has_selinux_label_option () then (
List.push_back args "--selinux-label";
List.push_back args "system_u:object_r:svirt_socket_t:s0"
diff --git a/lib/qemuNBD.mli b/lib/qemuNBD.mli
index e10d3106c7..afe9d944e1 100644
--- a/lib/qemuNBD.mli
+++ b/lib/qemuNBD.mli
@@ -43,6 +43,11 @@ val set_snapshot : cmd -> bool -> unit
val set_format : cmd -> string option -> unit
(** Set the format [--format] parameter. *)
+val set_image_opts : cmd -> bool -> unit
+(** Set whether the [--image-opts] parameter is used. This changes
+ the meaning of the [filename] parameter to a set of image options.
+ Consult the qemu-nbd man page for more details. *)
+
val run_unix : string -> cmd -> string * int
(** Start qemu-nbd command listening on a Unix domain socket,
waiting for the process to start up.
diff --git a/output/output.ml b/output/output.ml
index 5c6670b99c..5fe8555a7a 100644
--- a/output/output.ml
+++ b/output/output.ml
@@ -69,7 +69,7 @@ let error_if_disk_count_gt dir n =
if Sys.file_exists socket then
error (f_"this output module doesn't support copying more than %d
disks") n
-let output_to_local_file ?(changeuid = fun f -> f ())
+let output_to_local_file ?(changeuid = fun f -> f ()) ?(compressed = false)
output_alloc output_format filename size socket =
(* Check nbdkit is installed and has the required plugin. *)
if not (Nbdkit.is_installed ()) then
@@ -78,6 +78,10 @@ let output_to_local_file ?(changeuid = fun f -> f ())
error (f_"nbdkit-file-plugin is not installed or not working");
let nbdkit_config = Nbdkit.config () in
+ (* Only allow compressed with -of qcow2. *)
+ if compressed && output_format <> "qcow2" then
+ error (f_"‘-oo compressed’ is only allowed when the output format is a local
qcow2-format file, ie ‘-of qcow2’");
+
Can you wrap this line?
(Although I notice there's another quite long line (another error
message) in this file, about nbdkit not being installed.)
I suggest "i.e.," rather than "ie".
let g = open_guestfs () in
let preallocation =
match output_alloc with
@@ -103,9 +107,37 @@ let output_to_local_file ?(changeuid = fun f -> f ())
On_exit.kill pid
| "qcow2" ->
- let cmd = QemuNBD.create filename in
+ let cmd =
+ if not compressed then (
IMO, whenever we have an "else" branch, it's better to simplify the
condition by dropping "not", and swapping around the branches.
+ let cmd = QemuNBD.create filename in
+ QemuNBD.set_format cmd (Some "qcow2");
+ cmd
+ )
+ else (
+ (* Check nbdcopy is new enough. This assumes that
+ * the version of libnbd is the same as the version
+ * of nbdcopy, but parsing this is easier. We can
+ * remove this check when we build-depend on
+ * libnbd >= 1.14.
+ *)
+ let () =
What is the purpose of "let ()" here specifically?
(I know what it's good for in general, from your earlier reference
<https://baturin.org/docs/ocaml-faq/>;, but I don't remember seeing it
much outside of the outermost scope.)
+ let version = NBD.create () |> NBD.get_version in
+ let version = String.nsplit "." version in
+ let version = List.map int_of_string version in
+ if version < [1; 13; 5] then
(Huh, didn't know such comparison existed, great!)
+ error (f_"-oo compressed option requires nbdcopy
>= 1.13.5") in
+
+ let qemu_quote str = String.replace str "," ",," in
+ let image_opts = [ "driver=compress";
+ "file.driver=qcow2";
+ "file.file.driver=file";
+ "file.file.filename=" ^ qemu_quote filename ] in
+ let image_opts = String.concat "," image_opts in
+ let cmd = QemuNBD.create image_opts in
+ QemuNBD.set_image_opts cmd true;
+ cmd
+ ) in
QemuNBD.set_snapshot cmd false;
- QemuNBD.set_format cmd (Some "qcow2");
Aha! This actually supports my suggestion to drop the "not" from the
"if"'s condition, and swap the branches around -- that way the patch
reads more easily. I first thought that the "set_format" call on the
not-compressed branch above materialized from thin air.
let _, pid = QemuNBD.run_unix socket cmd in
On_exit.kill pid
diff --git a/output/output.mli b/output/output.mli
index 8d3d686594..c1f0f53d8e 100644
--- a/output/output.mli
+++ b/output/output.mli
@@ -84,6 +84,7 @@ val error_if_disk_count_gt : string -> int -> unit
called. *)
val output_to_local_file : ?changeuid:((unit -> unit) -> unit) ->
+ ?compressed:bool ->
Types.output_allocation ->
string -> string -> int64 -> string ->
unit
Only made some style comments; feel free to pick whatever you like (even
none):
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks
Laszlo
2:52 a.m.
New subject: [PATCH v2v 1/3] qemu-nbd: Implement output compression for qcow2 files
On Tue, Jul 05, 2022 at 08:51:49AM +0200, Laszlo Ersek wrote:
On 07/01/22 13:01, Richard W.M. Jones wrote:
> ---
> lib/qemuNBD.ml | 11 +++++++++--
> lib/qemuNBD.mli | 5 +++++
> output/output.ml | 38 +++++++++++++++++++++++++++++++++++---
> output/output.mli | 1 +
> 4 files changed, 50 insertions(+), 5 deletions(-)
(Can you update your git diff order so that *.mli be put before *.ml?)
In nbdkit we have scripts/git.orderfile:
https://gitlab.com/nbdkit/nbdkit/-/blob/master/scripts/git.orderfile
I guess we should copy this into other projects.
...
> + let () =
What is the purpose of "let ()" here specifically?
(I know what it's good for in general, from your earlier reference
<https://baturin.org/docs/ocaml-faq/>;, but I don't remember seeing it
much outside of the outermost scope.)
> + let version = NBD.create () |> NBD.get_version in
> + let version = String.nsplit "." version in
> + let version = List.map int_of_string version in
> + if version < [1; 13; 5] then
It hides the binding of "version", so it doesn't escape from this
scope. It's not necessary, but avoids possible confusion if we used
an unrelated "version" symbol somewhere later in the code.
(Huh, didn't know such comparison existed, great!)
Sure, you can compare anything except recursive structures, functions
(because of the Halting Problem IIRC) and certain peculiar internal
types. It's nothing clever, it just recursively compares the two
structures until it finds something unequal:
https://github.com/ocaml/ocaml/blob/d9afa408c612e74a266b95f0fa25bb1efde72...
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks - I'll address the other comments you made in the updated 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
5:56 a.m.
New subject: [PATCH v2v 1/3] qemu-nbd: Implement output compression for qcow2 files
On 07/05/22 09:52, Richard W.M. Jones wrote:
On Tue, Jul 05, 2022 at 08:51:49AM +0200, Laszlo Ersek wrote:
> On 07/01/22 13:01, Richard W.M. Jones wrote:
>> ---
>> lib/qemuNBD.ml | 11 +++++++++--
>> lib/qemuNBD.mli | 5 +++++
>> output/output.ml | 38 +++++++++++++++++++++++++++++++++++---
>> output/output.mli | 1 +
>> 4 files changed, 50 insertions(+), 5 deletions(-)
>
> (Can you update your git diff order so that *.mli be put before *.ml?)
In nbdkit we have scripts/git.orderfile:
https://gitlab.com/nbdkit/nbdkit/-/blob/master/scripts/git.orderfile
I guess we should copy this into other projects.
...
>> + let () =
>
> What is the purpose of "let ()" here specifically?
>
> (I know what it's good for in general, from your earlier reference
> <https://baturin.org/docs/ocaml-faq/>;, but I don't remember seeing it
> much outside of the outermost scope.)
>
>> + let version = NBD.create () |> NBD.get_version in
>> + let version = String.nsplit "." version in
>> + let version = List.map int_of_string version in
>> + if version < [1; 13; 5] then
It hides the binding of "version", so it doesn't escape from this
scope. It's not necessary, but avoids possible confusion if we used
an unrelated "version" symbol somewhere later in the code.
Ah, thanks. I've learned something again :)
> (Huh, didn't know such comparison existed, great!)
Sure, you can compare anything except recursive structures, functions
(because of the Halting Problem IIRC) and certain peculiar internal
types. It's nothing clever, it just recursively compares the two
structures until it finds something unequal:
https://github.com/ocaml/ocaml/blob/d9afa408c612e74a266b95f0fa25bb1efde72...
Thanks for the reference. It's interesting to see that it manages the
"compare stack" manually and can even fail (gracefully) because of
"stack overflow"!
Thanks
Laszlo
> Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks - I'll address the other comments you made in the updated patch.
Rich.
6:01 a.m.
New subject: [PATCH v2v 2/3] -o disk, -o libvirt, -o qemu: Implement -of qcow2 -oo compressed
For various output modes, implement -oo compressed which can be used
to generate compressed qcow2 files. This option was dropped when
modularizing virt-v2v, and required changes to nbdcopy which are
finally upstream in libnbd >= 1.13.5.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2047660
Fixes: commit 255722cbf39afc0b012e2ac00d16fa6ba2f8c21f
Reported-by: Xiaodai Wang
---
TODO | 14 --------------
output/output_disk.ml | 29 +++++++++++++++++++++--------
output/output_libvirt.ml | 31 ++++++++++++++++++++++---------
output/output_qemu.ml | 25 ++++++++++++++++++-------
4 files changed, 61 insertions(+), 38 deletions(-)
diff --git a/TODO b/TODO
index f578d50621..04b1dd2036 100644
--- a/TODO
+++ b/TODO
@@ -1,17 +1,3 @@
-virt-v2v -o disk|qemu -oo compressed
-------------------------------------
-
-This was temporarily dropped when I modularized virt-v2v. It would
-not be too difficult to add it back. The following is the qemu-nbd
-command required (to be run as the output helper) which creates a
-compressed qcow2 disk image:
-
-$ qemu-nbd --image-opts
driver=compress,file.driver=qcow2,file.file.driver=file,file.file.filename=new.qcow2
-
-Note this requires fixes in nbdcopy so it obeys the advertised block
-alignment:
-https://lists.gnu.org/archive/html/qemu-block/2022-01/threads.html#00729
-
virt-v2v -o rhv-upload
----------------------
diff --git a/output/output_disk.ml b/output/output_disk.ml
index bc5b4e1ca6..abcfcdc0ce 100644
--- a/output/output_disk.ml
+++ b/output/output_disk.ml
@@ -30,7 +30,7 @@ open Create_libvirt_xml
open Output
module Disk = struct
- type poptions = Types.output_allocation * string * string * string
+ type poptions = bool * Types.output_allocation * string * string * string
type t = unit
@@ -41,11 +41,21 @@ module Disk = struct
| None -> ""
let query_output_options () =
- printf (f_"No output options can be used in this mode.\n")
+ printf (f_"Output options that can be used with -o disk:
+
+ -oo compressed Compress the output file (used only with -of qcow2)
+")
let parse_options options source =
- if options.output_options <> [] then
- error (f_"no -oo (output options) are allowed here");
+ let compressed = ref false in
+ List.iter (
+ function
+ | "compressed", "" -> compressed := true
+ | "compressed", v -> compressed := bool_of_string v
+ | k, _ ->
+ error (f_"-o disk: unknown output option ‘-oo %s’") k
+ ) options.output_options;
+
if options.output_password <> None then
error_option_cannot_be_used_in_output_mode "local" "-op";
@@ -60,11 +70,13 @@ module Disk = struct
let output_name = Option.default source.s_name options.output_name in
- options.output_alloc, options.output_format, output_name, output_storage
+ !compressed, options.output_alloc, options.output_format,
+ output_name, output_storage
let setup dir options source =
let disks = get_disks dir in
- let output_alloc, output_format, output_name, output_storage = options in
+ let compressed, output_alloc, output_format, output_name, output_storage =
+ options in
List.iter (
fun (i, size) ->
@@ -73,11 +85,12 @@ module Disk = struct
(* Create the actual output disk. *)
let outdisk = disk_path output_storage output_name i in
- output_to_local_file output_alloc output_format outdisk size socket
+ output_to_local_file ~compressed output_alloc output_format
+ outdisk size socket
) disks
let finalize dir options () source inspect target_meta =
- let output_alloc, output_format, output_name, output_storage = options in
+ let _, output_alloc, output_format, output_name, output_storage = options in
(* Convert metadata to libvirt XML. *)
(match target_meta.target_firmware with
diff --git a/output/output_libvirt.ml b/output/output_libvirt.ml
index e0d3432d25..04b4c5f81e 100644
--- a/output/output_libvirt.ml
+++ b/output/output_libvirt.ml
@@ -32,7 +32,7 @@ open Create_libvirt_xml
open Output
module Libvirt_ = struct
- type poptions = Libvirt.rw Libvirt.Connect.t Lazy.t *
+ type poptions = Libvirt.rw Libvirt.Connect.t Lazy.t * bool *
Types.output_allocation * string * string * string
type t = string * string
@@ -44,11 +44,21 @@ module Libvirt_ = struct
| None -> ""
let query_output_options () =
- printf (f_"No output options can be used in this mode.\n")
+ printf (f_"Output options that can be used with -o libvirt:
+
+ -oo compressed Compress the output file (used only with -of qcow2)
+")
let parse_options options source =
- if options.output_options <> [] then
- error (f_"no -oo (output options) are allowed here");
+ let compressed = ref false in
+ List.iter (
+ function
+ | "compressed", "" -> compressed := true
+ | "compressed", v -> compressed := bool_of_string v
+ | k, _ ->
+ error (f_"-o disk: unknown output option ‘-oo %s’") k
+ ) options.output_options;
+
if options.output_password <> None then
error_option_cannot_be_used_in_output_mode "libvirt" "-op";
@@ -59,12 +69,13 @@ module Libvirt_ = struct
let output_name = Option.default source.s_name options.output_name in
- (conn, options.output_alloc, options.output_format, output_name,
- output_pool)
+ (conn, !compressed, options.output_alloc, options.output_format,
+ output_name, output_pool)
let setup dir options source =
let disks = get_disks dir in
- let conn, output_alloc, output_format, output_name, output_pool = options in
+ let conn, compressed, output_alloc, output_format,
+ output_name, output_pool = options in
let conn = Lazy.force conn in
(* Get the capabilities from libvirt. *)
@@ -119,13 +130,15 @@ module Libvirt_ = struct
(* Create the actual output disk. *)
let outdisk = target_path // output_name ^ "-sd" ^ (drive_name i) in
- output_to_local_file output_alloc output_format outdisk size socket
+ output_to_local_file ~compressed output_alloc output_format
+ outdisk size socket
) disks;
(capabilities_xml, pool_name)
let rec finalize dir options t source inspect target_meta =
- let conn, output_alloc, output_format, output_name, output_pool = options in
+ let conn, _, output_alloc, output_format, output_name, output_pool =
+ options in
let capabilities_xml, pool_name = t in
(match target_meta.target_firmware with
diff --git a/output/output_qemu.ml b/output/output_qemu.ml
index 3269fba501..5081b79f99 100644
--- a/output/output_qemu.ml
+++ b/output/output_qemu.ml
@@ -29,7 +29,8 @@ open Utils
open Output
module QEMU = struct
- type poptions = bool * Types.output_allocation * string * string * string
+ type poptions = bool * bool *
+ Types.output_allocation * string * string * string
type t = unit
@@ -42,6 +43,7 @@ module QEMU = struct
let query_output_options () =
printf (f_"Output options (-oo) which can be used with -o qemu:
+ -oo compressed Compress the output file (used only with -of qcow2)
-oo qemu-boot Boot the guest in qemu after conversion
")
@@ -49,10 +51,16 @@ module QEMU = struct
if options.output_password <> None then
error_option_cannot_be_used_in_output_mode "qemu" "-op";
- let qemu_boot = ref false in
+ let compressed = ref false
+ and qemu_boot = ref false in
List.iter (
fun (k, v) ->
match k with
+ | "compressed" ->
+ if v = "" || v = "true" then compressed := true
+ else if v = "false" then compressed := false
+ else
+ error (f_"-o qemu: use -oo compressed[=true|false]")
| "qemu-boot" ->
if v = "" || v = "true" then qemu_boot := true
else if v = "false" then qemu_boot := false
@@ -61,7 +69,8 @@ module QEMU = struct
| k ->
error (f_"-o qemu: unknown output option ‘-oo %s’") k
) options.output_options;
- let qemu_boot = !qemu_boot in
+ let compressed = !compressed
+ and qemu_boot = !qemu_boot in
(* -os must be set to a directory. *)
let output_storage =
@@ -74,12 +83,13 @@ module QEMU = struct
let output_name = Option.default source.s_name options.output_name in
- (qemu_boot, options.output_alloc, options.output_format,
+ (compressed, qemu_boot, options.output_alloc, options.output_format,
output_name, output_storage)
let setup dir options source =
let disks = get_disks dir in
- let _, output_alloc, output_format, output_name, output_storage = options in
+ let compressed, _, output_alloc, output_format,
+ output_name, output_storage = options in
List.iter (
fun (i, size) ->
@@ -88,11 +98,12 @@ module QEMU = struct
(* Create the actual output disk. *)
let outdisk = disk_path output_storage output_name i in
- output_to_local_file output_alloc output_format outdisk size socket
+ output_to_local_file ~compressed output_alloc output_format
+ outdisk size socket
) disks
let finalize dir options () source inspect target_meta =
- let qemu_boot, output_alloc, output_format,
+ let _, qemu_boot, output_alloc, output_format,
output_name, output_storage = options in
let { guestcaps; target_buses; target_firmware } = target_meta in
--
2.37.0.rc2
1:58 a.m.
New subject: [PATCH v2v 2/3] -o disk, -o libvirt, -o qemu: Implement -of qcow2 -oo compressed
On 07/01/22 13:01, Richard W.M. Jones wrote:
For various output modes, implement -oo compressed which can be used
to generate compressed qcow2 files. This option was dropped when
modularizing virt-v2v, and required changes to nbdcopy which are
finally upstream in libnbd >= 1.13.5.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2047660
Fixes: commit 255722cbf39afc0b012e2ac00d16fa6ba2f8c21f
Reported-by: Xiaodai Wang
---
TODO | 14 --------------
output/output_disk.ml | 29 +++++++++++++++++++++--------
output/output_libvirt.ml | 31 ++++++++++++++++++++++---------
output/output_qemu.ml | 25 ++++++++++++++++++-------
4 files changed, 61 insertions(+), 38 deletions(-)
diff --git a/TODO b/TODO
index f578d50621..04b1dd2036 100644
--- a/TODO
+++ b/TODO
@@ -1,17 +1,3 @@
-virt-v2v -o disk|qemu -oo compressed
-------------------------------------
-
-This was temporarily dropped when I modularized virt-v2v. It would
-not be too difficult to add it back. The following is the qemu-nbd
-command required (to be run as the output helper) which creates a
-compressed qcow2 disk image:
-
-$ qemu-nbd --image-opts
driver=compress,file.driver=qcow2,file.file.driver=file,file.file.filename=new.qcow2
-
-Note this requires fixes in nbdcopy so it obeys the advertised block
-alignment:
-https://lists.gnu.org/archive/html/qemu-block/2022-01/threads.html#00729
-
virt-v2v -o rhv-upload
----------------------
diff --git a/output/output_disk.ml b/output/output_disk.ml
index bc5b4e1ca6..abcfcdc0ce 100644
--- a/output/output_disk.ml
+++ b/output/output_disk.ml
@@ -30,7 +30,7 @@ open Create_libvirt_xml
open Output
module Disk = struct
- type poptions = Types.output_allocation * string * string * string
+ type poptions = bool * Types.output_allocation * string * string * string
type t = unit
@@ -41,11 +41,21 @@ module Disk = struct
| None -> ""
let query_output_options () =
- printf (f_"No output options can be used in this mode.\n")
+ printf (f_"Output options that can be used with -o disk:
+
+ -oo compressed Compress the output file (used only with -of qcow2)
+")
let parse_options options source =
- if options.output_options <> [] then
- error (f_"no -oo (output options) are allowed here");
+ let compressed = ref false in
+ List.iter (
+ function
+ | "compressed", "" -> compressed := true
+ | "compressed", v -> compressed := bool_of_string v
+ | k, _ ->
+ error (f_"-o disk: unknown output option ‘-oo %s’") k
+ ) options.output_options;
+
if options.output_password <> None then
error_option_cannot_be_used_in_output_mode "local" "-op";
@@ -60,11 +70,13 @@ module Disk = struct
let output_name = Option.default source.s_name options.output_name in
- options.output_alloc, options.output_format, output_name, output_storage
+ !compressed, options.output_alloc, options.output_format,
+ output_name, output_storage
let setup dir options source =
let disks = get_disks dir in
- let output_alloc, output_format, output_name, output_storage = options in
+ let compressed, output_alloc, output_format, output_name, output_storage =
+ options in
List.iter (
fun (i, size) ->
@@ -73,11 +85,12 @@ module Disk = struct
(* Create the actual output disk. *)
let outdisk = disk_path output_storage output_name i in
- output_to_local_file output_alloc output_format outdisk size socket
+ output_to_local_file ~compressed output_alloc output_format
+ outdisk size socket
) disks
let finalize dir options () source inspect target_meta =
- let output_alloc, output_format, output_name, output_storage = options in
+ let _, output_alloc, output_format, output_name, output_storage = options in
(* Convert metadata to libvirt XML. *)
(match target_meta.target_firmware with
diff --git a/output/output_libvirt.ml b/output/output_libvirt.ml
index e0d3432d25..04b4c5f81e 100644
--- a/output/output_libvirt.ml
+++ b/output/output_libvirt.ml
@@ -32,7 +32,7 @@ open Create_libvirt_xml
open Output
module Libvirt_ = struct
- type poptions = Libvirt.rw Libvirt.Connect.t Lazy.t *
+ type poptions = Libvirt.rw Libvirt.Connect.t Lazy.t * bool *
Types.output_allocation * string * string * string
type t = string * string
@@ -44,11 +44,21 @@ module Libvirt_ = struct
| None -> ""
let query_output_options () =
- printf (f_"No output options can be used in this mode.\n")
+ printf (f_"Output options that can be used with -o libvirt:
+
+ -oo compressed Compress the output file (used only with -of qcow2)
+")
let parse_options options source =
- if options.output_options <> [] then
- error (f_"no -oo (output options) are allowed here");
+ let compressed = ref false in
+ List.iter (
+ function
+ | "compressed", "" -> compressed := true
+ | "compressed", v -> compressed := bool_of_string v
+ | k, _ ->
+ error (f_"-o disk: unknown output option ‘-oo %s’") k
+ ) options.output_options;
+
if options.output_password <> None then
error_option_cannot_be_used_in_output_mode "libvirt" "-op";
@@ -59,12 +69,13 @@ module Libvirt_ = struct
let output_name = Option.default source.s_name options.output_name in
- (conn, options.output_alloc, options.output_format, output_name,
- output_pool)
+ (conn, !compressed, options.output_alloc, options.output_format,
+ output_name, output_pool)
let setup dir options source =
let disks = get_disks dir in
- let conn, output_alloc, output_format, output_name, output_pool = options in
+ let conn, compressed, output_alloc, output_format,
+ output_name, output_pool = options in
let conn = Lazy.force conn in
(* Get the capabilities from libvirt. *)
@@ -119,13 +130,15 @@ module Libvirt_ = struct
(* Create the actual output disk. *)
let outdisk = target_path // output_name ^ "-sd" ^ (drive_name i) in
- output_to_local_file output_alloc output_format outdisk size socket
+ output_to_local_file ~compressed output_alloc output_format
+ outdisk size socket
) disks;
(capabilities_xml, pool_name)
let rec finalize dir options t source inspect target_meta =
- let conn, output_alloc, output_format, output_name, output_pool = options in
+ let conn, _, output_alloc, output_format, output_name, output_pool =
+ options in
let capabilities_xml, pool_name = t in
(match target_meta.target_firmware with
diff --git a/output/output_qemu.ml b/output/output_qemu.ml
index 3269fba501..5081b79f99 100644
--- a/output/output_qemu.ml
+++ b/output/output_qemu.ml
@@ -29,7 +29,8 @@ open Utils
open Output
module QEMU = struct
- type poptions = bool * Types.output_allocation * string * string * string
+ type poptions = bool * bool *
+ Types.output_allocation * string * string * string
type t = unit
@@ -42,6 +43,7 @@ module QEMU = struct
let query_output_options () =
printf (f_"Output options (-oo) which can be used with -o qemu:
+ -oo compressed Compress the output file (used only with -of qcow2)
-oo qemu-boot Boot the guest in qemu after conversion
")
@@ -49,10 +51,16 @@ module QEMU = struct
if options.output_password <> None then
error_option_cannot_be_used_in_output_mode "qemu" "-op";
- let qemu_boot = ref false in
+ let compressed = ref false
+ and qemu_boot = ref false in
List.iter (
fun (k, v) ->
match k with
+ | "compressed" ->
+ if v = "" || v = "true" then compressed := true
+ else if v = "false" then compressed := false
+ else
+ error (f_"-o qemu: use -oo compressed[=true|false]")
| "qemu-boot" ->
if v = "" || v = "true" then qemu_boot := true
else if v = "false" then qemu_boot := false
The pre-patch code could perhaps do with a bit of refactoring (I really
liked the bool_of_string approach with the single-level match above),
but that's not a blocker for this patch.
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks
Laszlo
@@ -61,7 +69,8 @@ module QEMU = struct
| k ->
error (f_"-o qemu: unknown output option ‘-oo %s’") k
) options.output_options;
- let qemu_boot = !qemu_boot in
+ let compressed = !compressed
+ and qemu_boot = !qemu_boot in
(* -os must be set to a directory. *)
let output_storage =
@@ -74,12 +83,13 @@ module QEMU = struct
let output_name = Option.default source.s_name options.output_name in
- (qemu_boot, options.output_alloc, options.output_format,
+ (compressed, qemu_boot, options.output_alloc, options.output_format,
output_name, output_storage)
let setup dir options source =
let disks = get_disks dir in
- let _, output_alloc, output_format, output_name, output_storage = options in
+ let compressed, _, output_alloc, output_format,
+ output_name, output_storage = options in
List.iter (
fun (i, size) ->
@@ -88,11 +98,12 @@ module QEMU = struct
(* Create the actual output disk. *)
let outdisk = disk_path output_storage output_name i in
- output_to_local_file output_alloc output_format outdisk size socket
+ output_to_local_file ~compressed output_alloc output_format
+ outdisk size socket
) disks
let finalize dir options () source inspect target_meta =
- let qemu_boot, output_alloc, output_format,
+ let _, qemu_boot, output_alloc, output_format,
output_name, output_storage = options in
let { guestcaps; target_buses; target_firmware } = target_meta in
6:01 a.m.
New subject: [PATCH v2v 3/3] tests: Add a simple test of -o local -of qcow2 -oo compressed
This only tests that it doesn't completely fail, which it did before
we fixed nbdcopy. I checked the file sizes manually and with
compression the resulting file is about half the size.
---
tests/Makefile.am | 2 +
tests/test-v2v-o-local-qcow2-compressed.sh | 51 ++++++++++++++++++++++
tests/test-v2v-of-option.sh | 2 +
3 files changed, 55 insertions(+)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ebc433ae5e..fb068624c7 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -83,6 +83,7 @@ TESTS = \
test-v2v-networks-and-bridges.sh \
test-v2v-o-glance.sh \
test-v2v-o-libvirt.sh \
+ test-v2v-o-local-qcow2-compressed.sh \
test-v2v-o-null.sh \
test-v2v-o-openstack.sh \
test-v2v-o-qemu.sh \
@@ -242,6 +243,7 @@ EXTRA_DIST += \
test-v2v-networks-and-bridges-expected.xml \
test-v2v-o-glance.sh \
test-v2v-o-libvirt.sh \
+ test-v2v-o-local-qcow2-compressed.sh \
test-v2v-o-null.sh \
test-v2v-o-openstack.sh \
test-v2v-o-qemu.sh \
diff --git a/tests/test-v2v-o-local-qcow2-compressed.sh
b/tests/test-v2v-o-local-qcow2-compressed.sh
new file mode 100755
index 0000000000..6ebf92c976
--- /dev/null
+++ b/tests/test-v2v-o-local-qcow2-compressed.sh
@@ -0,0 +1,51 @@
+#!/bin/bash -
+# libguestfs virt-v2v test script
+# Copyright (C) 2014-2022 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test -o local -of qcow2 -oo compressed.
+
+set -e
+
+source ./functions.sh
+set -e
+set -x
+
+skip_if_skipped
+requires test -f ../test-data/phony-guests/windows.img
+
+# This requires fixed nbdcopy >= 1.13.5.
+requires nbdcopy --version
+version="$( nbdcopy --version | head -1 | awk '{print $2}' )"
+minor="$( echo "$version" | awk -F. '{print $2}' )"
+release="$( echo "$version" | awk -F. '{print $3}' )"
+requires test $minor -gt 13 -o \( $minor -eq 13 -a $release -ge 5 \)
+
+export VIRT_TOOLS_DATA_DIR="$srcdir/../test-data/fake-virt-tools"
+
+d=test-v2v-o-local-qcow2-compressed.d
+rm -rf $d
+cleanup_fn rm -rf $d
+mkdir $d
+
+$VG virt-v2v --debug-gc \
+ -i disk ../test-data/phony-guests/windows.img \
+ -o local -of qcow2 -oo compressed -os $d
+
+# Test the libvirt XML metadata and a disk was created.
+ls -l $d
+test -f $d/windows.xml
+test -f $d/windows-sda
diff --git a/tests/test-v2v-of-option.sh b/tests/test-v2v-of-option.sh
index bdfd34180d..6c5f5938c8 100755
--- a/tests/test-v2v-of-option.sh
+++ b/tests/test-v2v-of-option.sh
@@ -42,6 +42,8 @@ $VG virt-v2v --debug-gc \
-i libvirt -ic "$libvirt_uri" windows \
-o local -os $d -of qcow2
+ls -l $d
+
# Test the disk is qcow2 format.
if [ "$(guestfish disk-format $d/windows-sda)" != qcow2 ]; then
echo "$0: test failed: output is not qcow2"
--
2.37.0.rc2
2:29 a.m.
New subject: [PATCH v2v 3/3] tests: Add a simple test of -o local -of qcow2 -oo compressed
On 07/01/22 13:01, Richard W.M. Jones wrote:
This only tests that it doesn't completely fail, which it did
before
we fixed nbdcopy. I checked the file sizes manually and with
compression the resulting file is about half the size.
---
tests/Makefile.am | 2 +
tests/test-v2v-o-local-qcow2-compressed.sh | 51 ++++++++++++++++++++++
tests/test-v2v-of-option.sh | 2 +
3 files changed, 55 insertions(+)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ebc433ae5e..fb068624c7 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -83,6 +83,7 @@ TESTS = \
test-v2v-networks-and-bridges.sh \
test-v2v-o-glance.sh \
test-v2v-o-libvirt.sh \
+ test-v2v-o-local-qcow2-compressed.sh \
test-v2v-o-null.sh \
test-v2v-o-openstack.sh \
test-v2v-o-qemu.sh \
@@ -242,6 +243,7 @@ EXTRA_DIST += \
test-v2v-networks-and-bridges-expected.xml \
test-v2v-o-glance.sh \
test-v2v-o-libvirt.sh \
+ test-v2v-o-local-qcow2-compressed.sh \
test-v2v-o-null.sh \
test-v2v-o-openstack.sh \
test-v2v-o-qemu.sh \
diff --git a/tests/test-v2v-o-local-qcow2-compressed.sh
b/tests/test-v2v-o-local-qcow2-compressed.sh
new file mode 100755
index 0000000000..6ebf92c976
--- /dev/null
+++ b/tests/test-v2v-o-local-qcow2-compressed.sh
@@ -0,0 +1,51 @@
+#!/bin/bash -
+# libguestfs virt-v2v test script
+# Copyright (C) 2014-2022 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test -o local -of qcow2 -oo compressed.
+
+set -e
+
+source ./functions.sh
+set -e
+set -x
+
+skip_if_skipped
+requires test -f ../test-data/phony-guests/windows.img
+
+# This requires fixed nbdcopy >= 1.13.5.
+requires nbdcopy --version
+version="$( nbdcopy --version | head -1 | awk '{print $2}' )"
+minor="$( echo "$version" | awk -F. '{print $2}' )"
+release="$( echo "$version" | awk -F. '{print $3}' )"
+requires test $minor -gt 13 -o \( $minor -eq 13 -a $release -ge 5 \)
Perhaps simpler:
{
printf '1.13.5\n'
nbdcopy --version | sed -n 's/^nbdcopy //p'
} \
| requires sort --sort=version --check=quiet
Another alternative:
nbdcopy --version \
| {
IFS=' .' read name major minor release
requires test \( "$major" -gt 1 \) -o \
\( "$major" -eq 1 -a "$minor" -gt 13 \) -o
\( "$major" -eq 1 -a "$minor" -eq 13 -a
"$release" -ge 5 \)
}
+
+export VIRT_TOOLS_DATA_DIR="$srcdir/../test-data/fake-virt-tools"
+
+d=test-v2v-o-local-qcow2-compressed.d
+rm -rf $d
+cleanup_fn rm -rf $d
+mkdir $d
+
+$VG virt-v2v --debug-gc \
+ -i disk ../test-data/phony-guests/windows.img \
+ -o local -of qcow2 -oo compressed -os $d
+
+# Test the libvirt XML metadata and a disk was created.
+ls -l $d
+test -f $d/windows.xml
+test -f $d/windows-sda
diff --git a/tests/test-v2v-of-option.sh b/tests/test-v2v-of-option.sh
index bdfd34180d..6c5f5938c8 100755
--- a/tests/test-v2v-of-option.sh
+++ b/tests/test-v2v-of-option.sh
@@ -42,6 +42,8 @@ $VG virt-v2v --debug-gc \
-i libvirt -ic "$libvirt_uri" windows \
-o local -os $d -of qcow2
+ls -l $d
+
# Test the disk is qcow2 format.
if [ "$(guestfish disk-format $d/windows-sda)" != qcow2 ]; then
echo "$0: test failed: output is not qcow2"
The last hunk really confused me. Please add a note to the commit message that the new
test is a modified clone of "test-v2v-of-option.sh", and that because we add
"ls -l $d" to the new test, we retrofit the old one for consistency.
(In theory, it should be a separate, precursor patch... but at least a commit msg note
should guide the reader.)
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks,
Laszlo
5:09 a.m.
New subject: [PATCH v2v 3/3] tests: Add a simple test of -o local -of qcow2 -oo compressed
The series is upstream in:
https://github.com/libguestfs/virt-v2v/commit/71c4301909cb307def02ebcd0e8...
https://github.com/libguestfs/virt-v2v/commit/53690a0c602a4286fdb9408fdf6...
https://github.com/libguestfs/virt-v2v/commit/7505750972b49e1a448c519a279...
Also I added a git.orderfile in this commit:
https://github.com/libguestfs/virt-v2v/commit/c8ae6ec7a1bbdb72c95e410b37e...
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
872
days inactive
876
days old
9 comments
2 participants
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones