Support qcow2 format in VDI export/import #6396

gthvn1 · 2025-04-01T18:23:01Z

The PR implements the support of QCow2 for the import/export of the VDI as described in this proposal.

It is a work in progress and several steps are required:
- [x] Import the Opam package into XAPI
- [x] Add the package in XAPI RPMs
- [x] Adding path into the XAPI to manage the qcow2 type when using vdi import/export
- [ ] Adding streaming of data in qcow tool

gthvn1 · 2025-04-01T18:25:02Z

It is a work in progress that I proposed in early steps in the development to be sure I'm doing things right. If you prefer I can close it and only create it when it is ready for review no worries.

We just clone the repo without modifications from https://github.com/mirage/ocaml-qcow.git This breaks the build... This is the first step to enable VDI import/export for qcow2 file. This commit is just the import from Mirage. Next step is to build the package. Signed-off-by: Guillaume <[email protected]>

Use io-page instead of io-page-unix because io-page is 3.0.0 and io-page-unix is 2.3.0 only Signed-off-by: Guillaume <[email protected]>

Since 6.0.1 Cstruct.len function was deprecated and we must use Cstruct.length. This patch fixes this issue Signed-off-by: Guillaume <[email protected]>

In MirageOS, the Unimplemented variant was removed from the Mirage_block.error type in version 2.0.0. This patch removes all places where this varient is used. In some places we replace it with a Failure. Signed-off-by: Guillaume <[email protected]>

In recent versions of cmdliner, Term.pure and Term.info have been replaced with newer, more explicit constructors. This patch uses Term.const as a replacement. Signed-off-by: Guillaume <[email protected]>

remove dune-project from `ocaml/qcow-tool` that was here from the first import. Now we are building qcow-tool with xapi. This patch creates the package in the toplevel dune-project and removes uneeded build stuff from the original repository. To be able to build qcow-tool we added the following packages to xs-opam: - asetmap - ezjsonm - mirage-block-combinators - mirage-types (deprecated) - mirage-types-lwt (deprecated) - prometheus - unix-type-representations Signed-off-by: Guillaume <[email protected]>

Signed-off-by: Guillaume <[email protected]>

This patch allows to pass "qcow2" as a supported format when calling VDI import/export. Currently we should reach unimplemented qcow tool wrapper if we are using "qcow2"" format. Signed-off-by: Guillaume <[email protected]>

Signed-off-by: Guillaume <[email protected]>

last-genius

General comments, given that this is a preview to better understand the practical implementation of the design.

last-genius · 2025-04-02T09:19:52Z

ocaml/xapi/export_raw_vdi.ml

+              if format = Qcow then
+                Qcow_tool_wrapper.send
+                  (Qcow_tool_wrapper.update_task_progress __context)
+                  s path size
+              else
+                Vhd_tool_wrapper.send ?relative_to:base_path
+                  (Vhd_tool_wrapper.update_task_progress __context)
+                  "none"
+                  (Importexport.Format.to_string format)


It's better to use a match statement here (like the one below). In case more variants are added in the future, if would not raise an error/warning, but an explicit match would

Yes if a new format is added it will fall through the else. I see the point of catching new format. I will do that way.

last-genius · 2025-04-02T09:27:47Z

ocaml/xapi/qcow_tool_wrapper.ml

+(*
+ * Copyright (C) 2025 Vates.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * 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 Lesser General Public License for more details.
+ *)
+


I think it'd be easier to review (and revert in case of errors) if you introduced the (unused) qcow_tool_wrapper in a separate commit, and only then started using it import_raw_vdi etc.

Separate commit not separate PR right?

last-genius · 2025-04-02T09:29:21Z

ocaml/xapi/import_raw_vdi.ml

                    let prezeroed =
                      not
                        (Sm_fs_ops.must_write_zeroes_into_new_vdi ~__context vdi)
                    in
