Libguestfs - Feb 2016 - [PATCH 1/2] v2v: -o libvirt: Refactor video and graphics elements.

This is just a refactoring and doesn't change the meaning of the code.
---
 v2v/output_libvirt.ml | 60 +++++++++++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/v2v/output_libvirt.ml b/v2v/output_libvirt.ml
index 68af3de..d1cbaa1 100644
--- a/v2v/output_libvirt.ml
+++ b/v2v/output_libvirt.ml
@@ -222,46 +222,44 @@ let create_libvirt_xml ?pool source target_buses guestcaps
   (* Same as old virt-v2v, we always add a display here even if it was
    * missing from the old metadata.
    *)
-  let video, graphics -    let video_model, graphics +  let video +    let
video_model        match guestcaps.gcaps_video with
-      | QXL ->
-        e "model" [ "type", "qxl";
"ram", "65536" ] [],
-        e "graphics" [ "type", "vnc" ] []
-      | Cirrus ->
-        e "model" [ "type", "cirrus";
"vram", "9216" ] [],
-        e "graphics" [ "type", "spice" ] [] in
-
+      | QXL ->    e "model" [ "type", "qxl";
"ram", "65536" ] []
+      | Cirrus -> e "model" [ "type",
"cirrus"; "vram", "9216" ] [] in
     append_attr ("heads", "1") video_model;
-    let video = e "video" [] [ video_model ] in
+    e "video" [] [ video_model ] in
 
-    (match source.s_display with
-    | Some { s_keymap = Some km } -> append_attr ("keymap", km)
graphics
-    | _ -> ());
-    (match source.s_display with
-    | Some { s_password = Some pw } -> append_attr ("passwd", pw)
graphics
-    | _ -> ());
-    (match source.s_display with
-    | Some { s_listen = listen } ->
+  let graphics +    match guestcaps.gcaps_video with
+    | QXL ->    e "graphics" [ "type", "vnc" ]
[]
+    | Cirrus -> e "graphics" [ "type", "spice"
] [] in
+
+  (match source.s_display with
+   | Some { s_keymap = Some km } -> append_attr ("keymap", km)
graphics
+   | _ -> ());
+  (match source.s_display with
+   | Some { s_password = Some pw } -> append_attr ("passwd", pw)
graphics
+   | _ -> ());
+  (match source.s_display with
+   | Some { s_listen = listen } ->
       (match listen with
-      | LAddress a ->
-        let sub = e "listen" [ "type", "address";
"address", a ] [] in
-        append_child sub graphics
-      | LNetwork n ->
-        let sub = e "listen" [ "type", "network";
"network", n ] [] in
-        append_child sub graphics
-      | LNone -> ())
-    | _ -> ());
-    (match source.s_display with
-    | Some { s_port = Some p } ->
+       | LAddress a ->
+          let sub = e "listen" [ "type",
"address"; "address", a ] [] in
+          append_child sub graphics
+       | LNetwork n ->
+          let sub = e "listen" [ "type",
"network"; "network", n ] [] in
+          append_child sub graphics
+       | LNone -> ())
+   | _ -> ());
+  (match source.s_display with
+   | Some { s_port = Some p } ->
       append_attr ("autoport", "no") graphics;
       append_attr ("port", string_of_int p) graphics
-    | _ ->
+   | _ ->
       append_attr ("autoport", "yes") graphics;
       append_attr ("port", "-1") graphics);
 
-    video, graphics in
-
   let sound      match source.s_sound with
     | None -> []
-- 
2.5.0

Old virt-v2v changed the <graphics> to VNC if the guest was using the
Cirrus hardware and Spice if the guest used QXL.

In commit 3d1cb74b3eee51c5d015032ad964892be914ea9c I got the logic
backwards, using Spice if the guest used Cirrus and VNC if the guest
used QXL, which obviously makes no sense.

In this commit, I preserve the original <graphics> type from the
source guest.  This has the advantage that the user can use the same
method to access the guest after conversion.  If the source guest had
no <graphics> element, then we force VNC (a safe choice), and if the
source guest is a local disk that we use SDL, but this should only be
used for testing.

Thanks: Xiaodai Wang for the original bug report here:
https://bugzilla.redhat.com/show_bug.cgi?id=1225789#c10
---
 v2v/output_libvirt.ml | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/v2v/output_libvirt.ml b/v2v/output_libvirt.ml
index d1cbaa1..aedde61 100644
--- a/v2v/output_libvirt.ml
+++ b/v2v/output_libvirt.ml
@@ -231,9 +231,14 @@ let create_libvirt_xml ?pool source target_buses guestcaps
     e "video" [] [ video_model ] in
 
   let graphics -    match guestcaps.gcaps_video with
