Libguestfs - May 2016 - Re: [PATCH 03/11] customize: fix windows firstboot script

Hi there!

I know it's been a while since I posted my first version of some patches.
But here I have rebased them on top of Roman's work and added a few other
ones.

Cédric Bosdonnat (11):
  v2v: also search for windows virtio drivers in symlinks
  Update packagelist for SLES
  customize: fix windows firstboot script
  customize: change windows firstboot path
  customize: add support for pvvxsvc
  v2v: document SUSE's Xen as a working input hypervisor
  v2v: add support for SUSE VMDP drivers
  v2v: don't set spice display if QXL isn't supported
  appliance: fix errors in init for SLE / openSUSE
  v2v: rename RHEL 5 Xen input section into Xen in man
  v2v: improve initrd search

 appliance/init               |  5 ++-
 appliance/packagelist.in     |  4 +--
 builder/virt-builder.pod     | 13 ++++++--
 customize/firstboot.ml       | 53 ++++++++++++++++++--------------
 customize/virt-customize.pod |  6 ++++
 sysprep/virt-sysprep.pod     |  6 ++++
 v2v/convert_linux.ml         | 11 +++----
 v2v/convert_windows.ml       | 73 ++++++++++++++++++++++++++++++++++++--------
 v2v/output_libvirt.ml        |  4 ++-
 v2v/virt-v2v.pod             | 15 +++++++--
 v2v/windows_virtio.ml        | 27 +++++++++++-----
 11 files changed, 157 insertions(+), 60 deletions(-)

-- 
2.6.6

To allow saving save in the package shipping the windows virtio
drivers, we can use symlinks between the drivers folders. For example
SUSE VMDP drivers are the same for Win8.1 and Win2012r2, one folder
is a symlink to the other.

To take advantages of this, find must use the -L flag.
---
 v2v/windows_virtio.ml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