+                    debug "GTNDEBUG: we are receiving Raw, Vhd or Qcow file" ;
                    Sm_fs_ops.with_block_attached_device __context rpc
                      session_id vdi `RW (fun path ->
                        if chunked then


Should there be calls to (some, currently undefined) Qcow_tool_wrapper.receive here? With Qcow as a separate case in the match statement?

Yes exactly. In fact I did that so I was able to compile but a better way is to report an error that it is not implemented or an empty Qcow_tool_wrapper.receive. But yes the calls will be to Qcow_tool_wrapper.receive.

last-genius · 2025-04-02T09:35:13Z

ocaml/xapi/vhd_tool_wrapper.ml

-(** [find_backend_device path] returns [Some path'] where [path'] is the backend path in
-    the driver domain corresponding to the frontend device [path] in this domain. *)
-let find_backend_device path =
-  try
-    let open Ezxenstore_core.Xenstore in
-    (* If we're looking at a xen frontend device, see if the backend
-       is in the same domain. If so check if it looks like a .vhd *)
-    let rdev = (Unix.stat path).Unix.st_rdev in
-    let major = rdev / 256 and minor = rdev mod 256 in
-    let link =
-      Unix.readlink (Printf.sprintf "/sys/dev/block/%d:%d/device" major minor)
-    in
-    match List.rev (String.split '/' link) with
-    | id :: "xen" :: "devices" :: _
-      when Astring.String.is_prefix ~affix:"vbd-" id ->
-        let id = int_of_string (String.sub id 4 (String.length id - 4)) in
-        with_xs (fun xs ->
-            let self = xs.Xs.read "domid" in
-            let backend =
-              xs.Xs.read (Printf.sprintf "device/vbd/%d/backend" id)
-            in
-            let params = xs.Xs.read (Printf.sprintf "%s/params" backend) in
-            match String.split '/' backend with
-            | "local" :: "domain" :: bedomid :: _ ->
-                if not (self = bedomid) then
-                  Helpers.internal_error
-                    "find_backend_device: Got domid %s but expected %s" bedomid
-                    self ;
-                Some params
-            | _ ->
-                raise Not_found
-        )
-    | _ ->
-        raise Not_found
-  with _ -> None
-


There is an identical function also in ocaml/vhd-tool/cli/sparse_dd.ml. It might be worth putting this into some library accessible to all users (Xapi_stdext? I'm not sure what the best place for it would be) and replacing all the usages with the single one.

Probably in xcp-idl somewhere. I don't think there's a good place for this currently. It might be worth creating a library package on top of ezxenstore that encapsulates common patterns when interacting with xenstore. Maybe a sublibrary of ezxenstore.

Yes I agree, I saw some common patterns.

psafont

What are the plans for the QCOW library code?

We could keep it maintained in the mirage org (I already maintain some mirage repos).

psafont · 2025-04-02T09:18:54Z

ocaml/qcow-tool/lib/qcow_recycler.ml

               | Error `Disconnected -> Lwt.return (Error `Disconnected)
               | Error `Is_read_only -> Lwt.return (Error `Is_read_only)
-               | Error e -> Format.kasprintf Lwt.fail_with "Unknown error: %a" B.pp_write_error e
+               | Error _ -> Format.kasprintf Lwt.fail_with "Unknown error in qcow_recylcer.ml"


I'd rather keep printing the error here, otherwise you might have a bad time debugging the cause

psafont · 2025-04-02T09:22:06Z

Makefile

@@ -173,7 +173,7 @@ DUNE_IU_PACKAGES3=-j $(JOBS) --destdir=$(DESTDIR) --prefix=$(OPTDIR) --libdir=$(
 install-dune3:
 	dune install $(DUNE_IU_PACKAGES3)

-DUNE_IU_PACKAGES4=-j $(JOBS) --destdir=$(DESTDIR) --prefix=$(PREFIX) --libdir=$(LIBDIR) --libexecdir=/usr/libexec --mandir=$(MANDIR) vhd-tool forkexec
+DUNE_IU_PACKAGES4=-j $(JOBS) --destdir=$(DESTDIR) --prefix=$(PREFIX) --libdir=$(LIBDIR) --libexecdir=/usr/libexec --mandir=$(MANDIR) vhd-tool qcow-tool forkexec


Is there any particular reason you want to package it here? If it's going to be used as a library there's no need for this. For using the binaries in hosts, they can be generated as part of xapi-tools (so a change in the dune file)

No particular reason. I just follow what has been done (anyway as I think I understood how it was done with vhd-tool). And also as it looks like it was not maintained anymore so I did that. But yes I can also update the mirage package to compile and use it.

psafont · 2025-04-02T09:52:02Z

ocaml/qcow-tool/lib_test/dune

@@ -1,6 +0,0 @@
-(executables


Why are the tests being removed?

I think that I have an issue when building with some missing opam package (not 100% sure) so I just removed them to compile. But clearly my idea was to reintroduce them later in the process ;)

psafont · 2025-04-02T09:59:09Z

ocaml/xapi/common_tool_wrapper.ml

+ * GNU Lesser General Public License for more details.
+ *)
+
+open Xapi_stdext_std.Xstringext


Avoid this library if possible, instead use Astring.String, or the standard library (String.split_on_char)

Also avoid open, they can be quite evil. Instead use
module Example = This.Is.Some.Example
Christian maintains a useful style guide here: https://github.com/lindig/ocaml-style?tab=readme-ov-file#avoid-opening-modules-globally

let opens are much better because their scope is contained and are obvious if the function is not too long.

I 100% agree with let open. I copied the open from another package I guess but I will apply the your suggestion for sure.

psafont · 2025-04-02T10:03:21Z

ocaml/xapi/vhd_tool_wrapper.ml

-(** [find_backend_device path] returns [Some path'] where [path'] is the backend path in
-    the driver domain corresponding to the frontend device [path] in this domain. *)
-let find_backend_device path =
-  try
-    let open Ezxenstore_core.Xenstore in
-    (* If we're looking at a xen frontend device, see if the backend
-       is in the same domain. If so check if it looks like a .vhd *)
-    let rdev = (Unix.stat path).Unix.st_rdev in
-    let major = rdev / 256 and minor = rdev mod 256 in
-    let link =
-      Unix.readlink (Printf.sprintf "/sys/dev/block/%d:%d/device" major minor)
-    in
-    match List.rev (String.split '/' link) with
-    | id :: "xen" :: "devices" :: _
-      when Astring.String.is_prefix ~affix:"vbd-" id ->
-        let id = int_of_string (String.sub id 4 (String.length id - 4)) in
-        with_xs (fun xs ->
-            let self = xs.Xs.read "domid" in
-            let backend =
-              xs.Xs.read (Printf.sprintf "device/vbd/%d/backend" id)
-            in
-            let params = xs.Xs.read (Printf.sprintf "%s/params" backend) in
-            match String.split '/' backend with
-            | "local" :: "domain" :: bedomid :: _ ->
-                if not (self = bedomid) then
-                  Helpers.internal_error
-                    "find_backend_device: Got domid %s but expected %s" bedomid
-                    self ;
-                Some params
-            | _ ->
-                raise Not_found
-        )
-    | _ ->
-        raise Not_found
-  with _ -> None
-


Probably in xcp-idl somewhere. I don't think there's a good place for this currently. It might be worth creating a library package on top of ezxenstore that encapsulates common patterns when interacting with xenstore. Maybe a sublibrary of ezxenstore.

gthvn1 · 2025-04-04T09:42:12Z

What are the plans for the QCOW library code?

We could keep it maintained in the mirage org (I already maintain some mirage repos).

My idea was to do as was done with vhd-tool and maintained the package within XAPI. But yes sure we can also keep it maintained in the mirage org. Both are ok for me.

lindig · 2025-04-08T13:14:22Z

I like the idea of importing the code at least for now such that it is easy to work on. We most likely want at least to make It fit into our build environment.

gthvn1 marked this pull request as draft April 1, 2025 18:23

gthvn1 added 10 commits April 2, 2025 09:42

[qcow-tool packaging] build fix: use io-page instead of io-page-unix

c2621a6

Use io-page instead of io-page-unix because io-page is 3.0.0 and io-page-unix is 2.3.0 only Signed-off-by: Guillaume <[email protected]>

[qcow-tool packaging] build fix: cstruct

9bd40d4

Since 6.0.1 Cstruct.len function was deprecated and we must use Cstruct.length. This patch fixes this issue Signed-off-by: Guillaume <[email protected]>

[qcow-tool packaging] build fix: update cmdliner

81232c5

In recent versions of cmdliner, Term.pure and Term.info have been replaced with newer, more explicit constructors. This patch uses Term.const as a replacement. Signed-off-by: Guillaume <[email protected]>

[qcow-tool packaging] remove lib-test

2f6c847

Signed-off-by: Guillaume <[email protected]>

[qcow-tool] run make format

ecc0ee1

[qcow-tool] add qcow as supported format

15f0f6e

This patch allows to pass "qcow2" as a supported format when calling VDI import/export. Currently we should reach unimplemented qcow tool wrapper if we are using "qcow2"" format. Signed-off-by: Guillaume <[email protected]>

[qcow-tool] [WIP] implement streaming

d77adf5

Signed-off-by: Guillaume <[email protected]>

gthvn1 force-pushed the gtn-add-qcow-tool branch from 99b3756 to d77adf5 Compare April 2, 2025 07:48

last-genius reviewed Apr 2, 2025

View reviewed changes

psafont reviewed Apr 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support qcow2 format in VDI export/import #6396

Support qcow2 format in VDI export/import #6396

gthvn1 commented Apr 1, 2025

gthvn1 commented Apr 1, 2025

last-genius left a comment

last-genius Apr 2, 2025

gthvn1 Apr 4, 2025

last-genius Apr 2, 2025 •

edited

Loading

gthvn1 Apr 4, 2025

last-genius Apr 4, 2025

last-genius Apr 2, 2025

gthvn1 Apr 4, 2025

last-genius Apr 2, 2025

psafont Apr 2, 2025

gthvn1 Apr 4, 2025

psafont left a comment

psafont Apr 2, 2025

psafont Apr 2, 2025

gthvn1 Apr 4, 2025

psafont Apr 2, 2025

gthvn1 Apr 4, 2025 •

edited

Loading

psafont Apr 2, 2025

gthvn1 Apr 4, 2025

psafont Apr 2, 2025

gthvn1 commented Apr 4, 2025

lindig commented Apr 8, 2025

Support qcow2 format in VDI export/import #6396

Are you sure you want to change the base?

Support qcow2 format in VDI export/import #6396

Conversation

gthvn1 commented Apr 1, 2025

gthvn1 commented Apr 1, 2025

last-genius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

last-genius Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psafont left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gthvn1 Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gthvn1 commented Apr 4, 2025

lindig commented Apr 8, 2025

last-genius Apr 2, 2025 •

edited

Loading

gthvn1 Apr 4, 2025 •

edited

Loading