-    | QXL ->    e "graphics" [ "type", "vnc" ]
[]
-    | Cirrus -> e "graphics" [ "type", "spice"
] [] in
+    match source.s_display with
+    | None -> e "graphics" [ "type", "vnc" ]
[]
+    | Some { s_display_type = Window } ->
+       e "graphics" [ "type", "sdl" ] []
+    | Some { s_display_type = VNC } ->
+       e "graphics" [ "type", "vnc" ] []
+    | Some { s_display_type = Spice } ->
+       e "graphics" [ "type", "spice" ] [] in
 
   (match source.s_display with
    | Some { s_keymap = Some km } -> append_attr ("keymap", km)
graphics
-- 
2.5.0

On Friday 26 February 2016 10:15:45 Richard W.M. Jones
wrote:
> This is just a refactoring and doesn't change the meaning of the code.
> ---
>  v2v/output_libvirt.ml | 60
+++++++++++++++++++++++++--------------------------
>  1 file changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/v2v/output_libvirt.ml b/v2v/output_libvirt.ml
> index 68af3de..d1cbaa1 100644
> --- a/v2v/output_libvirt.ml
> +++ b/v2v/output_libvirt.ml
> @@ -222,46 +222,44 @@ let create_libvirt_xml ?pool source target_buses
guestcaps
>    (* Same as old virt-v2v, we always add a display here even if it was
>     * missing from the old metadata.
>     *)
> -  let video, graphics > -    let video_model, graphics > +  let
video > +    let video_model >        match guestcaps.gcaps_video with
> -      | QXL ->
> -        e "model" [ "type", "qxl";
"ram", "65536" ] [],
> -        e "graphics" [ "type", "vnc" ] []
> -      | Cirrus ->
> -        e "model" [ "type", "cirrus";
"vram", "9216" ] [],
> -        e "graphics" [ "type", "spice" ] []
in
> -
> +      | QXL ->    e "model" [ "type",
"qxl"; "ram", "65536" ] []
> +      | Cirrus -> e "model" [ "type",
"cirrus"; "vram", "9216" ] [] in
>      append_attr ("heads", "1") video_model;
> -    let video = e "video" [] [ video_model ] in
> +    e "video" [] [ video_model ] in
>  
> -    (match source.s_display with
> -    | Some { s_keymap = Some km } -> append_attr ("keymap",
km) graphics
> -    | _ -> ());
> -    (match source.s_display with
> -    | Some { s_password = Some pw } -> append_attr ("passwd",
pw) graphics
> -    | _ -> ());
> -    (match source.s_display with
> -    | Some { s_listen = listen } ->
> +  let graphics > +    match guestcaps.gcaps_video with
> +    | QXL ->    e "graphics" [ "type",
"vnc" ] []
> +    | Cirrus -> e "graphics" [ "type",
"spice" ] [] in
> +
> +  (match source.s_display with
> +   | Some { s_keymap = Some km } -> append_attr ("keymap",
km) graphics
> +   | _ -> ());
> +  (match source.s_display with
> +   | Some { s_password = Some pw } -> append_attr ("passwd",
pw) graphics
> +   | _ -> ());
> +  (match source.s_display with
> +   | Some { s_listen = listen } ->
>        (match listen with
> -      | LAddress a ->
> -        let sub = e "listen" [ "type",
"address"; "address", a ] [] in
> -        append_child sub graphics
> -      | LNetwork n ->
> -        let sub = e "listen" [ "type",
"network"; "network", n ] [] in
> -        append_child sub graphics
> -      | LNone -> ())
> -    | _ -> ());
> -    (match source.s_display with
> -    | Some { s_port = Some p } ->
> +       | LAddress a ->
> +          let sub = e "listen" [ "type",
"address"; "address", a ] [] in
> +          append_child sub graphics
> +       | LNetwork n ->
> +          let sub = e "listen" [ "type",
"network"; "network", n ] [] in
> +          append_child sub graphics
> +       | LNone -> ())
> +   | _ -> ());
> +  (match source.s_display with
> +   | Some { s_port = Some p } ->
>        append_attr ("autoport", "no") graphics;
>        append_attr ("port", string_of_int p) graphics
> -    | _ ->
> +   | _ ->
>        append_attr ("autoport", "yes") graphics;
>        append_attr ("port", "-1") graphics);
>  
> -    video, graphics in
> -
>    let sound >      match source.s_sound with
>      | None -> []
The series LGTM.

Thanks,
-- 
Pino Toscano