Libguestfs - Dec 2018 - [supermin PATCH v2 0/3] Better handle no config files

This is a "merge" of two previous series:
https://www.redhat.com/archives/libguestfs/2018-December/msg00015.html
https://www.redhat.com/archives/libguestfs/2018-December/msg00020.html

The goal is to handle better situations like:
- empty file in the appliance directory
- no config files available in the packages to include in an appliance

Compared to the two series, the changes are:
- patch #1 has a mention of a reproducer in the commit message
- patch #3 changed approach, from "create really empty" to "not
create
  at all"

Pino Toscano (3):
  build: ignore empty files
  prepare: keep config_files available for longer
  prepare: do not create base.tar.gz with no config files

 src/mode_build.ml   |  7 +++-
 src/mode_prepare.ml | 83 +++++++++++++++++++++++++--------------------
 2 files changed, 52 insertions(+), 38 deletions(-)

-- 
2.17.2

Do not error out on empty files, just ignore them.

This can be easily reproduced by touch'ing an empty file in the
directory of an appliance created with --prepare.
---
 src/mode_build.ml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/mode_build.ml b/src/mode_build.ml
index b5f5fa6..8a06012 100644
--- a/src/mode_build.ml
+++ b/src/mode_build.ml
@@ -46,6 +46,7 @@ and file_content  | Packages
 | Hostfiles
 | Excludefiles
+| Empty
 
 let rec string_of_file_type = function
   | GZip c -> sprintf "gzip %s" (string_of_file_content c)
@@ -56,6 +57,7 @@ and string_of_file_content = function
   | Packages -> "packages"
   | Hostfiles -> "hostfiles"
   | Excludefiles -> "excludefiles"
+  | Empty -> "empty"
 
 let rec build debug
     (copy_kernel, format, host_cpu,
@@ -250,6 +252,8 @@ and read_appliance debug basedir appliance = function
     (* Depending on the file type, read or unpack the file. *)
     let appliance        match file_type with
+      | Uncompressed Empty | GZip Empty | XZ Empty ->
+        appliance
       | Uncompressed ((Packages|Hostfiles|Excludefiles) as t) ->
         let chan = open_in file in
         let lines = input_all_lines chan in
@@ -294,7 +298,7 @@ and update_appliance appliance lines = function
         String.sub path 1 (n-1)
     ) lines in
     { appliance with excludefiles = appliance.excludefiles @ lines }
-  | Base_image -> assert false
+  | Base_image | Empty -> assert false
 
 (* Determine the [file_type] of [file], or exit with an error. *)
 and get_file_type file @@ -331,6 +335,7 @@ and get_file_content file buf len   
else if len >= 2 && buf.[0] = '/' then Hostfiles
   else if len >= 2 && buf.[0] = '-' then Excludefiles
   else if len >= 1 && isalnum buf.[0] then Packages
+  else if len = 0 then Empty
   else error "%s: unknown file type in supermin directory" file
 
 and get_compressed_file_content zcat file -- 
2.17.2

This is just refactoring, with no behaviour changes.
---
 src/mode_prepare.ml | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/src/mode_prepare.ml b/src/mode_prepare.ml
index 7c8221e..7759c58 100644
--- a/src/mode_prepare.ml
+++ b/src/mode_prepare.ml
@@ -128,28 +128,28 @@ let prepare debug (copy_kernel, format, host_cpu,
    * be missing either from the package or from the filesystem (the
    * latter case with --use-installed).
    *)
