Libguestfs - Apr 2020 - Re: [v2v PATCH 2/2] Consolidate handling of temporary files/dirs

Since this new temporary directory will contain UNIX sockets for
communicating with nbdkit, then its path must not be too long.

Use the existing directory that libguestfs exposes for this, i.e.
sockdir.
---
 v2v/nbdkit.ml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/v2v/nbdkit.ml b/v2v/nbdkit.ml
index 65317f9b..46b20c9d 100644
--- a/v2v/nbdkit.ml
+++ b/v2v/nbdkit.ml
@@ -103,9 +103,12 @@ let add_filter_if_available cmd filter    if probe_filter
filter then add_filter cmd filter else cmd
 
 let run_unix cmd -  (* Create a temporary directory where we place the socket
and PID file. *)
+  (* Create a temporary directory where we place the socket and PID file.
+   * Use the libguestfs socket directory, so it is more likely the full path
+   * of the UNIX sockets will fit in the (limited) socket pathname.
+   *)
   let tmpdir -    let base_dir = (open_guestfs ())#get_cachedir () in
+    let base_dir = (open_guestfs ())#get_sockdir () in
     let t = Mkdtemp.temp_dir ~base_dir "v2vnbdkit." in
     (* tmpdir must be readable (but not writable) by "other" so that
      * qemu can open the sockets.
-- 
2.25.1

Create two temporary directories for all the files created during the
virt-v2v run:
1) tmpdir, created as $TMPDIR/virt-v2v.XXXXXX, for all the small files
2) cachedir, created as $LIBGUESTFS_CACHEDIR/virt-v2v.XXXXXX, for the
   big files (e.g. disks)
This way there is no need to manually schedule all the temporary files
and directories for removal when the application quits.
---
 v2v/input_ova.ml         |  5 ++---
 v2v/input_vmx.ml         |  5 -----
 v2v/measure_disk.ml      |  5 +++--
 v2v/output_glance.ml     | 18 ++++++------------
 v2v/output_libvirt.ml    |  3 +++
 v2v/output_null.ml       | 14 ++------------
 v2v/output_openstack.ml  |  3 +--
 v2v/output_rhv.ml        |  2 +-
 v2v/output_rhv_upload.ml |  7 -------
 v2v/parse_ova.ml         | 30 ++++++++++++++----------------
 v2v/utils.ml             | 25 +++++++++++++++++++++++--
 v2v/utils.mli            |  7 +++++++
 v2v/v2v.ml               | 15 ++++++---------
 13 files changed, 68 insertions(+), 71 deletions(-)

diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
index 5d3bece1..5829c15e 100644
--- a/v2v/input_ova.ml
+++ b/v2v/input_ova.ml
@@ -26,6 +26,7 @@ open Types
 open Parse_ova
 open Parse_ovf_from_ova
 open Name_from_disk
