-
Notifications
You must be signed in to change notification settings - Fork 292
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
Changes from 1 commit
0647d38
c2621a6
9bd40d4
def1cf6
81232c5
05b56c4
2f6c847
ecc0ee1
15f0f6e
d77adf5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -158,11 +158,12 @@ let localhost_handler rpc session_id vdi_opt (req : Request.t) | |
in | ||
Http_svr.headers s headers ; | ||
( match format with | ||
| Raw | Vhd -> | ||
| Raw | Vhd | Qcow -> | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should there be calls to (some, currently undefined) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
(* | ||
* 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. | ||
*) | ||
|
||
Comment on lines
+1
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Separate commit not separate PR right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
module D = Debug.Make (struct let name = "qcow_tool_wrapper" end) | ||
|
||
open D | ||
|
||
let unimplemented () = | ||
raise | ||
(Api_errors.Server_error (Api_errors.unimplemented_in_qcow_tool_wrapper, [])) | ||
|
||
let run_qcow_tool (progress_cb : int -> unit) (args : string list) | ||
(ufd : Unix.file_descr) = | ||
let qcow_tool = !Xapi_globs.qcow_tool in | ||
info "Executing %s %s" qcow_tool (String.concat " " args) ; | ||
let open Forkhelpers in | ||
let pipe_read, pipe_write = Unix.pipe () in | ||
Xapi_stdext_pervasives.Pervasiveext.finally | ||
(fun () -> | ||
match | ||
with_logfile_fd "qcow-tool" (fun log_fd -> | ||
let ufd_str = Uuidx.(to_string (make ())) in | ||
let pid = | ||
safe_close_and_exec None (Some pipe_write) (Some log_fd) | ||
[(ufd_str, ufd)] | ||
qcow_tool args | ||
in | ||
let _, status = waitpid pid in | ||
if status <> Unix.WEXITED 0 then ( | ||
error "qcow-tool failed, returning VDI_IO_ERROR" ; | ||
raise | ||
(Api_errors.Server_error | ||
(Api_errors.vdi_io_error, ["Device I/O errors"]) | ||
) | ||
) | ||
) | ||
with | ||
| Success (out, _) -> | ||
debug "%s" out | ||
| Failure (out, e) -> | ||
error "qcow-tool output: %s" out ; | ||
raise e | ||
) | ||
(fun () -> List.iter Unix.close [pipe_read; pipe_write]) | ||
|
||
let update_task_progress (__context : Context.t) (x : int) = | ||
TaskHelper.set_progress ~__context (float_of_int x /. 100.) | ||
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 explicitmatch
wouldThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.