Libguestfs - Feb 2015 - [PATCH 0/4] firstboot: assorted enhancements

This patchset attempts to address a number of shortcomings in the
firstboot infrastructure I came across while working with v2v conversion
of various Windows VMs.

Roman Kagan (4):
  firstboot: consolidate line ending conversion
  firstboot: enhance firstboot driver script for Windows
  firstboot: make script naming descriptive
  convert_windows: split firstboot into steps

 customize/customize_run.ml | 19 +++++-------
 customize/firstboot.ml     | 73 +++++++++++++++++++++++++++++++---------------
 customize/firstboot.mli    | 13 +++++----
 v2v/convert_windows.ml     | 31 ++++++++++----------
 4 files changed, 80 insertions(+), 56 deletions(-)

-- 
2.1.0

This patch moves line ending conversion for windows scripts into a
separate helper function.

This simplifies code a bit, and fixes the problem that actual firstboot
scripts used to remain with unix-style line endings.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
---
 customize/firstboot.ml | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/customize/firstboot.ml b/customize/firstboot.ml
index 23a0432..ffcee12 100644
--- a/customize/firstboot.ml
+++ b/customize/firstboot.ml
@@ -24,6 +24,9 @@ open Common_gettext.Gettext
 open Customize_utils
 open Regedit
 
+let unix2dos s +  String.concat "\r\n" (Str.split_delim
(Str.regexp_string "\n") s)
+
 (* For Linux guests. *)
 module Linux = struct
   let firstboot_dir = "/usr/lib/virt-sysprep"
@@ -207,22 +210,18 @@ module Windows = struct
      * scripts in the directory.  Note we need to use CRLF line endings
      * in this script.
      *)
-    let firstboot_script = [
-      "@echo off";
-      "echo starting firstboot service >>log.txt";
-      (* Notes:
-       * - You have to use double %% inside the batch file, but NOT
-       * when typing the same commands on the command line.
-       * - You have to use 'call' in front of every external command
-       * else it basically exec's the command and never returns.
-       * FFS.
-       *)
-      "for /f %%f in ('dir /b scripts') do call
\"scripts\\%%f\" >>log.txt";
-      "echo uninstalling firstboot service >>log.txt";
-      "rhsrvany.exe -s firstboot uninstall >>log.txt";
-    ] in
-    let firstboot_script = String.concat "\r\n" firstboot_script ^
"\r\n" in
-    g#write (firstboot_dir // "firstboot.bat") firstboot_script;
+    let firstboot_script = "\
+@echo off
+
+echo starting firstboot service >>log.txt
+
+for /f %%f in ('dir /b scripts') do call \"scripts\\%%f\"
>>log.txt
+
+echo uninstalling firstboot service >>log.txt
+rhsrvany.exe -s firstboot uninstall >>log.txt
+" in
+
+    g#write (firstboot_dir // "firstboot.bat") (unix2dos
firstboot_script);
 
     (* Open the SYSTEM hive. *)
     let systemroot = g#inspect_get_windows_systemroot root in
@@ -284,7 +283,7 @@ let add_firstboot_script (g : Guestfs.guestfs) root i
content      let t = Int64.of_float (Unix.time ()) in
     let r = string_random8 () in
     let filename = sprintf "%s/scripts/%04d-%Ld-%s.bat" firstboot_dir
i t r in
-    g#write filename content
+    g#write filename (unix2dos content)
 
   | _ ->
     error (f_"guest type %s/%s is not supported") typ distro
-- 
2.1.0

This patch is an attempt to enhance the firstboot driver script for
Windows, and make it somewhat closer in functionality to what is done
for Linux guests.

Specifically, for every firstboot script it now will log its exit
status, and, if the script reported success, move it to -done directory.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
---
 customize/firstboot.ml | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/customize/firstboot.ml b/customize/firstboot.ml
index ffcee12..f529462 100644
--- a/customize/firstboot.ml
+++ b/customize/firstboot.ml
@@ -210,16 +210,39 @@ module Windows = struct
      * scripts in the directory.  Note we need to use CRLF line endings
      * in this script.
      *)
-    let firstboot_script = "\
+    let firstboot_script = sprintf "\
 @echo off
 
-echo starting firstboot service >>log.txt
+setlocal EnableDelayedExpansion
+set firstboot=%s
+set log=%%firstboot%%\\log.txt
 
-for /f %%f in ('dir /b scripts') do call \"scripts\\%%f\"
>>log.txt
+set scripts=%%firstboot%%\\scripts
+set scripts_done=%%firstboot%%\\scripts-done
 
