thr3ads.net - Libguestfs - [Libguestfs] [PATCH] v2v: Make the interface between cmdline.ml and v2v.ml [Nov 2015]

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2015-Nov-10 20:00 UTC

[Libguestfs] [PATCH] v2v: Make the interface between cmdline.ml and v2v.ml

I'm interested to hear opinions on whether this makes the code
clearer, or not.

This is virt-v2v, but many other virt-* tools work the same way, and
analogous changes could be made.

Currently when command line argument parsing is done in 'cmdline.ml'
the list of parsed parameters is passed to the main program in a very
long tuple.  Each parameter is strongly typed, but not named (so for
example two 'bool' flags on the command line might be swapped
accidentally).

This patch uses a struct to pass the parameters instead, so they are
both strongly typed *and* named - two parameters with the same type
could not be swapped by accident.

Also the type of each parameter is now defined in the struct 'type'
definition, which also appears in a 'cmdline.mli' file.

In addition, in the main program it should be clearer where a
particular variable comes from, ie. you have to write
'cmdline.output_alloc' instead of 'output_alloc', which may make
it
clearer that it comes from the command line '-oa' parameter, thus
making the code easier to understand.

Rich.

Richard W.M. Jones

2015-Nov-10 20:00 UTC

head link

[Libguestfs] [PATCH] v2v: Make the interface between cmdline.ml and v2v.ml explicit.

---
 v2v/Makefile.am |  1 +
 v2v/cmdline.ml  | 27 +++++++++++++++++++++++----
 v2v/cmdline.mli | 35 +++++++++++++++++++++++++++++++++++
 v2v/v2v.ml      | 57 ++++++++++++++++++++++++++++-----------------------------
 4 files changed, 87 insertions(+), 33 deletions(-)
 create mode 100644 v2v/cmdline.mli

diff --git a/v2v/Makefile.am b/v2v/Makefile.am
index 49eeedc..5dfef6e 100644
--- a/v2v/Makefile.am
+++ b/v2v/Makefile.am
@@ -40,6 +40,7 @@ EXTRA_DIST = \
 CLEANFILES = *~ *.annot *.cmi *.cmo *.cmx *.cmxa *.o virt-v2v
 
 SOURCES_MLI = \
+	cmdline.mli \
 	convert_linux.mli \
 	convert_windows.mli \
 	curl.mli \
diff --git a/v2v/cmdline.ml b/v2v/cmdline.ml
index f6e75ce..100c2a4 100644
--- a/v2v/cmdline.ml
+++ b/v2v/cmdline.ml
@@ -26,6 +26,20 @@ open Common_utils
 open Types
 open Utils
 
+type cmdline = {
+  compressed : bool;
+  debug_overlays : bool;
+  do_copy : bool;
+  in_place : bool;
+  network_map : ((vnet_type * string) * string) list;
+  no_trim : string list;
+  output_alloc : output_allocation;
+  output_format : string option;
+  output_name : string option;
+  print_source : bool;
+  root_choice : [`Ask|`Single|`First|`Dev of string];
+}
+
 let parse_cmdline ()    let compressed = ref false in
   let debug_overlays = ref false in
@@ -416,7 +430,12 @@ read the man page virt-v2v(1).
       } in
       Output_vdsm.output_vdsm os vdsm_params vmtype output_alloc in
 
-  input, output,
-  compressed, debug_overlays, do_copy, in_place, network_map, no_trim,
-  output_alloc, output_format, output_name,
-  print_source, root_choice
+  {
+    compressed = compressed; debug_overlays = debug_overlays;
+    do_copy = do_copy; in_place = in_place; network_map = network_map;
+    no_trim = no_trim;
+    output_alloc = output_alloc; output_format = output_format;
+    output_name = output_name;
+    print_source = print_source; root_choice = root_choice;
+  },
+  input, output
diff --git a/v2v/cmdline.mli b/v2v/cmdline.mli
new file mode 100644
index 0000000..f7c3753
--- /dev/null
+++ b/v2v/cmdline.mli
@@ -0,0 +1,35 @@
+(* virt-v2v
+ * Copyright (C) 2009-2015 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.
+ *)
+
+(** Command line argument parsing. *)
+
+type cmdline = {
+  compressed : bool;
+  debug_overlays : bool;
+  do_copy : bool;
+  in_place : bool;
+  network_map : ((Types.vnet_type * string) * string) list;
+  no_trim : string list;
+  output_alloc : Types.output_allocation;
+  output_format : string option;
+  output_name : string option;
+  print_source : bool;
+  root_choice : [`Ask|`Single|`First|`Dev of string];
+}
+
+val parse_cmdline : unit -> cmdline * Types.input * Types.output
diff --git a/v2v/v2v.ml b/v2v/v2v.ml
index a4f35ce..7e8b459 100644
--- a/v2v/v2v.ml
+++ b/v2v/v2v.ml
@@ -27,6 +27,8 @@ open Common_utils
 open Types
 open Utils
 