+open Utils
 
 (* RHBZ#1570407: VMware-generated OVA files found in the wild can
  * contain hrefs referencing snapshots.  The href will be something
@@ -132,9 +133,7 @@ class input_ova ova = object
            (* The spec allows the file to be gzip-compressed, in
             * which case we must uncompress it into a temporary.
             *)
-           let temp_dir = (open_guestfs ())#get_cachedir () in
-           let new_filename = Filename.temp_file ~temp_dir "ova"
".vmdk" in
-           unlink_on_exit new_filename;
+           let new_filename = Filename.temp_file ~temp_dir:cachedir
"ova" ".vmdk" in
            let cmd               sprintf "zcat %s > %s" (quote
filename) (quote new_filename) in
            if shell_command cmd <> 0 then
diff --git a/v2v/input_vmx.ml b/v2v/input_vmx.ml
index f1d143e9..6fd60f94 100644
--- a/v2v/input_vmx.ml
+++ b/v2v/input_vmx.ml
@@ -388,11 +388,6 @@ and find_nics vmx    nics
 
 class input_vmx input_password input_transport arg -  let tmpdir -    let
base_dir = (open_guestfs ())#get_cachedir () in
-    let t = Mkdtemp.temp_dir ~base_dir "vmx." in
-    rmdir_on_exit t;
-    t in
 object
   inherit input
 
diff --git a/v2v/measure_disk.ml b/v2v/measure_disk.ml
index 5c01eaac..6a1212f0 100644
--- a/v2v/measure_disk.ml
+++ b/v2v/measure_disk.ml
@@ -21,6 +21,8 @@ open Tools_utils
 open JSON_parser
 open Common_gettext.Gettext
 
+open Utils
+
 (* Run qemu-img measure on a disk. *)
 
 let measure ?format filename @@ -38,8 +40,7 @@ let measure ?format filename   
List.push_back cmd "--output=json";
   List.push_back cmd filename;
 
-  let json, chan = Filename.open_temp_file "v2vmeasure"
".json" in
-  unlink_on_exit json;
+  let json, chan = Filename.open_temp_file ~temp_dir:tmpdir
"v2vmeasure" ".json" in
   let fd = Unix.descr_of_out_channel chan in
   if run_command ~stdout_fd:fd !cmd <> 0 then
     error (f_"qemu-img measure failed, see earlier errors");
diff --git a/v2v/output_glance.ml b/v2v/output_glance.ml
index 0a9e9181..f2468055 100644
--- a/v2v/output_glance.ml
+++ b/v2v/output_glance.ml
@@ -27,16 +27,6 @@ open Types
 open Utils
 
 class output_glance () -  (* Although glance can slurp in a stream from stdin,
unfortunately
-   * 'qemu-img convert' cannot write to a stream (although I guess
-   * it could be implemented at least for raw).  Therefore we have
-   * to write to a temporary file.  XXX
-   *)
-  let tmpdir -    let base_dir = (open_guestfs ())#get_cachedir () in
-    let t = Mkdtemp.temp_dir ~base_dir "glance." in
-    rmdir_on_exit t;
-    t in
 object
   inherit output
 
@@ -60,8 +50,12 @@ object
   method supported_firmware = [ TargetBIOS; TargetUEFI ]
 
   method prepare_targets _ overlays _ -    (* Write targets to a temporary
local file - see above for reason. *)
-    List.map (fun (_, ov) -> TargetFile (tmpdir // ov.ov_sd)) overlays
+    (* Although glance can slurp in a stream from stdin, unfortunately
+     * 'qemu-img convert' cannot write to a stream (although I guess
+     * it could be implemented at least for raw).  Therefore we have
+     * to write to a temporary file.  XXX
+     *)
+    List.map (fun (_, ov) -> TargetFile (cachedir // ov.ov_sd)) overlays
 
   method create_metadata source targets
                          target_buses guestcaps inspect target_firmware diff
--git a/v2v/output_libvirt.ml b/v2v/output_libvirt.ml
index 033e26b1..9c189b9d 100644
--- a/v2v/output_libvirt.ml
+++ b/v2v/output_libvirt.ml
@@ -176,6 +176,9 @@ object (self)
       create_libvirt_xml ~pool:pool_name source targets target_buses
                          guestcaps target_features target_firmware inspect in
 
+    (* Do not store the temporary file inside our [tmpdir], as we preserve it
+     * for the user in case Domain.define_xml fails.
+     *)
     let tmpfile, chan = Filename.open_temp_file "v2vlibvirt"
".xml" in
     DOM.doc_to_chan chan doc;
     close_out chan;
diff --git a/v2v/output_null.ml b/v2v/output_null.ml
index 3528da50..493c9f0e 100644
--- a/v2v/output_null.ml
+++ b/v2v/output_null.ml
@@ -50,7 +50,7 @@ let can_use_qemu_null_co_device ()    (* We actually attempt
to convert a raw file to the null-co device
    * using a JSON URL.
    *)
-  let tmp = Filename.temp_file "v2vqemunullcotst" ".img" in
+  let tmp = Filename.temp_file ~temp_dir:tmpdir "v2vqemunullcotst"
".img" in
   Unix.truncate tmp 1024;
 
   let json = [
@@ -65,20 +65,10 @@ let can_use_qemu_null_co_device ()              (if verbose
() then "" else " 2>&1") in
   debug "%s" cmd;
   let r = 0 = Sys.command cmd in
-  Unix.unlink tmp;
   debug "qemu-img supports the null-co device: %b" r;
   r
 
 class output_null -  (* Create a temporary directory which is always deleted at
exit,
-   * so we can put the drives there in case qemu does not support
-   * the null-co device w/ a JSON URL.
-   *)
-  let tmpdir -    let base_dir = (open_guestfs ())#get_cachedir () in
-    let t = Mkdtemp.temp_dir ~base_dir "null." in
-    rmdir_on_exit t;
-    t in
 object
   inherit output
 
@@ -99,7 +89,7 @@ object
 
       List.map (fun _ -> target_file) overlays
     ) else (
-      List.map (fun (_, ov) -> TargetFile (tmpdir // ov.ov_sd)) overlays
+      List.map (fun (_, ov) -> TargetFile (cachedir // ov.ov_sd)) overlays
     )
 
   method create_metadata _ _ _ _ _ _ = ()
diff --git a/v2v/output_openstack.ml b/v2v/output_openstack.ml
index 179b0edf..39f30c9e 100644
--- a/v2v/output_openstack.ml
+++ b/v2v/output_openstack.ml
@@ -191,8 +191,7 @@ class output_openstack output_conn output_password
output_storage
   let run_openstack_command_capture_json args      let cmd = [ openstack_binary
] @ extra_args @ args in
 
-    let json, chan = Filename.open_temp_file "v2vopenstack"
".json" in
-    unlink_on_exit json;
+    let json, chan = Filename.open_temp_file ~temp_dir:tmpdir
"v2vopenstack" ".json" in
     let fd = descr_of_out_channel chan in
 
     (* Note that Tools_utils.run_command closes fd.
diff --git a/v2v/output_rhv.ml b/v2v/output_rhv.ml
index 49cc0381..49bfaaa8 100644
--- a/v2v/output_rhv.ml
+++ b/v2v/output_rhv.ml
@@ -40,7 +40,7 @@ let rec mount_and_check_storage_domain domain_class os      (*
Create a mountpoint.  Default mode is too restrictive for us
      * when we need to write into the directory as 36:36.
      *)
-    let mp = Mkdtemp.temp_dir "v2v." in
+    let mp = Mkdtemp.temp_dir ~base_dir:tmpdir "v2v." in
     chmod mp 0o755;
 
     (* Try mounting it. *)
diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml
index 5c6c2611..40498ba3 100644
--- a/v2v/output_rhv_upload.ml
+++ b/v2v/output_rhv_upload.ml
@@ -153,13 +153,6 @@ let json_optstring = function
 class output_rhv_upload output_alloc output_conn
                         output_password output_storage
                         rhv_options -  (* Create a temporary directory which
will be deleted on exit. *)
-  let tmpdir -    let base_dir = (open_guestfs ())#get_cachedir () in
-    let t = Mkdtemp.temp_dir ~base_dir "rhvupload." in
-    rmdir_on_exit t;
-    t in
-
   let diskid_file_of_id id = tmpdir // sprintf "diskid.%d" id in
 
   (* Create Python scripts for precheck, vmcheck, plugin and create VM. *)
diff --git a/v2v/parse_ova.ml b/v2v/parse_ova.ml
index 0b939ac4..5c8d349f 100644
--- a/v2v/parse_ova.ml
+++ b/v2v/parse_ova.ml
@@ -70,11 +70,10 @@ let rec parse_ova ova    let top_dir, ova_type      if
is_directory ova then ova, Directory
     else (
-      let tmpdir -        let base_dir = (open_guestfs ())#get_cachedir () in
-        let t = Mkdtemp.temp_dir ~base_dir "ova." in
-        rmdir_on_exit t;
-        t in
+      let ovatmpdir +        let d = cachedir // "ova" in
+        Unix.mkdir d 0o700;
+        d in
 
       match detect_file_type ova with
       | `Tar ->
@@ -85,15 +84,15 @@ let rec parse_ova ova            *)
          if qemu_img_supports_offset_and_size () &&
             libvirt_supports_json_raw_driver () &&
-            (untar_metadata ova tmpdir;
-             no_disks_are_compressed ova tmpdir) then
-           tmpdir, TarOptimized ova
+            (untar_metadata ova ovatmpdir;
+             no_disks_are_compressed ova ovatmpdir) then
+           ovatmpdir, TarOptimized ova
          else (
            (* If qemu/libvirt is too old or any disk is compressed
             * then we must fall back on the slow path.
             *)
-           untar ova tmpdir;
-           tmpdir, Directory
+           untar ova ovatmpdir;
+           ovatmpdir, Directory
          )
 
       | `Zip ->
@@ -102,16 +101,16 @@ let rec parse_ova ova            *)
          let cmd             [ "unzip" ] @ (if verbose () then []
else [ "-q" ]) @
-           [ "-j"; "-d"; tmpdir; ova ] in
+           [ "-j"; "-d"; ovatmpdir; ova ] in
          if run_command cmd <> 0 then
            error (f_"error unpacking %s, see earlier error messages")
ova;
-         tmpdir, Directory
+         ovatmpdir, Directory
 
       | (`GZip|`XZ) as format ->
          (match uncompressed_type format ova with
           | `Tar ->
-             untar ~format ova tmpdir;
-             tmpdir, Directory
+             untar ~format ova ovatmpdir;
+             ovatmpdir, Directory
           | `Zip | `GZip | `XZ | `Unknown ->
              error (f_"%s: unsupported file format\n\nFormats which we
currently understand for '-i ova' are: tar (uncompressed, compress with
gzip or xz), zip") ova
          )
@@ -222,11 +221,10 @@ and uncompress_head format file  and uncompressed_type
format file    let head, headlen = uncompress_head format file in
   let tmpfile, chan -    Filename.open_temp_file "ova.file."
"" in
+    Filename.open_temp_file ~temp_dir:tmpdir "ova.file." ""
in
   output chan head 0 headlen;
   close_out chan;
   let ret = detect_file_type tmpfile in
-  Sys.remove tmpfile;
   ret
 
 (* Find files in [dir] ending with [ext]. *)
diff --git a/v2v/utils.ml b/v2v/utils.ml
index ccbb9d68..3dbb4da9 100644
--- a/v2v/utils.ml
+++ b/v2v/utils.ml
@@ -22,8 +22,30 @@ open Printf
 
 open Std_utils
 open Tools_utils
+open Unix_utils
 open Common_gettext.Gettext
 
+let lazy_tmpdir +  lazy (
+    let tmpdir = Mkdtemp.temp_dir "virt-v2v." in
+    rmdir_on_exit tmpdir;
+    tmpdir
+  )
+
+let lazy_cachedir +  lazy (
+    let base_dir = (open_guestfs ())#get_cachedir () in
+    let cachedir = Mkdtemp.temp_dir ~base_dir "virt-v2v." in
+    rmdir_on_exit cachedir;
+    cachedir
+  )
+
+let tmpdir +  Lazy.force lazy_tmpdir
+
+let cachedir +  Lazy.force lazy_cachedir
+
 (* Is SELinux enabled and enforcing on the host? *)
 let have_selinux    0 = Sys.command "getenforce 2>/dev/null | grep -isq
Enforcing"
@@ -112,7 +134,7 @@ let qemu_img_supports_offset_and_size ()    (* We actually
attempt to create a qcow2 file with a raw backing
    * file that has an offset and size.
    *)
-  let tmp = Filename.temp_file "v2vqemuimgtst" ".img" in
+  let tmp = Filename.temp_file ~temp_dir:tmpdir "v2vqemuimgtst"
".img" in
   Unix.truncate tmp 1024;
 
   let json = [
@@ -132,7 +154,6 @@ let qemu_img_supports_offset_and_size ()              (if
verbose () then "" else " 2>&1") in
   debug "%s" cmd;
   let r = 0 = Sys.command cmd in
-  Unix.unlink tmp;
   debug "qemu-img supports \"offset\" and \"size\" in
json URLs: %b" r;
   r
 
diff --git a/v2v/utils.mli b/v2v/utils.mli
index 937e2b9b..8f279263 100644
--- a/v2v/utils.mli
+++ b/v2v/utils.mli
@@ -18,6 +18,13 @@
 
 (** Utilities used in virt-v2v only. *)
 
+val tmpdir : string
+(** The single temporary directory of virt-v2v for small files. *)
+
+val cachedir : string
+(** The single temporary directory of virt-v2v for big files,
+    such as disks. *)
+
 val have_selinux : bool
 (** True if SELinux is enabled and enforcing on the host. *)
 
diff --git a/v2v/v2v.ml b/v2v/v2v.ml
index 73edff2c..6e88d978 100644
--- a/v2v/v2v.ml
+++ b/v2v/v2v.ml
@@ -264,8 +264,6 @@ and set_source_networks_and_bridges cmdline source    let
nics = List.map (Networks.map cmdline.network_map) source.s_nics in
   { source with s_nics = nics }
 
-and overlay_dir = (open_guestfs ())#get_cachedir ()
-
 (* Conversion can fail or hang if there is insufficient free space in
  * the temporary directory used to store overlays on the host
  * (RHBZ#1316479).  Although only a few hundred MB is actually
@@ -273,12 +271,12 @@ and overlay_dir = (open_guestfs ())#get_cachedir ()
  * guestfs appliance which is also stored here.
  *)
 and check_host_free_space () -  let free_space = StatVFS.free_space
(StatVFS.statvfs overlay_dir) in
-  debug "check_host_free_space: overlay_dir=%s free_space=%Ld"
-        overlay_dir free_space;
+  let free_space = StatVFS.free_space (StatVFS.statvfs cachedir) in
+  debug "check_host_free_space: cachedir=%s free_space=%Ld"
+        cachedir free_space;
   if free_space < 1_073_741_824L then
     error (f_"insufficient free space in the conversion server temporary
directory %s (%s).\n\nEither free up space in that directory, or set the
LIBGUESTFS_CACHEDIR environment variable to point to another directory with more
than 1GB of free space.\n\nSee also the virt-v2v(1) manual, section
\"Minimum free space check in the host\".")
-          overlay_dir (human_size free_space)
+          cachedir (human_size free_space)
 
 (* Create a qcow2 v3 overlay to protect the source image(s). *)
 and create_overlays source_disks @@ -286,8 +284,7 @@ and create_overlays
source_disks    List.mapi (
     fun i ({ s_qemu_uri = qemu_uri; s_format = format } as source) ->
       let overlay_file -        Filename.temp_file ~temp_dir:overlay_dir
"v2vovl" ".qcow2" in
-      unlink_on_exit overlay_file;
+        Filename.temp_file ~temp_dir:cachedir "v2vovl"
".qcow2" in
 
       (* There is a specific reason to use the newer qcow2 variant:
        * Because the L2 table can store zero clusters efficiently, and
@@ -823,7 +820,7 @@ and preserve_overlays overlays src_name    List.iter (
     fun ov ->
       let saved_filename -        sprintf "%s/%s-%s.qcow2"
overlay_dir src_name ov.ov_sd in
+        sprintf "%s/%s-%s.qcow2" cachedir src_name ov.ov_sd in
       rename ov.ov_overlay_file saved_filename;
       info (f_"Overlay saved as %s [--debug-overlays]")
saved_filename
   ) overlays
-- 
2.25.1

On 4/6/20 10:40 AM, Pino Toscano wrote:
> Create two temporary directories for all the files created during the
> virt-v2v run:
> 1) tmpdir, created as $TMPDIR/virt-v2v.XXXXXX, for all the small files
> 2) cachedir, created as $LIBGUESTFS_CACHEDIR/virt-v2v.XXXXXX, for the
>     big files (e.g. disks)
> This way there is no need to manually schedule all the temporary files
> and directories for removal when the application quits.
> ---
Side note:
> +++ b/v2v/output_glance.ml
> @@ -27,16 +27,6 @@ open Types
>   open Utils
>   
>   class output_glance () > -  (* Although glance can slurp in a stream
from stdin, unfortunately
> -   * 'qemu-img convert' cannot write to a stream (although I guess
> -   * it could be implemented at least for raw).  Therefore we have
> -   * to write to a temporary file.  XXX
> -   *)
> -  let tmpdir > -    let base_dir = (open_guestfs ())#get_cachedir () in
> -    let t = Mkdtemp.temp_dir ~base_dir "glance." in
> -    rmdir_on_exit t;
> -    t in
>   object
>     inherit output
>   
> @@ -60,8 +50,12 @@ object
>     method supported_firmware = [ TargetBIOS; TargetUEFI ]
>   
>     method prepare_targets _ overlays _ > -    (* Write targets to a
temporary local file - see above for reason. *)
> -    List.map (fun (_, ov) -> TargetFile (tmpdir // ov.ov_sd)) overlays
> +    (* Although glance can slurp in a stream from stdin, unfortunately
> +     * 'qemu-img convert' cannot write to a stream (although I
guess
> +     * it could be implemented at least for raw).  Therefore we have
> +     * to write to a temporary file.  XXX
> +     *)
> +    List.map (fun (_, ov) -> TargetFile (cachedir // ov.ov_sd))
overlays
I know this was just code motion, but does the nbdkit streaming plugin 
help here?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Apr 06, 2020 at 05:40:44PM +0200, Pino Toscano
wrote:
> Since this new temporary directory will contain UNIX sockets for
> communicating with nbdkit, then its path must not be too long.
> 
> Use the existing directory that libguestfs exposes for this, i.e.
> sockdir.
> ---
>  v2v/nbdkit.ml | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/v2v/nbdkit.ml b/v2v/nbdkit.ml
> index 65317f9b..46b20c9d 100644
> --- a/v2v/nbdkit.ml
> +++ b/v2v/nbdkit.ml
> @@ -103,9 +103,12 @@ let add_filter_if_available cmd filter >    if
probe_filter filter then add_filter cmd filter else cmd
>  
>  let run_unix cmd > -  (* Create a temporary directory where we place
the socket and PID file. *)
> +  (* Create a temporary directory where we place the socket and PID file.
> +   * Use the libguestfs socket directory, so it is more likely the full
path
> +   * of the UNIX sockets will fit in the (limited) socket pathname.
> +   *)
>    let tmpdir > -    let base_dir = (open_guestfs ())#get_cachedir () in
> +    let base_dir = (open_guestfs ())#get_sockdir () in
>      let t = Mkdtemp.temp_dir ~base_dir "v2vnbdkit." in
>      (* tmpdir must be readable (but not writable) by "other" so
that
>       * qemu can open the sockets.
Yes, this could be quite a serious bug if the temporary directory was
set to some long path -- it might even affect ‘make check’ in a
sufficiently deep subdirectory.  ACK.

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 Mon, Apr 06, 2020 at 05:40:45PM +0200, Pino Toscano
wrote:
> Create two temporary directories for all the files created during the
> virt-v2v run:
> 1) tmpdir, created as $TMPDIR/virt-v2v.XXXXXX, for all the small files
> 2) cachedir, created as $LIBGUESTFS_CACHEDIR/virt-v2v.XXXXXX, for the
>    big files (e.g. disks)
> This way there is no need to manually schedule all the temporary files
> and directories for removal when the application quits.
So part of the idea here is fine - having a single tmpdir for small
files and another single tmpdir for large files.  That's a worthwhile
improvement on what I posted.

However the greater problem is this doesn't fix the containers problem
as far as I can see.  We still are unable to separate out the location
of the libguestfs appliance from the large temporary overlay files
created by virt-v2v.

So I don't understand how this helps at all, and therefore at this
time I cannot approve it.

I'm installing another kubernetes install locally with NFS PVs so
hopefully I will have a better understanding of exactly what the
problem is later today.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top