-echo uninstalling firstboot service >>log.txt
-rhsrvany.exe -s firstboot uninstall >>log.txt
-" in
+call :main > \"%%log%%\" 2>&1
+exit /b
+
+:main
+echo starting firstboot service
+
+if not exist \"%%scripts_done%%\" (
+  mkdir \"%%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%%\"
+  )
+)
+
+echo uninstalling firstboot service
+rhsrvany.exe -s firstboot uninstall
+" firstboot_dir_win in
 
     g#write (firstboot_dir // "firstboot.bat") (unix2dos
firstboot_script);
 
-- 
2.1.0

The firstboot infrastructure used to give the firstboot scripts some
cryptic names which were impossible to relate to the actions they were
supposed to take.

This patch reworks the scheme such that the caller registering a
firstboot script has to provide a descriptive name for the action.  That
name, with non-alphanumeric characters replaced with dashes, prefixed by
the serial number, is then used as the name of the script, e.g.

  0004-install-gcc

or

  0002-msiexec--i-foo-msi.bat

OTOH the numbering becomes internal to the API, i.e. the scripts are
numbered and executed in the order they are registered.

This greatly facilitates debugging and troubleshooting in case when
there are multiple firstboot scripts.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
---
 customize/customize_run.ml | 19 +++++++------------
 customize/firstboot.ml     | 17 ++++++++++-------
 customize/firstboot.mli    | 13 ++++++++-----
 v2v/convert_windows.ml     |  2 +-
 4 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/customize/customize_run.ml b/customize/customize_run.ml
index 5cc6a6e..980df7e 100644
--- a/customize/customize_run.ml
+++ b/customize/customize_run.ml
@@ -143,9 +143,6 @@ exec >>%s 2>&1
   if not (Random_seed.set_random_seed g root) then
     warning (f_"random seed could not be set for this type of
guest");
 
-  (* Used for numbering firstboot commands. *)
-  let i = ref 0 in
-
   (* Store the passwords and set them all at the end. *)
   let passwords = Hashtbl.create 13 in
   let set_password user pw @@ -191,22 +188,20 @@ exec >>%s 2>&1
       Perl_edit.edit_file ~verbose g#ocaml_handle path expr
 
     | `FirstbootCommand cmd ->
-      incr i;
-      msg (f_"Installing firstboot command: [%d] %s") !i cmd;
-      Firstboot.add_firstboot_script g root !i cmd
+      msg (f_"Installing firstboot command: %s") cmd;
+      Firstboot.add_firstboot_script g root cmd cmd
 
     | `FirstbootPackages pkgs ->
-      incr i;
-      msg (f_"Installing firstboot packages: [%d] %s") !i
+      msg (f_"Installing firstboot packages: %s")
         (String.concat " " pkgs);
       let cmd = guest_install_command pkgs in
-      Firstboot.add_firstboot_script g root !i cmd
+      let name = String.concat " " ("install" :: pkgs) in
+      Firstboot.add_firstboot_script g root name cmd
 
     | `FirstbootScript script ->
-      incr i;
-      msg (f_"Installing firstboot script: [%d] %s") !i script;
+      msg (f_"Installing firstboot script: %s") script;
       let cmd = read_whole_file script in
-      Firstboot.add_firstboot_script g root !i cmd
+      Firstboot.add_firstboot_script g root script cmd
 
     | `Hostname hostname ->
       msg (f_"Setting the hostname: %s") hostname;
diff --git a/customize/firstboot.ml b/customize/firstboot.ml
index f529462..b12d616 100644
--- a/customize/firstboot.ml
+++ b/customize/firstboot.ml
@@ -27,6 +27,9 @@ open Regedit
 let unix2dos s    String.concat "\r\n" (Str.split_delim
(Str.regexp_string "\n") s)
 
+let sanitize_name n +  Str.global_replace (Str.regexp
"[^A-Za-z0-9_]") "-" n
+
 (* For Linux guests. *)
 module Linux = struct
   let firstboot_dir = "/usr/lib/virt-sysprep"
@@ -289,23 +292,23 @@ rhsrvany.exe -s firstboot uninstall
 
 end
 
-let add_firstboot_script (g : Guestfs.guestfs) root i content +let script_count
= ref 0
+
+let add_firstboot_script (g : Guestfs.guestfs) root name content    let typ =
g#inspect_get_type root in
   let distro = g#inspect_get_distro root in
+  incr script_count;
+  let filename = sprintf "%04d-%s" !script_count (sanitize_name name)
in
   match typ, distro with
   | "linux", _ ->
     Linux.install_service g distro;
-    let t = Int64.of_float (Unix.time ()) in
-    let r = string_random8 () in
-    let filename = sprintf "%s/scripts/%04d-%Ld-%s"
Linux.firstboot_dir i t r in
+    let filename = Linux.firstboot_dir // "scripts" // filename in
     g#write filename content;
     g#chmod 0o755 filename
 
   | "windows", _ ->
     let firstboot_dir = Windows.install_service g root in
-    let t = Int64.of_float (Unix.time ()) in
-    let r = string_random8 () in
-    let filename = sprintf "%s/scripts/%04d-%Ld-%s.bat" firstboot_dir
i t r in
+    let filename = firstboot_dir // "scripts" // filename ^
".bat" in
     g#write filename (unix2dos content)
 
   | _ ->
diff --git a/customize/firstboot.mli b/customize/firstboot.mli
index 5f2fae5..d7556e7 100644
--- a/customize/firstboot.mli
+++ b/customize/firstboot.mli
@@ -16,13 +16,16 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  *)
 