+open Cmdline
+
 (* Mountpoint stats, used for free space estimation. *)
 type mpstat = {
   mp_dev : string;                      (* Filesystem device (eg. /dev/sda1) *)
@@ -49,24 +51,20 @@ let () = Random.self_init ()
 
 let rec main ()    (* Handle the command line. *)
-  let input, output,
-    compressed, debug_overlays, do_copy, in_place, network_map, no_trim,
-    output_alloc, output_format, output_name, print_source, root_choice -   
Cmdline.parse_cmdline () in
+  let cmdline, input, output = parse_cmdline () in
 
   (* Print the version, easier than asking users to tell us. *)
   if verbose () then
     printf "%s: %s %s (%s)\n%!"
       prog Guestfs_config.package_name Guestfs_config.package_version
Guestfs_config.host_cpu;
 
-  let source = open_source input print_source in
-  let source = amend_source source output_name network_map in
+  let source = open_source cmdline input in
+  let source = amend_source cmdline source in
 
   let conversion_mode -    if not in_place then (
+    if not cmdline.in_place then (
       let overlays = create_overlays source.s_disks in
-      let targets -        init_targets overlays source output output_format
compressed in
+      let targets = init_targets cmdline output source overlays in
       Copying (overlays, targets)
     )
     else In_place in
@@ -88,7 +86,7 @@ let rec main ()  
   (* Inspection - this also mounts up the filesystems. *)
   message (f_"Inspecting the overlay");
-  let inspect = inspect_source g root_choice in
+  let inspect = inspect_source cmdline g in
 
   let mpstats = get_mpstats g in
   check_free_space mpstats;
@@ -103,13 +101,14 @@ let rec main ()  
   g#umount_all ();
 
-  if no_trim <> ["*"] && (do_copy || debug_overlays)
then (
+  if cmdline.no_trim <> ["*"] &&
+       (cmdline.do_copy || cmdline.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;
+    do_fstrim g cmdline.no_trim inspect;
   );
 
   (match conversion_mode with
@@ -132,26 +131,26 @@ let rec main ()           printf "%s%!"
(string_of_target_buses target_buses);
 
        let targets -         if not do_copy then targets
-         else copy_targets targets input output output_alloc compressed in
+         if not cmdline.do_copy then targets
+         else copy_targets cmdline targets input output 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;
+       if cmdline.debug_overlays then preserve_overlays overlays source.s_name;
 
        delete_target_on_exit := false  (* Don't delete target on exit. *)
   );
   message (f_"Finishing off")
 
-and open_source input print_source +and open_source cmdline input    message
(f_"Opening the source %s") input#as_options;
   let source = input#source () in
 
   (* Print source and stop. *)
-  if print_source then (
+  if cmdline.print_source then (
     printf (f_"Source guest information (--print-source option):\n");
     printf "\n";
     printf "%s\n" (string_of_source source);
@@ -178,10 +177,10 @@ and open_source input print_source  
   source
 
-and amend_source source output_name network_map +and amend_source cmdline
source    (* Map source name. *)
   let source -    match output_name with
+    match cmdline.output_name with
     | None -> source
     (* Note the s_orig_name field retains the original name in case we
      * need it for some reason.
@@ -195,12 +194,12 @@ and amend_source source output_name network_map         
(* Look for a --network or --bridge parameter which names this
          * network/bridge (eg. --network in:out).
          *)
-        let new_name = List.assoc (t, vnet) network_map in
+        let new_name = List.assoc (t, vnet) cmdline.network_map in
         { nic with s_vnet = new_name }
       with Not_found ->
         try
           (* Not found, so look for a default mapping (eg. --network out). *)
-          let new_name = List.assoc (t, "") network_map in
+          let new_name = List.assoc (t, "") cmdline.network_map in
           { nic with s_vnet = new_name }
         with Not_found ->
           (* Not found, so return the original NIC unchanged. *)
@@ -248,7 +247,7 @@ and create_overlays src_disks          ov_virtual_size =
vsize; ov_source = source }
   ) src_disks
 
-and init_targets overlays source output output_format compressed +and
init_targets cmdline output source overlays    (* Work out where we will write
the final output.  Do this early
    * just so we can display errors to the user before doing too much
    * work.
@@ -259,7 +258,7 @@ and init_targets overlays source output output_format
compressed        fun ov ->
         (* What output format should we use? *)
         let format -          match output_format, ov.ov_source.s_format with
+          match cmdline.output_format, ov.ov_source.s_format with
           | Some format, _ -> format    (* -of overrides everything *)
           | None, Some format -> format (* same as backing format *)
           | None, None ->
@@ -276,7 +275,7 @@ and init_targets overlays source output output_format
compressed            error (f_"output format should be 'raw' or
'qcow2'.\n\nUse the '-of <format>' option to select a
different output format for the converted guest.\n\nOther output formats are not
supported at the moment, although might be considered in future.");
 
         (* Only allow compressed with qcow2. *)
-        if compressed && format <> "qcow2" then
+        if cmdline.compressed && format <> "qcow2" then
           error (f_"the --compressed flag is only allowed when the output
format is qcow2 (-of qcow2)");
 
         (* output#prepare_targets will fill in the target_file field.
@@ -307,7 +306,7 @@ and populate_disks g src_disks                           
~discard:"besteffort"
   ) src_disks
 
-and inspect_source g root_choice +and inspect_source cmdline g    let roots =
g#inspect_os () in
   let roots = Array.to_list roots in
 
@@ -317,7 +316,7 @@ and inspect_source g root_choice         error
(f_"inspection could not detect the source guest (or physical
machine).\n\nAssuming that you are running virt-v2v/virt-p2v on a source which
is supported (and not, for example, a blank disk), then this should not happen. 
You should run 'virt-v2v -v -x ... >& log' and attach the
complete log to a new bug report (see http://libguestfs.org).\n\nNo root device
found in this operating system image.");
     | [root] -> root
     | roots ->
-      match root_choice with
+      match cmdline.root_choice with
       | `Ask ->
         (* List out the roots and ask the user to choose. *)
         printf "\n***\n";
@@ -762,7 +761,7 @@ and get_target_firmware inspect guestcaps source output  
 and delete_target_on_exit = ref true
 
-and copy_targets targets input output output_alloc compressed +and copy_targets
cmdline targets input output    (* Copy the source to the output. *)
   at_exit (fun () ->
     if !delete_target_on_exit then (
@@ -807,7 +806,7 @@ and copy_targets targets input output output_alloc
compressed         *)
       (* What output preallocation mode should we use? *)
       let preallocation -        match t.target_format, output_alloc with
+        match t.target_format, cmdline.output_alloc with
         | ("raw"|"qcow2"), Sparse -> Some
"sparse"
         | ("raw"|"qcow2"), Preallocated -> Some
"full"
         | _ -> None (* ignore -oa flag for other formats *) in
@@ -821,7 +820,7 @@ and copy_targets targets input output output_alloc
compressed          sprintf "qemu-img convert%s -n -f qcow2 -O %s%s %s
%s"
           (if not (quiet ()) then " -p" else "")
           (quote t.target_format)
-          (if compressed then " -c" else "")
+          (if cmdline.compressed then " -c" else "")
           (quote overlay_file)
           (quote t.target_file) in
       if verbose () then printf "%s\n%!" cmd;
-- 
2.5.0

Pino Toscano

2015-Nov-11 13:28 UTC

head link

Re: [Libguestfs] [PATCH] v2v: Make the interface between cmdline.ml and v2v.ml

On Tuesday 10 November 2015 20:00:56 Richard W.M. Jones
wrote:> I'm interested to hear opinions on whether this makes the code
> clearer, or not.
I'd say it does, indeed.
> This is virt-v2v, but many other virt-* tools work the same way, and
> analogous changes could be made.
+1 for applying the same on other OCaml tools.
> Currently when command line argument parsing is done in
'cmdline.ml'
> the list of parsed parameters is passed to the main program in a very
> long tuple.  Each parameter is strongly typed, but not named (so for
> example two 'bool' flags on the command line might be swapped
> accidentally).
> 
> This patch uses a struct to pass the parameters instead, so they are
> both strongly typed *and* named - two parameters with the same type
> could not be swapped by accident.
> 
> Also the type of each parameter is now defined in the struct 'type'
> definition, which also appears in a 'cmdline.mli' file.
> 
> In addition, in the main program it should be clearer where a
> particular variable comes from, ie. you have to write
> 'cmdline.output_alloc' instead of 'output_alloc', which may
make it
> clearer that it comes from the command line '-oa' parameter, thus
> making the code easier to understand.
Sounds nice as approach -- why not adding also input and output to the
struct?

Thanks,
-- 
Pino Toscano

Richard W.M. Jones

2015-Nov-11 14:52 UTC

head link

Re: [Libguestfs] [PATCH] v2v: Make the interface between cmdline.ml and v2v.ml

On Wed, Nov 11, 2015 at 02:28:16PM +0100, Pino Toscano
wrote:> Sounds nice as approach -- why not adding also input and output to the
> struct?
I did that initially, but I found that confusing.  The problem is that
input & output are mutable objects (so they are passed to virtually
every function and they are updated as v2v.ml progresses), whereas all
of the other cmdline.* fields are immutable cmdline parameter values.

I don't think any other tool apart from virt-v2v would have this
problem/inconsistency though.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Maybe Matching Threads

Search for more maybe matching threads

Libguestfs - Nov 2015 - [PATCH] v2v: Make the interface between cmdline.ml and v2v.ml

[Libguestfs] [PATCH] v2v: Make the interface between cmdline.ml and v2v.ml

[Libguestfs] [PATCH] v2v: Make the interface between cmdline.ml and v2v.ml explicit.

Re: [Libguestfs] [PATCH] v2v: Make the interface between cmdline.ml and v2v.ml

Re: [Libguestfs] [PATCH] v2v: Make the interface between cmdline.ml and v2v.ml

Maybe Matching Threads