3:59 p.m.
On Thu, Aug 27, 2015 at 09:39:30PM +0300, Roman Kagan wrote:
On Thu, Aug 27, 2015 at 04:08:43PM +0100, Richard W.M. Jones wrote:
> On Tue, Aug 11, 2015 at 08:00:34PM +0300, Roman Kagan wrote:
> > + let overlays =
> > + if not in_place then create_overlays source.s_disks
> > + else [] in
> > + let targets =
> > + if not in_place then init_targets overlays source output output_format
> > + else [] in
>
> This doesn't solve the problem I raised before which is that overlays
> and targets should never be empty lists.
Why shouldn't they?
Because the point of using the language is to use its type safety to
ensure there are no errors, now or in the future. I reproduced your
new main() function after the patch series at the end of this email so
we have something concrete to look at.
At the moment, it's (sort of) obvious that
in_place => overlays = targets = []
but we're not using the compiler to enforce that, so it could break in
future.
It can be made type-safe without a huge amount of work. Something
like this. First define a new type:
type conversion_mode =
| Copying of overlay list * target list
| In_place
Then change the code to look like below (I didn't change all of it,
but it should be fairly obvious from the examples here):
let conversion_mode =
if not in_place then
Copying (
let overlays = create_overlays source.s_disks in
let targets = init_targets overlays source output output_format in
overlays, targets)
else In_place in
...
(match conversion_mode with
Copying (overlays, _) -> populate_overlays g overlays
In_place -> populate_disks g source.s_disks
);
...
match conversion_mode with
| In_place ->
message (f_"Finishing off");
exit 0
| Copying (overlays, targets) ->
(* rest of main function follows here ... *)
> (2) v2v.ml calls out to either Copying or In_place -- see how
> virt-sparsify works.
I actually followed the advice you gave in the previous review and did
look into virt-sparsify. I must admit I didn't appreciate the amount of
code duplication between the two usage scenarios. That was exactly what
I wanted to avoid in my submission.
The posted code covers two usage scenarios like this:
- common steps (command line, initialization)
- conditional steps (creation of overlays, guestfs handle with either
overlays or original disks)
- common steps (inspection, space estimate, conversion)
- conditional steps (creation of destination VM)
That's basically all to it. How can this conveniently be split into
independent scenarios without code duplication and the risk of
forgetting to update one of the two when the common steps are being
modified?
OK so you've convinced me that a Common code module isn't
necessary and would lead to a lot of duplication.
However - I still don't like the bottom-up reworking of the code, and
changing that means having fewer, simpler patches in this series. And
I think the in-place/copying stuff can be made type-safe as above.
Rich.
----------------------------------------------------------------------
let main () =
(* Handle the command line. *)
let input, output,
debug_gc, debug_overlays, do_copy, in_place, network_map, no_trim,
output_alloc, output_format, output_name, print_source, root_choice =
Cmdline.parse_cmdline () in
(* Print the version, easier than asking users to tell us. *)
if verbose () then
printf "%s: %s %s (%s)\n%!"
prog Config.package_name Config.package_version Config.host_cpu;
if debug_gc then
at_exit (fun () -> Gc.compact());
let source = open_source input print_source in
let source = amend_source source output_name network_map in
let overlays =
if not in_place then create_overlays source.s_disks
else [] in
let targets =
if not in_place then init_targets overlays source output output_format
else [] in
let guestfs_kind =
if not in_place then "overlay" else "source VM" in
message (f_"Opening the %s") guestfs_kind;
let g = open_guestfs () in
if not in_place then populate_overlays g overlays
else populate_disks g source.s_disks;
g#launch ();
(* Inspection - this also mounts up the filesystems. *)
message (f_"Inspecting the %s") guestfs_kind;
let inspect = inspect_source g root_choice in
let mpstats = get_mpstats g in
check_free_space mpstats;
if not in_place then
check_target_free_space mpstats source targets output;
let keep_serial_console =
if not in_place then output#keep_serial_console
else true in
let guestcaps = do_convert g inspect source keep_serial_console in
if no_trim <> ["*"] && (do_copy || debug_overlays) then (
(* Doing fstrim on all the filesystems reduces the transfer size
* because unused blocks are marked in the overlay and thus do
* not have to be copied.
*)
message (f_"Mapping filesystem data to avoid copying unused and blank
areas");
do_fstrim g no_trim inspect;
);
message (f_"Closing the %s") guestfs_kind;
g#close ();
if in_place then (
message (f_"Finishing off");
exit 0
);
let target_firmware = get_target_firmware inspect guestcaps source output in
let target_buses = target_bus_assignment source targets guestcaps in
let targets =
if not do_copy then targets
else copy_targets targets input output output_alloc in
(* Create output metadata. *)
message (f_"Creating output metadata");
output#create_metadata source targets target_buses guestcaps inspect
target_firmware;
if debug_overlays then preserve_overlays overlays source.s_name;
message (f_"Finishing off");
delete_target_on_exit := false (* Don't delete target on exit. *)
----------------------------------------------------------------------
--
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