-val add_firstboot_script : Guestfs.guestfs -> string -> int -> string
-> unit
-  (** [add_firstboot_script g root idx content] adds a firstboot
-      script called [shortname] containing [content].
+val add_firstboot_script : Guestfs.guestfs -> string -> string ->
string -> unit
+  (** [add_firstboot_script g root name content] adds a firstboot
+      script called [name] containing [content].
 
-      NB. [content] is the contents of the script, {b not} a filename.
+      [content] is the contents of the script, {b not} a filename.
 
-      The scripts run in index ([idx]) order.
+      The actual name of the script on the guest filesystem is made of [name]
+      with all characters but alphanumeric replaced with dashes.
+
+      The scripts are run in the order they are registered.
 
       For Linux guests using SELinux you should make sure the
       filesystem is relabelled after calling this. *)
diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
index 4e60e31..a3c7152 100644
--- a/v2v/convert_windows.ml
+++ b/v2v/convert_windows.ml
@@ -148,7 +148,7 @@ let convert ~verbose ~keep_serial_console (g : G.guestfs)
inspect source
     (* Write the completed script to the guest. *)
     let firstboot_script = Buffer.contents fb in
-    Firstboot.add_firstboot_script g inspect.i_root 1 firstboot_script
+    Firstboot.add_firstboot_script g inspect.i_root "firstboot"
firstboot_script
 
   and configure_rhev_apt fb      (* Configure RHEV-APT (the RHEV guest agent). 
However if it doesn't
-- 
2.1.0

Instead of doing all firstboot actions in a single script, take the
advantage of the firstboot infrastructure and store and run unrelated
actions as individual steps.

This facilitates troubleshooting and fault recovery; besides it makes
adding more actions easier.

Signed-off-by: Roman Kagan <rkagan@parallels.com>
---
 v2v/convert_windows.ml | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
index a3c7152..0cd818a 100644
--- a/v2v/convert_windows.ml
+++ b/v2v/convert_windows.ml
@@ -140,17 +140,10 @@ let convert ~verbose ~keep_serial_console (g : G.guestfs)
inspect source    (* Perform the conversion of the Windows guest. *)
 
   let rec configure_firstboot () -    let fb = Buffer.create 1024 in
-    bprintf fb "@echo off\n";
+    configure_rhev_apt ();
+    unconfigure_xenpv ()
 
-    configure_rhev_apt fb;
-    unconfigure_xenpv fb;
-
-    (* Write the completed script to the guest. *)
-    let firstboot_script = Buffer.contents fb in
-    Firstboot.add_firstboot_script g inspect.i_root "firstboot"
firstboot_script
-
-  and configure_rhev_apt fb +  and configure_rhev_apt ()      (* Configure
RHEV-APT (the RHEV guest agent).  However if it doesn't
      * exist just warn about it and continue.
      *)
@@ -159,22 +152,30 @@ let convert ~verbose ~keep_serial_console (g : G.guestfs)
inspect source      | Some rhev_apt_exe ->
       g#upload rhev_apt_exe "/rhev-apt.exe"; (* XXX *)
 
-      bprintf fb "\
+      let fb_script = "\
+@echo off
+
 echo installing rhev-apt
 \"\\rhev-apt.exe\" /S /v /qn
 
 echo starting rhev-apt
 net start rhev-apt
-"
+" in
+      Firstboot.add_firstboot_script g inspect.i_root
+        "configure rhev-apt" fb_script
 
-  and unconfigure_xenpv fb +  and unconfigure_xenpv ()      match xenpv_uninst
with
     | None -> () (* nothing to be uninstalled *)
     | Some uninst ->
-      bprintf fb "\
+      let fb_script = sprintf "\
+@echo off
+
 echo uninstalling Xen PV driver
 \"%s\"
-" uninst
+" uninst in
+      Firstboot.add_firstboot_script g inspect.i_root
+        "uninstall Xen PV" fb_script
   in
 
   let rec update_system_hive root -- 
2.1.0

It all looks sensible, ACK series.

I will push this shortly, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html