index be50107..7e9f735 100644
--- a/v2v/windows_virtio.ml
+++ b/v2v/windows_virtio.ml
@@ -222,7 +222,7 @@ and ddb_regedits current_cs drv_name drv_pciid  and
copy_drivers g inspect driverdir    let ret = ref false in
   if is_directory virtio_win then (
-    let cmd = sprintf "cd %s && find -type f" (quote
virtio_win) in
+    let cmd = sprintf "cd %s && find -L -type f" (quote
virtio_win) in
     let paths = external_command cmd in
     List.iter (
       fun path ->
-- 
2.6.6

SUSE Linux Entreprise Server doesn't have dhcpcd and the hivex package
is not in the default repositories. Better use dhcp-client and libhivex0
---
 appliance/packagelist.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/appliance/packagelist.in b/appliance/packagelist.in
index daf589e..c221854 100644
--- a/appliance/packagelist.in
+++ b/appliance/packagelist.in
@@ -132,11 +132,11 @@ ifelse(SUSE,1,
   augeas-lenses
   btrfsprogs
   cryptsetup
-  dhcpcd
+  dhcp-client
   genisoimage
   glibc-locale
   gptfdisk
-  hivex
+  libhivex0
   iproute2
   iputils
   libcap2
-- 
2.6.6

Sync the windows firstboot script with its linux brother. Also change
the main redirection to append to the log rather than overwriting it.
With this change, the firstboot script will resist reboots in the
executed scripts.
---
 customize/firstboot.ml | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/customize/firstboot.ml b/customize/firstboot.ml
index aa5b694..83bd808 100644
--- a/customize/firstboot.ml
+++ b/customize/firstboot.ml
@@ -232,7 +232,7 @@ set log=%%firstboot%%\\log.txt
 set scripts=%%firstboot%%\\scripts
 set scripts_done=%%firstboot%%\\scripts-done
 
-call :main > \"%%log%%\" 2>&1
+call :main >> \"%%log%%\" 2>&1
 exit /b
 
 :main
@@ -244,15 +244,14 @@ if not exist \"%%scripts_done%%\" (
 
 for %%%%f in (\"%%scripts%%\"\\*.bat) do (
   echo running \"%%%%f\"
-  call \"%%%%f\"
-  set elvl=!errorlevel!
-  echo .... exit code !elvl!
-  if !elvl! equ 0 (
-    move \"%%%%f\" \"%%scripts_done%%\"
-  )
+  move \"%%%%f\" \"%%scripts_done%%\"
+  pushd \"%%scripts_done%%\"
+  call \"%%%%~nf\"
+  popd
 )
 
 echo uninstalling firstboot service
+rmdir /S /Q \"%%scripts_done%%\"
 rhsrvany.exe -s firstboot uninstall
 " firstboot_dir_win in
 
-- 
2.6.6

Rename the C:\Program Files\Red Hat\Firstboot folder into the more vendor
independent C:\Program Files\Guestfs\Firstboot.
---
 builder/virt-builder.pod | 2 +-
 customize/firstboot.ml   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builder/virt-builder.pod b/builder/virt-builder.pod
index 9a49138..dc36c13 100644
--- a/builder/virt-builder.pod
+++ b/builder/virt-builder.pod
@@ -849,7 +849,7 @@ C<VIRT_TOOLS_DATA_DIR> environment variable; if not
set, a compiled-in
 default will be used (something like F</usr/share/virt-tools>).
 
 The output of the first boot scripts is available in the guest as
-F<C:\Program Files\Red Hat\Firstboot\log.txt>.
+F<C:\Program Files\Guestfs\Firstboot\log.txt>.
 
 =back
 
diff --git a/customize/firstboot.ml b/customize/firstboot.ml
index 83bd808..4167098 100644
--- a/customize/firstboot.ml
+++ b/customize/firstboot.ml
@@ -211,7 +211,7 @@ module Windows = struct
           g#mkdir_p firstboot_dir;
           loop firstboot_dir firstboot_dir_win path
       in
-      loop "" "C:" ["Program Files"; "Red
Hat"; "Firstboot"] in
+      loop "" "C:" ["Program Files";
"Guestfs"; "Firstboot"] in
 
     g#mkdir_p (firstboot_dir // "scripts");
 
-- 
2.6.6

SUSE VMDP comes with a replacement for rhsrvany.exe named pvvxsvc.exe.
Check for either one of them instead of only rhsrvany.
---
 builder/virt-builder.pod     | 11 +++++++++--
 customize/firstboot.ml       | 38 +++++++++++++++++++++++---------------
 customize/virt-customize.pod |  6 ++++++
 sysprep/virt-sysprep.pod     |  6 ++++++
 v2v/virt-v2v.pod             |  6 ++++++
 5 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/builder/virt-builder.pod b/builder/virt-builder.pod
index dc36c13..be5b568 100644
--- a/builder/virt-builder.pod
+++ b/builder/virt-builder.pod
@@ -840,11 +840,12 @@ F<~root/virt-sysprep-firstboot.log>.
 =item Windows
 
 F<rhsrvany.exe>, available from sources at
-L<https://github.com/rwmjones/rhsrvany>, is installed to run the
+L<https://github.com/rwmjones/rhsrvany>, or F<pvvxsvc.exe>,
available
+with SUSE VMDP is installed to run the
 first boot scripts.  It is required, and the setup of first boot
 scripts will fail if it is not present.
 
-F<rhsrvany.exe> is copied from the location pointed to by the
+F<rhsrvany.exe> or F<pvvxsvc.exe> is copied from the location
pointed to by the
 C<VIRT_TOOLS_DATA_DIR> environment variable; if not set, a compiled-in
 default will be used (something like F</usr/share/virt-tools>).
 
@@ -1820,6 +1821,12 @@ I<--firstboot> or I<--firstboot-command>
options with Windows guests.
 
 See also: C<https://github.com/rwmjones/rhsrvany>
 
+=item F<pvvxsvc.exe>
+
+This is a Windows binary shipped with SUSE VMDP, used to install a
"firstboot"
+script in Windows guests.  It is required if you intend to use the
+I<--firstboot> or I<--firstboot-command> options with Windows
guests.
+
 =back
 
 =item C<XDG_CACHE_HOME>
diff --git a/customize/firstboot.ml b/customize/firstboot.ml
index 4167098..7d9325d 100644
--- a/customize/firstboot.ml
+++ b/customize/firstboot.ml
@@ -185,19 +185,27 @@ module Windows = struct
       try Sys.getenv "VIRT_TOOLS_DATA_DIR"
       with Not_found -> Guestfs_config.datadir // "virt-tools" in
 
-    (* rhsrvany.exe must exist.
+    (* either rhsrvany.exe or pvvxsvc.exe must exist.
      *
      * (Check also that it's not a dangling symlink but a real file).
      *)
-    let rhsrvany_exe = virt_tools_data_dir // "rhsrvany.exe" in
-    (try
-       let chan = open_in rhsrvany_exe in
-       close_in chan
-     with
-       Sys_error msg ->
-         error (f_"'%s' is missing.  This file is required in
order to install Windows firstboot scripts.  You can get it by building rhsrvany
(https://github.com/rwmjones/rhsrvany).  Original error: %s")
-           rhsrvany_exe msg
-    );
+    let services = ["rhsrvany.exe"; "pvvxsvc.exe"] in
+    let srvany = (
+      try
+        List.find (
+          fun service -> (
+            try
+              let chan = open_in (virt_tools_data_dir // service) in
+              close_in chan;
+              true
+            with _ ->
+              false
+          )
+        ) services
+      with Not_found ->
+       error (f_"One of rhsrvany.exe or pvvxsvc.exe is missing in %s.  One
of them is required in order to install Windows firstboot scripts.  You can get
one by building rhsrvany (https://github.com/rwmjones/rhsrvany)")
+         virt_tools_data_dir
+    ) in
 
     (* Create a directory for firstboot files in the guest. *)
     let firstboot_dir, firstboot_dir_win @@ -215,8 +223,8 @@ module Windows =
struct
 
     g#mkdir_p (firstboot_dir // "scripts");
 
-    (* Copy rhsrvany to the guest. *)
-    g#upload rhsrvany_exe (firstboot_dir // "rhsrvany.exe");
+    (* Copy pvvxsvc or rhsrvany to the guest. *)
+    g#upload (virt_tools_data_dir // srvany) (firstboot_dir // srvany);
 
     (* Write a firstboot.bat control script which just runs the other
      * scripts in the directory.  Note we need to use CRLF line endings
@@ -252,8 +260,8 @@ for %%%%f in (\"%%scripts%%\"\\*.bat) do (
 
 echo uninstalling firstboot service
 rmdir /S /Q \"%%scripts_done%%\"
-rhsrvany.exe -s firstboot uninstall
-" firstboot_dir_win in
+%s -s firstboot uninstall
+" firstboot_dir_win srvany in
 
     g#write (firstboot_dir // "firstboot.bat") (unix2dos
firstboot_script);
 
@@ -282,7 +290,7 @@ rhsrvany.exe -s firstboot uninstall
         "Start", REG_DWORD 0x2_l;
         "ErrorControl", REG_DWORD 0x1_l;
         "ImagePath",
-          REG_SZ (firstboot_dir_win ^ "\\rhsrvany.exe -s firstboot");
+          REG_SZ (sprintf "%s\\%s -s firstboot" firstboot_dir_win
srvany);
         "DisplayName", REG_SZ "Virt tools firstboot
service";
         "ObjectName", REG_SZ "LocalSystem" ];
 
diff --git a/customize/virt-customize.pod b/customize/virt-customize.pod
index 8fb9931..7654fee 100644
--- a/customize/virt-customize.pod
+++ b/customize/virt-customize.pod
@@ -250,6 +250,12 @@ I<--firstboot> or I<--firstboot-command>
options with Windows guests.
 
 See also: C<https://github.com/rwmjones/rhsrvany>
 
+=item F<pvvxsvc.exe>
+
+This is a Windows binary shipped with SUSE VMDP, used to install a
"firstboot"
+script in Windows guests.  It is required if you intend to use the
+I<--firstboot> or I<--firstboot-command> options with Windows
guests.
+
 =back
 
 =back
diff --git a/sysprep/virt-sysprep.pod b/sysprep/virt-sysprep.pod
index 4bbba9a..d86b1e4 100644
--- a/sysprep/virt-sysprep.pod
+++ b/sysprep/virt-sysprep.pod
@@ -550,6 +550,12 @@ I<--firstboot> or I<--firstboot-command>
options with Windows guests.
 
 See also: C<https://github.com/rwmjones/rhsrvany>
 
+=item F<pvvxsvc.exe>
+
+This is a Windows binary shipped with SUSE VMDP, used to install a
"firstboot"
+script in Windows guests.  It is required if you intend to use the
+I<--firstboot> or I<--firstboot-command> options with Windows
guests.
+
 =back
 
 =back
diff --git a/v2v/virt-v2v.pod b/v2v/virt-v2v.pod
index f8d05ee..c2a2ed2 100644
--- a/v2v/virt-v2v.pod
+++ b/v2v/virt-v2v.pod
@@ -1847,6 +1847,12 @@ script in the guest during conversion of Windows guests.
 
 See also: C<https://github.com/rwmjones/rhsrvany>
 
+=item F<pvvxsvc.exe>
+
+This is a Windows binary shipped with SUSE VMDP, used to install a
"firstboot"
+script in Windows guests.  It is required if you intend to use the
+I<--firstboot> or I<--firstboot-command> options with Windows
guests.
+
 =item F<rhev-apt.exe>
 
 (Optional)
-- 
2.6.6

---
 v2v/virt-v2v.pod | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/v2v/virt-v2v.pod b/v2v/virt-v2v.pod
index c2a2ed2..293efeb 100644
--- a/v2v/virt-v2v.pod
+++ b/v2v/virt-v2v.pod
@@ -167,6 +167,8 @@ OVAs from other hypervisors will not work.
 
 =item RHEL 5 Xen
 
+=item SUSE Xen
+
 =item Citrix Xen
 
 Citrix Xen has not been recently tested.
-- 
2.6.6

To add this support, two things are needed:
 * make the existing code searches for either the viostor
   or the SUSE VMDP (Virtual Machine Driver Pack) files.

 * add a firstboot script setting up VMDP.

Note that 2 firstboot scripts are intentionally added for the VMDP
setup. This is due to windows potentially rebooting after loading the
virtio block driver. It may happen that this reboot interrupts the VMDP
setup in the firstboot script, we thus make sure the setup is run a
second time in case it needs to finish the previous run.
---
 v2v/convert_windows.ml | 73 +++++++++++++++++++++++++++++++++++++++++---------
 v2v/windows_virtio.ml  | 25 ++++++++++++-----
 2 files changed, 78 insertions(+), 20 deletions(-)

diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
index 5daae6c..ab28636 100644
--- a/v2v/convert_windows.ml
+++ b/v2v/convert_windows.ml
@@ -43,18 +43,25 @@ let convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps      try Sys.getenv "VIRT_TOOLS_DATA_DIR"
     with Not_found -> Guestfs_config.datadir // "virt-tools" in
 
-  (* Check if RHEV-APT exists.  This is optional. *)
-  let rhev_apt_exe = virt_tools_data_dir // "rhev-apt.exe" in
-  let rhev_apt_exe +  (* Check if either RHEV-APT or VMDP exists.  This is
optional. *)
+  let tools = ["rhev-apt.exe"; "vmdp.exe"] in
+  let installer      try
-      let chan = open_in rhev_apt_exe in
-      close_in chan;
-      Some rhev_apt_exe
-    with
-      Sys_error msg ->
-        warning (f_"'%s' is missing.  Unable to install RHEV-APT
(RHEV guest agent).  Original error: %s")
-          rhev_apt_exe msg;
-        None in
+      let tool = List.find (
+        fun item ->
+          try (
+            let exe_path = virt_tools_data_dir // item in
+            let chan = open_in exe_path in
+            close_in chan;
+            true
+          ) with _ ->
+            false
+      ) tools in
+      Some (virt_tools_data_dir // tool)
+    with Not_found -> (
+      warning (f_"Neither rhev-apt.exe nor vmdp.exe can be found.  Unable
to install one of them.");
+      None
+    ) in
 
   (* Get the Windows %systemroot%. *)
   let systemroot = g#inspect_get_windows_systemroot inspect.i_root in
@@ -211,7 +218,14 @@ let convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps    (* Perform the conversion of the Windows guest. *)
 
   let rec configure_firstboot () -    configure_rhev_apt ();
+    match installer with
+    | None -> info (f_"No firstboot installer to configure")
+    | Some installer_path ->
+       let installer_name = Filename.basename installer_path in
+       match installer_name with
+        | "rhev-apt.exe" -> configure_rhev_apt ()
+        | "vmdp.exe" -> configure_vmdp ()
+        | _ -> info (f_"No setup function for installer
'%s'") installer_path;
     unconfigure_xenpv ();
     unconfigure_prltools ()
 
@@ -219,7 +233,7 @@ let convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps      (* Configure RHEV-APT (the RHEV guest agent).  However if it
doesn't
      * exist just warn about it and continue.
      *)
-    match rhev_apt_exe with
+    match installer with
     | None -> ()
     | Some rhev_apt_exe ->
       g#upload rhev_apt_exe "/rhev-apt.exe"; (* XXX *)
@@ -236,6 +250,39 @@ net start rhev-apt
       Firstboot.add_firstboot_script g inspect.i_root
         "configure rhev-apt" fb_script
 
+  and configure_vmdp () +    (* Configure VMDP if possible *)
+    match installer with
+    | None -> ()
+    | Some vmdp_exe ->
+      g#upload vmdp_exe "/vmdp.exe";
+
+      let fb_script = "\
+echo V2V first boot script started
+echo Decompressing VMDP installer
+\"\\vmdp.exe\"
+pushd \"VMDP-*\"
+echo Installing VMDP
+setup.exe /eula_accepted /no_reboot
+popd
+" in
+
+      let fb_recover_script = "\
+echo Finishing VMDP installation
+if not exist VMDP-* (
+  \"\\vmdp.exe\"
+)
+pushd \"VMDP-*\"
+setup.exe /eula_accepted /no_reboot
+popd
+" in
+
+      Firstboot.add_firstboot_script g inspect.i_root
+        "configure vmdp" fb_script;
+
+      Firstboot.add_firstboot_script g inspect.i_root
+        "finish vmdp setup" fb_recover_script
+
   and unconfigure_xenpv ()      match xenpv_uninst with
     | None -> () (* nothing to be uninstalled *)
diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
index 7e9f735..a878a3e 100644
--- a/v2v/windows_virtio.ml
+++ b/v2v/windows_virtio.ml
@@ -66,11 +66,19 @@ let rec install_drivers g inspect systemroot root current_cs
rcaps    else (
     (* Can we install the block driver? *)
     let block : guestcaps_block_type -      let has_viostor = g#exists
(driverdir // "viostor.inf") in
+      let filenames = ["virtio_blk"; "vrtioblk";
"viostor"] in
+      let driver_name = try (
+        List.find (
+          fun driver_file ->
+            let source = driverdir // (driver_file ^ ".sys") in
+            g#exists source
+        ) filenames
+      ) with Not_found -> "" in
+      let has_viostor = not (driver_name = "") in
       let has_vioscsi = g#exists (driverdir // "vioscsi.inf") in
       match rcaps.rcaps_block_bus, has_viostor, has_vioscsi with
       | Some Virtio_blk, false, _ ->
-        error (f_"there is no viostor (virtio block device) driver for
this version of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe
guest will be configured to use a slower emulated device.")
+        error (f_"there is no virtio block device driver for this version
of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe guest will
be configured to use a slower emulated device.")
               inspect.i_major_version inspect.i_minor_version
               inspect.i_arch virtio_win
 
@@ -79,8 +87,9 @@ let rec install_drivers g inspect systemroot root current_cs
rcaps                inspect.i_major_version inspect.i_minor_version
               inspect.i_arch virtio_win
 
+
       | None, false, _ ->
-        warning (f_"there is no viostor (virtio block device) driver for
this version of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe
guest will be configured to use a slower emulated device.")
+        warning (f_"there is no virtio block device driver for this
version of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe
guest will be configured to use a slower emulated device.")
                 inspect.i_major_version inspect.i_minor_version
                 inspect.i_arch virtio_win;
         IDE
@@ -88,11 +97,12 @@ let rec install_drivers g inspect systemroot root current_cs
rcaps        | (Some Virtio_blk | None), true, _ ->
         (* Block driver needs tweaks to allow booting; the rest is set up by
PnP
          * manager *)
-        let source = driverdir // "viostor.sys" in
-        let target = sprintf "%s/system32/drivers/viostor.sys"
systemroot in
+        let source = driverdir // (driver_name ^ ".sys") in
+        let targetdir = systemroot ^ "/system32/drivers/" in
+        let target = targetdir // (driver_name ^ ".sys") in
         let target = g#case_sensitive_path target in
         g#cp source target;
-        add_guestor_to_registry g root current_cs "viostor"
+        add_guestor_to_registry g root current_cs driver_name
                                 viostor_pciid;
         Virtio_blk
 
@@ -112,7 +122,8 @@ let rec install_drivers g inspect systemroot root current_cs
rcaps
     (* Can we install the virtio-net driver? *)
     let net : guestcaps_net_type -      let has_netkvm = g#exists (driverdir //
"netkvm.inf") in
+      let filenames = ["virtio_net.inf"; "netkvm.inf"] in
+      let has_netkvm = List.exists (fun driver_file -> g#exists (driverdir
// driver_file)) filenames in
       match rcaps.rcaps_net_bus, has_netkvm with
       | Some Virtio_net, false ->
         error (f_"there is no virtio network driver for this version of
Windows (%d.%d %s).  virt-v2v looks for this driver in %s")
-- 
2.6.6

Setting spice display if we know the libvirt guest doesn't have QXL
capabilities will only make the guest slower for the user. Fallback
to VNC in such a case.

A typical use case is when v2v-ing a windows guest without having the
windows QXL driver at hand.
---
 v2v/output_libvirt.ml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/v2v/output_libvirt.ml b/v2v/output_libvirt.ml
index bedd6b4..1999600 100644
--- a/v2v/output_libvirt.ml
+++ b/v2v/output_libvirt.ml
@@ -239,7 +239,9 @@ let create_libvirt_xml ?pool source target_buses guestcaps
     | Some { s_display_type = VNC } ->
        e "graphics" [ "type", "vnc" ] []
     | Some { s_display_type = Spice } ->
-       e "graphics" [ "type", "spice" ] [] in
+       match guestcaps.gcaps_video with
+       | QXL ->    e "graphics" [ "type",
"spice" ] []
+       | Cirrus -> e "graphics" [ "type",
"vnc" ] [] in
 
   (match source.s_display with
    | Some { s_keymap = Some km } -> append_attr ("keymap", km)
graphics
-- 
2.6.6

Running the init on openSUSE and SLE machines showed up minor errors:
 * skip the /etc/mtab symlink creation if the file is already existing.
 * make sure /run/lvm is created or lvmetab will complain.
---
 appliance/init | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/appliance/init b/appliance/init
index 413a95f..b22032e 100755
--- a/appliance/init
+++ b/appliance/init
@@ -70,7 +70,9 @@ ln -s ../run/lock /var/lock
 
 # On Fedora 23, util-linux creates /etc/mtab in %post .. stupid
 # and e2fsprogs fails if the link doesn't exist .. stupid stupid
-ln -s /proc/mounts /etc/mtab
+if ! test -e /etc/mtab; then
+  ln -s /proc/mounts /etc/mtab
+fi
 
 # devtmpfs is required since udev 176
 mount -t devtmpfs /dev /dev
@@ -132,6 +134,7 @@ mdadm -As --auto=yes --run
 
 # Scan for LVM.
 modprobe dm_mod ||:
+mkdir -p /run/lvm
 lvmetad ||:
 
 lvm vgchange -aay --sysinit
-- 
2.6.6

In virt-v2v man page the documentation on how to use RHEL 5 Xen as
input is generic enough to fit other Xen versions.
---
 v2v/virt-v2v.pod | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/v2v/virt-v2v.pod b/v2v/virt-v2v.pod
index 293efeb..292c5a3 100644
--- a/v2v/virt-v2v.pod
+++ b/v2v/virt-v2v.pod
@@ -338,7 +338,7 @@ Xen remote connections can be used.  Other remote libvirt
connections
 will not work in general.
 
 See also L</INPUT FROM VMWARE VCENTER SERVER>,
-L</INPUT FROM RHEL 5 XEN> below.
+L</INPUT FROM XEN> below.
 
 =item B<-if> format
 
@@ -1228,9 +1228,10 @@ Perform the conversion of the guest using virt-v2v:
 
 Remove the F<guest.xml> and F<guest-disk*> files.
 
-=head1 INPUT FROM RHEL 5 XEN
+=head1 INPUT FROM XEN
 
-Virt-v2v is able to import Xen guests from RHEL 5 Xen hosts.
+Virt-v2v is able to import Xen guests from RHEL 5 Xen or SLES and
+openSUSE Xen hosts.
 
 Virt-v2v uses libvirt for access to the remote Xen host, and therefore
 the input mode should be I<-i libvirt>.  As this is the default, you
-- 
2.6.6

To make sure we can also find the initrd on openSUSE and SLES, we need two
improvements:
 * the initrd filename may not end with '.img'
 * don't use the version + release from the RPM data, rather from the
   /lib/modules/<version>/ path as we need to find it out anyway.
---
 v2v/convert_linux.ml | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index e5778ef..4d3e628 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -125,7 +125,7 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps    let installed_kernels : kernel_info list      let rex_ko =
Str.regexp ".*\\.k?o\\(\\.xz\\)?$" in
     let rex_ko_extract = Str.regexp
".*/\\([^/]+\\)\\.k?o\\(\\.xz\\)?$" in
-    let rex_initrd = Str.regexp "^initr\\(d\\|amfs\\)-.*\\.img$" in
+    let rex_initrd = Str.regexp
"^initr\\(d\\|amfs\\)-.*\\(\\.img\\)?$" in
     filter_map (
       function
       | { G.app2_name = name } as app
@@ -157,8 +157,8 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps                 try g#statns vmlinuz with G.Error _ -> raise
Not_found in
 
              (* Get/construct the version.  XXX Read this from kernel file. *)
-             let version -               sprintf "%s-%s"
app.G.app2_version app.G.app2_release in
+             let prefix_len = String.length "/lib/modules/" in
+             let version = String.sub modpath prefix_len ((String.length
modpath) - prefix_len) in
 
              (* Find the initramfs which corresponds to the kernel.
               * Since the initramfs is built at runtime, and doesn't have
@@ -173,12 +173,11 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps                 let files                   List.filter (
                    fun n ->
-                     String.find n app.G.app2_version >= 0 &&
-                       String.find n app.G.app2_release >= 0
+                     String.find n version >= 0
                  ) files in
                (* Don't consider kdump initramfs images (RHBZ#1138184). *)
                let files -                 List.filter (fun n -> String.find
n "kdump.img" == -1) files in
+                 List.filter (fun n -> String.find n "kdump" ==
-1) files in
                (* If several files match, take the shortest match.  This
                 * handles the case where we have a mix of same-version non-Xen
                 * and Xen kernels:
-- 
2.6.6

On Thu, May 12, 2016 at 03:29:10PM +0200, Cédric Bosdonnat
wrote:
> SUSE Linux Entreprise Server doesn't have dhcpcd and the hivex package
> is not in the default repositories. Better use dhcp-client and libhivex0
> ---
>  appliance/packagelist.in | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/appliance/packagelist.in b/appliance/packagelist.in
> index daf589e..c221854 100644
> --- a/appliance/packagelist.in
> +++ b/appliance/packagelist.in
> @@ -132,11 +132,11 @@ ifelse(SUSE,1,
>    augeas-lenses
>    btrfsprogs
>    cryptsetup
> -  dhcpcd
> +  dhcp-client
>    genisoimage
>    glibc-locale
>    gptfdisk
> -  hivex
> +  libhivex0
>    iproute2
>    iputils
>    libcap2
Supermin ignores packages that don't exist (ie. aren't installed).  So
it would be OK to just add lines to this file.

It's only correct to remove the lines if:

 - those packages never existed in any former version of SUSE, or

 - those packages did exist before, but not in any version of
   SUSE on which you now want to run libguestfs

Also the lines should be kept in alphabetical order (libhivex0 isn't).

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

On Thu, May 12, 2016 at 03:29:12PM +0200, Cédric Bosdonnat
wrote:
> Rename the C:\Program Files\Red Hat\Firstboot folder into the more vendor
> independent C:\Program Files\Guestfs\Firstboot.
> ---
>  builder/virt-builder.pod | 2 +-
>  customize/firstboot.ml   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/builder/virt-builder.pod b/builder/virt-builder.pod
> index 9a49138..dc36c13 100644
> --- a/builder/virt-builder.pod
> +++ b/builder/virt-builder.pod
> @@ -849,7 +849,7 @@ C<VIRT_TOOLS_DATA_DIR> environment variable; if
not set, a compiled-in
>  default will be used (something like F</usr/share/virt-tools>).
>  
>  The output of the first boot scripts is available in the guest as
> -F<C:\Program Files\Red Hat\Firstboot\log.txt>.
> +F<C:\Program Files\Guestfs\Firstboot\log.txt>.
>  
>  =back
>  
> diff --git a/customize/firstboot.ml b/customize/firstboot.ml
> index 83bd808..4167098 100644
> --- a/customize/firstboot.ml
> +++ b/customize/firstboot.ml
> @@ -211,7 +211,7 @@ module Windows = struct
>            g#mkdir_p firstboot_dir;
>            loop firstboot_dir firstboot_dir_win path
>        in
> -      loop "" "C:" ["Program Files";
"Red Hat"; "Firstboot"] in
> +      loop "" "C:" ["Program Files";
"Guestfs"; "Firstboot"] in
>  
>      g#mkdir_p (firstboot_dir // "scripts");
This patch breaks the virt-v2v tests.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Thu, May 12, 2016 at 03:29:13PM +0200, Cédric Bosdonnat
wrote:
> -    (* rhsrvany.exe must exist.
> +    (* either rhsrvany.exe or pvvxsvc.exe must exist.
Best to capitalize "either" in this comment.
>       *
>       * (Check also that it's not a dangling symlink but a real file).
>       *)
> -    let rhsrvany_exe = virt_tools_data_dir // "rhsrvany.exe" in
> -    (try
> -       let chan = open_in rhsrvany_exe in
> -       close_in chan
> -     with
> -       Sys_error msg ->
> -         error (f_"'%s' is missing.  This file is required in
order to install Windows firstboot scripts.  You can get it by building rhsrvany
(https://github.com/rwmjones/rhsrvany).  Original error: %s")
> -           rhsrvany_exe msg
> -    );
> +    let services = ["rhsrvany.exe"; "pvvxsvc.exe"] in
> +    let srvany = (
You don't need the extra '(' here.
> +          fun service -> (
Nor after the fun .. -> here.
> +            try
> +              let chan = open_in (virt_tools_data_dir // service) in
> +              close_in chan;
> +              true
> +            with _ ->
> +              false
> +          )
> +        ) services
> +      with Not_found ->
> +       error (f_"One of rhsrvany.exe or pvvxsvc.exe is missing in %s.
One of them is required in order to install Windows firstboot scripts.  You can
get one by building rhsrvany (https://github.com/rwmjones/rhsrvany)")
> +         virt_tools_data_dir
> +    ) in
Although it's not written down anywhere, I usually put the `in'
on the preceeding line when defining a non-function, so (without
the unnecessary paren) this becomes:

  +       error (f_"....")
  +         virt_tools_data_dir in

Rest of this commit looks good.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Thu, May 12, 2016 at 03:29:15PM +0200, Cédric Bosdonnat
wrote:
> +  (* Check if either RHEV-APT or VMDP exists.  This is optional. *)
> +  let tools = ["rhev-apt.exe"; "vmdp.exe"] in
Strong typing FTW ...

  let tools = [ `RhevApt, "rhev-apt.exe"; `VmdpExe,
"vmdp.exe" ] in
> +  let installer >      try
> -      let chan = open_in rhev_apt_exe in
> -      close_in chan;
> -      Some rhev_apt_exe
> -    with
> -      Sys_error msg ->
> -        warning (f_"'%s' is missing.  Unable to install
RHEV-APT (RHEV guest agent).  Original error: %s")
> -          rhev_apt_exe msg;
> -        None in
> +      let tool = List.find (
> +        fun item ->
  let t, tool = List.find (
    fun (_, tool) ->
> +          try (
> +            let exe_path = virt_tools_data_dir // item in
      let exe_path = virt_tools_data_dir // tool in
> +            let chan = open_in exe_path in
> +            close_in chan;
> +            true
> +          ) with _ ->
> +            false
> +      ) tools in
> +      Some (virt_tools_data_dir // tool)
  Some (t, virt_tools_data_dir // tool)
> +    with Not_found -> (
> +      warning (f_"Neither rhev-apt.exe nor vmdp.exe can be found. 
Unable to install one of them.");
> +      None
> +    ) in
>  
>    (* Get the Windows %systemroot%. *)
>    let systemroot = g#inspect_get_windows_systemroot inspect.i_root in
> @@ -211,7 +218,14 @@ let convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps >    (* Perform the conversion of the Windows guest. *)
>  
>    let rec configure_firstboot () > -    configure_rhev_apt ();
> +    match installer with
> +    | None -> info (f_"No firstboot installer to configure")
No need to print anything here.  We've already printed a
warning earlier.
> +    | Some installer_path ->
> +       let installer_name = Filename.basename installer_path in
> +       match installer_name with
> +        | "rhev-apt.exe" -> configure_rhev_apt ()
> +        | "vmdp.exe" -> configure_vmdp ()
> +        | _ -> info (f_"No setup function for installer
'%s'") installer_path;
   | Some (`RhevApt, tool_path) ->
       configure_rhev_apt tool_path
   | Some (`VmdpExe, tool_path) ->
       configure_vmdp tool_path

You'll have to make the obvious adjustments to configure_rhev_apt and
configure_vmdp.
> diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
> index 7e9f735..a878a3e 100644
> --- a/v2v/windows_virtio.ml
> +++ b/v2v/windows_virtio.ml
> @@ -66,11 +66,19 @@ let rec install_drivers g inspect systemroot root
current_cs rcaps >    else (
>      (* Can we install the block driver? *)
>      let block : guestcaps_block_type > -      let has_viostor =
g#exists (driverdir // "viostor.inf") in
> +      let filenames = ["virtio_blk"; "vrtioblk";
"viostor"] in
> +      let driver_name = try (
> +        List.find (
> +          fun driver_file ->
> +            let source = driverdir // (driver_file ^ ".sys") in
You don't need the extra parens here     ^                    ^ .
> +            g#exists source
> +        ) filenames
> +      ) with Not_found -> "" in
> +      let has_viostor = not (driver_name = "") in
>        let has_vioscsi = g#exists (driverdir // "vioscsi.inf") in
>        match rcaps.rcaps_block_bus, has_viostor, has_vioscsi with
>        | Some Virtio_blk, false, _ ->
> -        error (f_"there is no viostor (virtio block device) driver
for this version of Windows (%d.%d %s).  virt-v2v looks for this driver in
%s\n\nThe guest will be configured to use a slower emulated device.")
> +        error (f_"there is no virtio block device driver for this
version of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe
guest will be configured to use a slower emulated device.")
>                inspect.i_major_version inspect.i_minor_version
>                inspect.i_arch virtio_win
>  
> @@ -79,8 +87,9 @@ let rec install_drivers g inspect systemroot root
current_cs rcaps >                inspect.i_major_version
inspect.i_minor_version
>                inspect.i_arch virtio_win
>  
> +
Seem to have added a blank line here.
>        | None, false, _ ->
> -        warning (f_"there is no viostor (virtio block device) driver
for this version of Windows (%d.%d %s).  virt-v2v looks for this driver in
%s\n\nThe guest will be configured to use a slower emulated device.")
> +        warning (f_"there is no virtio block device driver for this
version of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe
guest will be configured to use a slower emulated device.")
>                  inspect.i_major_version inspect.i_minor_version
>                  inspect.i_arch virtio_win;
>          IDE
> @@ -88,11 +97,12 @@ let rec install_drivers g inspect systemroot root
current_cs rcaps >        | (Some Virtio_blk | None), true, _ ->
>          (* Block driver needs tweaks to allow booting; the rest is set up
by PnP
>           * manager *)
> -        let source = driverdir // "viostor.sys" in
> -        let target = sprintf "%s/system32/drivers/viostor.sys"
systemroot in
> +        let source = driverdir // (driver_name ^ ".sys") in
> +        let targetdir = systemroot ^ "/system32/drivers/" in
> +        let target = targetdir // (driver_name ^ ".sys") in
What was wrong with using sprintf to construct the target?
>          let target = g#case_sensitive_path target in
>          g#cp source target;
> -        add_guestor_to_registry g root current_cs "viostor"
> +        add_guestor_to_registry g root current_cs driver_name
>                                  viostor_pciid;
>          Virtio_blk
>  
> @@ -112,7 +122,8 @@ let rec install_drivers g inspect systemroot root
current_cs rcaps >
>      (* Can we install the virtio-net driver? *)
>      let net : guestcaps_net_type > -      let has_netkvm = g#exists
(driverdir // "netkvm.inf") in
> +      let filenames = ["virtio_net.inf"; "netkvm.inf"]
in
> +      let has_netkvm = List.exists (fun driver_file -> g#exists
(driverdir // driver_file)) filenames in
This line is a bit too long.
>        match rcaps.rcaps_net_bus, has_netkvm with
>        | Some Virtio_net, false ->
>          error (f_"there is no virtio network driver for this version
of Windows (%d.%d %s).  virt-v2v looks for this driver in %s")
> -- 
> 2.6.6
Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Thu, May 12, 2016 at 03:29:19PM +0200, Cédric Bosdonnat
wrote:
> To make sure we can also find the initrd on openSUSE and SLES, we need two
improvements:
>  * the initrd filename may not end with '.img'
>  * don't use the version + release from the RPM data, rather from the
>    /lib/modules/<version>/ path as we need to find it out anyway.
> ---
>  v2v/convert_linux.ml | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
> index e5778ef..4d3e628 100644
> --- a/v2v/convert_linux.ml
> +++ b/v2v/convert_linux.ml
> @@ -125,7 +125,7 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps >    let installed_kernels : kernel_info list >     
let rex_ko = Str.regexp ".*\\.k?o\\(\\.xz\\)?$" in
>      let rex_ko_extract = Str.regexp
".*/\\([^/]+\\)\\.k?o\\(\\.xz\\)?$" in
> -    let rex_initrd = Str.regexp "^initr\\(d\\|amfs\\)-.*\\.img$"
in
> +    let rex_initrd = Str.regexp
"^initr\\(d\\|amfs\\)-.*\\(\\.img\\)?$" in
>      filter_map (
>        function
>        | { G.app2_name = name } as app
> @@ -157,8 +157,8 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps >                 try g#statns vmlinuz with G.Error _
-> raise Not_found in
>  
>               (* Get/construct the version.  XXX Read this from kernel
file. *)
> -             let version > -               sprintf "%s-%s"
app.G.app2_version app.G.app2_release in
> +             let prefix_len = String.length "/lib/modules/" in
> +             let version = String.sub modpath prefix_len ((String.length
modpath) - prefix_len) in
Too many parentheses here.  Function application always has the
highest precedence in functional languages.

Also you can nest variables to make them private.

Thus this becomes:

  let version     let prefix_len = String.length "/lib/modules/" in
    String.sub modpath prefix_len (String.length modpath - prefix_len) in

- - -

The patches that I _didn't_ comment on all look OK to me.

I'd like to see the whole series posted again with everything
fixed though.

Thanks, 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

On Thursday 12 May 2016 15:29:09 Cédric Bosdonnat wrote:
> To allow saving save in the package shipping the windows virtio
s/save/space/, I guess?

-- 
Pino Toscano

On Thursday 12 May 2016 15:29:10 Cédric Bosdonnat wrote:
> diff --git a/appliance/packagelist.in b/appliance/packagelist.in
> index daf589e..c221854 100644
> --- a/appliance/packagelist.in
> +++ b/appliance/packagelist.in
> @@ -132,11 +132,11 @@ ifelse(SUSE,1,
>    gptfdisk
> -  hivex
> +  libhivex0
It seems like openSUSE 13.2 has libhivex0 as well, so this change
LGTM.

Thanks,
-- 
Pino Toscano

Hi Roman,

On Thu, 2016-05-12 at 17:57 +0300, Roman Kagan wrote:
> On Thu, May 12, 2016 at 03:29:15PM +0200, Cédric Bosdonnat wrote:
> > To add this support, two things are needed:
> >  * make the existing code searches for either the viostor
> >    or the SUSE VMDP (Virtual Machine Driver Pack) files.
> > 
> >  * add a firstboot script setting up VMDP.
> > 
> > Note that 2 firstboot scripts are intentionally added for the VMDP
> > setup. This is due to windows potentially rebooting after loading
> > the
> > virtio block driver. It may happen that this reboot interrupts the
> > VMDP
> > setup in the firstboot script, we thus make sure the setup is run a
> > second time in case it needs to finish the previous run.
> 
> Have you been able to identify the reason of this reboot?  Isn't it
> triggered by the PnP manager?
> 
> I guess it is, and, since we copy over the drivers with all the stuff
> to
> properly install them, the PnP manager will do so completely
> unsynchronized to our firstboot activity, so all scripts are in
> danger.
> 
> I think this should better be resolved generically, by not running
> the
> scripts until PnP completes (that includes reboots).
I agree, but so far I couldn't find a way to be sure that firstboot
executes after all potential reboots. Even when playing with the
service dependencies I still couldn't find a working solution.

Using the function you mentioned the other day could be an option, but
that has to be implemented in the pvvxsvc or rhsrvany tool, and there
is a restriction to it: it can't be run from the service main callback.

The solution I found here with my colleague working on VMDP is a
compromise to get things right at least for VMDP setup.
> > @@ -66,11 +66,19 @@ let rec install_drivers g inspect systemroot
> > root current_cs rcaps > >    else (
> >      (* Can we install the block driver? *)
> >      let block : guestcaps_block_type > > -      let has_viostor
= g#exists (driverdir // "viostor.inf") in
> > +      let filenames = ["virtio_blk"; "vrtioblk";
"viostor"] in
> > +      let driver_name = try (
> > +        List.find (
> > +          fun driver_file ->
> > +            let source = driverdir // (driver_file ^
".sys") in
> > +            g#exists source
> > +        ) filenames
> > +      ) with Not_found -> "" in
> > +      let has_viostor = not (driver_name = "") in
> 
> So you have somehow managed to decouple virtio_blk from the balloon
> driver, haven't you?
Yes. Older versions of the VMDP drivers didn't have those coupled. And
newer (not yet released) won't have it too. I'm just avoiding the
latest VMDP drivers in v2v for this reason ;)

--
Cedric

On Thu, 2016-05-12 at 17:24 +0300, Roman Kagan wrote:
> On Thu, May 12, 2016 at 03:29:11PM +0200, Cédric Bosdonnat wrote:
> > Sync the windows firstboot script with its linux brother. Also
> > change
> > the main redirection to append to the log rather than overwriting
> > it.
> > With this change, the firstboot script will resist reboots in the
> > executed scripts.
> > ---
> >  customize/firstboot.ml | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/customize/firstboot.ml b/customize/firstboot.ml
> > index aa5b694..83bd808 100644
> > --- a/customize/firstboot.ml
> > +++ b/customize/firstboot.ml
> > @@ -232,7 +232,7 @@ set log=%%firstboot%%\\log.txt
> >  set scripts=%%firstboot%%\\scripts
> >  set scripts_done=%%firstboot%%\\scripts-done
> >  
> > -call :main > \"%%log%%\" 2>&1
> > +call :main >> \"%%log%%\" 2>&1
> >  exit /b
> >  
> >  :main
> > @@ -244,15 +244,14 @@ if not exist \"%%scripts_done%%\" (
> >  
> >  for %%%%f in (\"%%scripts%%\"\\*.bat) do (
> >    echo running \"%%%%f\"
> > -  call \"%%%%f\"
> > -  set elvl=!errorlevel!
> > -  echo .... exit code !elvl!
> > -  if !elvl! equ 0 (
> > -    move \"%%%%f\" \"%%scripts_done%%\"
> > -  )
> > +  move \"%%%%f\" \"%%scripts_done%%\"
> > +  pushd \"%%scripts_done%%\"
> > +  call \"%%%%~nf\"
> > +  popd
> >  )
> >  
> >  echo uninstalling firstboot service
> > +rmdir /S /Q \"%%scripts_done%%\"
> 
> How will you debug failures then?
> (And yes, linux firstboot is broken in this regard, too)
Good question! But I think this should be fixed in both linux and
windows cases in another commit. This one is only catching up what we
have in the linux case.

For the debugging purpose, couldn't we delete the successfully run
scripts one by one after they have run?

--
Cedric
> >  rhsrvany.exe -s firstboot uninstall
> >  " firstboot_dir_win in
> >

On Fri, 2016-05-13 at 10:36 +0300, Roman Kagan wrote:
> IMO deleting anything is a bad idea, in particular, because a
> succefully
> completed script may cause the following ones to fail.
Well, I guess most of the users won't be aware of it and scripts will
probably stay there for ever.
> The whole point of scripts_done is to preserve all the steps but move
> them out of the way.
> 
> I think the only controversial item is whether to move them to
> scripts_done on success only or regardless of the status: there are
> pros
> and cons either way.
The goal of moving the script to scripts_done BEFORE running was to
avoid running them twice if the system reboots in the middle of the
firstboot script.

--
Cedric

On Friday 13 May 2016 10:36:10 Roman Kagan wrote:
> On Fri, May 13, 2016 at 09:17:40AM +0200, Cedric Bosdonnat wrote:
> > On Thu, 2016-05-12 at 17:24 +0300, Roman Kagan wrote:
> > > On Thu, May 12, 2016 at 03:29:11PM +0200, Cédric Bosdonnat wrote:
> > > > @@ -244,15 +244,14 @@ if not exist
\"%%scripts_done%%\" (
> > > >  
> > > >  for %%%%f in (\"%%scripts%%\"\\*.bat) do (
> > > >    echo running \"%%%%f\"
> > > > -  call \"%%%%f\"
> > > > -  set elvl=!errorlevel!
> > > > -  echo .... exit code !elvl!
> > > > -  if !elvl! equ 0 (
> > > > -    move \"%%%%f\" \"%%scripts_done%%\"
> > > > -  )
> > > > +  move \"%%%%f\" \"%%scripts_done%%\"
> > > > +  pushd \"%%scripts_done%%\"
> > > > +  call \"%%%%~nf\"
> > > > +  popd
> > > >  )
> > > >  
> > > >  echo uninstalling firstboot service
> > > > +rmdir /S /Q \"%%scripts_done%%\"
> > > 
> > > How will you debug failures then?
> > > (And yes, linux firstboot is broken in this regard, too)
> > 
> > Good question! But I think this should be fixed in both linux and
> > windows cases in another commit. This one is only catching up what we
> > have in the linux case.
> > 
> > For the debugging purpose, couldn't we delete the successfully run
> > scripts one by one after they have run?
> 
> IMO deleting anything is a bad idea, in particular, because a succefully
> completed script may cause the following ones to fail.
> 
> The whole point of scripts_done is to preserve all the steps but move
> them out of the way.
> 
> I think the only controversial item is whether to move them to
> scripts_done on success only or regardless of the status: there are pros
> and cons either way.
Moving a script to scripts_done only after its execution ended means
that scripts which cause a reboot will be executed over and over. See
rhbz#1159651 and commit f8ed15462fbb03c5b19972361f2a2e6fed4c5f02.

-- 
Pino Toscano

On Thursday 12 May 2016 15:29:15 Cédric Bosdonnat wrote:
> diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
> index 7e9f735..a878a3e 100644
> --- a/v2v/windows_virtio.ml
> +++ b/v2v/windows_virtio.ml
> @@ -66,11 +66,19 @@ let rec install_drivers g inspect systemroot root
current_cs rcaps >    else (
>      (* Can we install the block driver? *)
>      let block : guestcaps_block_type > -      let has_viostor =
g#exists (driverdir // "viostor.inf") in
> +      let filenames = ["virtio_blk"; "vrtioblk";
"viostor"] in
> +      let driver_name = try (
> +        List.find (
> +          fun driver_file ->
> +            let source = driverdir // (driver_file ^ ".sys") in
> +            g#exists source
> +        ) filenames
> +      ) with Not_found -> "" in
> +      let has_viostor = not (driver_name = "") in
Instead of using an empty string for driver_name, use an OCaml option:

let driver_name   try (
    Some (
      List.find (
        ...
      )
    )
  ) with Not_found -> None in

let has_viostor = is_some driver_name in

It is slightly longer, but IMHO clearer to read.

-- 
Pino Toscano

On Thursday 12 May 2016 15:29:17 Cédric Bosdonnat wrote:
> Running the init on openSUSE and SLE machines showed up minor errors:
>  * skip the /etc/mtab symlink creation if the file is already existing.
>  * make sure /run/lvm is created or lvmetab will complain.
> ---
>  appliance/init | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/appliance/init b/appliance/init
> index 413a95f..b22032e 100755
> --- a/appliance/init
> +++ b/appliance/init
> @@ -132,6 +134,7 @@ mdadm -As --auto=yes --run
>  
>  # Scan for LVM.
>  modprobe dm_mod ||:
> +mkdir -p /run/lvm
>  lvmetad ||:
In openSUSE 13.2 I see:

  $ cat /usr/lib/tmpfiles.d/lvm2.conf 
  d /run/lock/lvm 0700 root root -
  d /run/lvm 0700 root root -

which is part of the lvm2 package, thus copied into the appliance.
Later /init runs `systemd-tmpfiles --prefix=/dev --create --boot`...
which is told to handle files under /dev only.  A simple fix would be:
-systemd-tmpfiles --prefix=/dev --create --boot
+systemd-tmpfiles --prefix=/dev --prefix=/run --create --boot

OTOH, I see that /run/lvm (and /run/lvm/lock) are created correctly in
my openSUSE VMs, so could you please provide more detail on the issue
you noticed?

Thanks,
-- 
Pino Toscano