Skip to content

Support qcow2 format in VDI export/import #6396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
4 changes: 4 additions & 0 deletions ocaml/qcow-tool/cli/impl.ml
Original file line number Diff line number Diff line change
Expand Up @@ -844,3 +844,7 @@ let rehydrate _common input_filename output_filename =
>>= fun () -> Lwt.return (`Ok ())
in
Lwt_main.run t

let stream _common source output =
failwith
(Printf.sprintf "streaming from %s to %s is not implemented" source output)
20 changes: 20 additions & 0 deletions ocaml/qcow-tool/cli/main.ml
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,25 @@ let rehydrate_cmd =
, Cmd.info "rehydrate" ~sdocs:_common_options ~doc ~man
)

let stream_cmd =
let doc = "stream the contents of a virtual disk" in
let man =
[
`S "DESCRIPTION"
; `P
"Read the contents of a virtual disk from a source and write it to\n\
\ a destination that is a qcow2 file."
]
@ help
in
let source =
let doc = Printf.sprintf "The disk to be streamed" in
Arg.(value & opt string "stdin:" & info ["source"] ~doc)
in
( Term.(ret (const Impl.stream $ common_options_t $ source $ output))
, Cmd.info "stream" ~sdocs:_common_options ~doc ~man
)

let cmds =
[
info_cmd
Expand All @@ -475,6 +494,7 @@ let cmds =
; sha_cmd
; dehydrate_cmd
; rehydrate_cmd
; stream_cmd
]
|> List.map (fun (t, i) -> Cmd.v i t)

Expand Down
60 changes: 60 additions & 0 deletions ocaml/xapi/common_tool_wrapper.ml
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
(*
* 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.
*)

open Xapi_stdext_std.Xstringext
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


(** [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
raise
Api_errors.(
Server_error
( internal_error
, [
Printf.sprintf
"find_backend_device: Got domid %s but expected \
%s"
bedomid self
]
)
) ;
Some params
| _ ->
raise Not_found
)
| _ ->
raise Not_found
with _ -> None
1 change: 0 additions & 1 deletion ocaml/xapi/import_raw_vdi.ml
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@ let localhost_handler rpc session_id vdi_opt (req : Request.t)
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
34 changes: 31 additions & 3 deletions ocaml/xapi/qcow_tool_wrapper.ml
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,37 @@ let run_qcow_tool (progress_cb : int -> unit) (args : string list)
let update_task_progress (__context : Context.t) (x : int) =
TaskHelper.set_progress ~__context (float_of_int x /. 100.)

let qcow_of_device path =
let tapdisk_of_path path =
try
match Tapctl.of_device (Tapctl.create ()) path with
| _, str, Some (_, qcow) ->
debug "Found str %s and file %s" str qcow ;
Some qcow
| _ ->
None
with Not_found ->
debug "Device %s has an unknown driver" path ;
None
in
Common_tool_wrapper.find_backend_device path
|> Option.value ~default:path
|> tapdisk_of_path

let send (progress_cb : int -> unit) (unix_fd : Unix.file_descr) (path : string)
(size : Int64.t) =
debug "Qcow send called with a size of %Ld and path equal to %s" size path ;
let _ = progress_cb in
let _ = unix_fd in
run_qcow_tool progress_cb ["stream"] unix_fd
let _, source =
match (Stream_vdi.get_nbd_device path, qcow_of_device path) with
| Some (nbd_path, exportname), Some p ->
debug "get_nbd_device (path=%s, exportname=%s), p = %s" nbd_path
exportname p ;
(nbd_path, exportname)
| None, Some p ->
debug "nbd device not found but p = %s" p ;
("gtn_no_nbd", p)
| _ ->
("gtn_unknown", "gtn_unknown")
in
let args = ["stream"; "--source"; source; path] in
run_qcow_tool progress_cb args unix_fd
47 changes: 8 additions & 39 deletions ocaml/xapi/vhd_tool_wrapper.ml
Original file line number Diff line number Diff line change
Expand Up @@ -113,42 +113,6 @@ let receive progress_cb format protocol (s : Unix.file_descr)
in
run_vhd_tool progress_cb args s s' path

(** [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

Comment on lines -116 to -151
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, I saw some common patterns.

(** [vhd_of_device path] returns (Some vhd) where 'vhd' is the vhd leaf backing a particular device [path] or None.
[path] may either be a blktap2 device *or* a blkfront device backed by a blktap2 device. If the latter then
the script must be run in the same domain as blkback. *)
Expand Down Expand Up @@ -178,22 +142,27 @@ let vhd_of_device path =
debug "Device %s has an unknown driver" path ;
None
in
find_backend_device path |> Option.value ~default:path |> tapdisk_of_path
Common_tool_wrapper.find_backend_device path
|> Option.value ~default:path
|> tapdisk_of_path

let send progress_cb ?relative_to (protocol : string) (dest_format : string)
(s : Unix.file_descr) (path : string) (size : Int64.t) (prefix : string) =
let s' = Uuidx.(to_string (make ())) in
debug "GTNDEBUG: path is %s" path ;
debug "GTNDEBUG: prefix is %s" prefix ;
let source_format, source =
debug "GTNDEBUG: get_nbd_device %s" path ;
debug "GTNDEBUG: s' is %s" s' ;
match (Stream_vdi.get_nbd_device path, vhd_of_device path, relative_to) with
| Some (nbd_server, exportname), _, None ->
debug "GTNDEBUG: nbdhybrid %s:%s:%s:%Ld" path nbd_server exportname size ;
( "nbdhybrid"
, Printf.sprintf "%s:%s:%s:%Ld" path nbd_server exportname size
)
| Some _, Some vhd, Some _ | None, Some vhd, _ ->
debug "GTNDEBUG: hybrid %s" (path ^ ":" ^ vhd) ;
("hybrid", path ^ ":" ^ vhd)
| None, None, None ->
debug "GTNDEBUG: raw %s" path ;
("raw", path)
| _, None, Some _ ->
let msg = "Cannot compute differences on non-VHD images" in
Expand Down
Loading