-  let files_from -    let config_files -      List.map (
-        fun (_, files) ->
-          filter_map (
-            function
-            | { ft_config = true; ft_path = path } -> Some path
-            | { ft_config = false } -> None
-          ) files
-      ) packages in
-    let config_files = List.flatten config_files in
-
-    let config_files = List.filter (
-      fun path ->
-        try close_in (open_in (dir // path)); true
-        with Sys_error _ -> false
-    ) config_files in
+  let config_files +    List.map (
+      fun (_, files) ->
+        filter_map (
+          function
+          | { ft_config = true; ft_path = path } -> Some path
+          | { ft_config = false } -> None
+        ) files
+    ) packages in
+  let config_files = List.flatten config_files in
+
+  let config_files = List.filter (
+    fun path ->
+      try close_in (open_in (dir // path)); true
+      with Sys_error _ -> false
+  ) config_files in
 
-    if debug >= 1 then
-      printf "supermin: there are %d config files\n"
-             (List.length config_files);
+  if debug >= 1 then
+    printf "supermin: there are %d config files\n"
+           (List.length config_files);
 
+  let files_from      (* Put the list of config files into a file, for tar to
read. *)
     let files_from = tmpdir // "files-from.txt" in
     let chan = open_out files_from in
-- 
2.17.2

tar defaults to 10K as block size, and thus it pads the created archives
to multiples of that size using zero's.  Hence, when creating an archive
with no files, it will be 10K zero's, that is compressed by gzip,
resulting in few bytes.  The issue happens later, during the build
phase: base.tar.gz is correctly detected as gz, and zcat is run to
detect its content: since the empty tar was 10K of zero's, the buffer in
get_compressed_file_content will be filled by zero's, and
get_file_content will fail to detect anything.

As solution, at least for our own base.tar.gz: in case there are no
config files to copy, do not create base.tar.gz at all.
---
 src/mode_prepare.ml | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/src/mode_prepare.ml b/src/mode_prepare.ml
index 7759c58..70f9dd4 100644
--- a/src/mode_prepare.ml
+++ b/src/mode_prepare.ml
@@ -149,20 +149,29 @@ let prepare debug (copy_kernel, format, host_cpu,
     printf "supermin: there are %d config files\n"
            (List.length config_files);
 
-  let files_from -    (* Put the list of config files into a file, for tar to
read. *)
-    let files_from = tmpdir // "files-from.txt" in
-    let chan = open_out files_from in
-    List.iter (fprintf chan ".%s\n") config_files; (*
"./filename" *)
-    close_out chan;
-
-    files_from in
-
-  (* Write base.tar.gz. *)
-  let base = outputdir // "base.tar.gz" in
-  if debug >= 1 then printf "supermin: writing %s\n%!" base;
-  let cmd -    sprintf "tar%s -C %s -zcf %s -T %s"
-            (if debug >=1 then " -v" else "")
-            (quote dir) (quote base) (quote files_from) in
-  run_command cmd;
+  if config_files <> [] then (
+    (* There are config files to copy, so create the list with them,
+     * and then compress them with tar.
+     *)
+    let files_from +      (* Put the list of config files into a file, for tar
to read. *)
+      let files_from = tmpdir // "files-from.txt" in
+      let chan = open_out files_from in
+      List.iter (fprintf chan ".%s\n") config_files; (*
"./filename" *)
+      close_out chan;
+
+      files_from in
+
+    (* Write base.tar.gz. *)
+    let base = outputdir // "base.tar.gz" in
+    if debug >= 1 then printf "supermin: writing %s\n%!" base;
+    let cmd +      sprintf "tar%s -C %s -zcf %s -T %s"
+              (if debug >=1 then " -v" else "")
+              (quote dir) (quote base) (quote files_from) in
+    run_command cmd;
+  )
+  else (
+    (* No config files to copy, so do not create base.tar.gz. *)
+    if debug >= 1 then printf "supermin: not creating
base.tar.gz\n%!";
+  )
-- 
2.17.2

On Mon, Dec 03, 2018 at 06:31:12PM +0100, Pino Toscano
wrote:
> This is a "merge" of two previous series:
> https://www.redhat.com/archives/libguestfs/2018-December/msg00015.html
> https://www.redhat.com/archives/libguestfs/2018-December/msg00020.html
> 
> The goal is to handle better situations like:
> - empty file in the appliance directory
> - no config files available in the packages to include in an appliance
> 
> Compared to the two series, the changes are:
> - patch #1 has a mention of a reproducer in the commit message
> - patch #3 changed approach, from "create really empty" to
"not create
>   at all"
> 
> Pino Toscano (3):
>   build: ignore empty files
>   prepare: keep config_files available for longer
>   prepare: do not create base.tar.gz with no config files
> 
>  src/mode_build.ml   |  7 +++-
>  src/mode_prepare.ml | 83 +++++++++++++++++++++++++--------------------
>  2 files changed, 52 insertions(+), 38 deletions(-)
ACK series.

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