Libguestfs - Mar 2015 - [PATCH 1/2] mllib: allow external_command to return on nonzero return value

This patchset adds support for resuming downloads in virt-builder.

Partially downloaded file is not deleted on exit anymore.
There is a check for partially downloaded image in cache directory
based on its name. When found, download_to crafts appropriate
options to continue its download.

Maros Zatko (2):
  mllib: allow external_command to return [] on nonzero return value
  builder: support for download resume

 builder/downloader.ml  | 16 ++++++++++++----
 mllib/common_utils.ml  | 15 ++++++++++-----
 mllib/common_utils.mli |  2 +-
 3 files changed, 23 insertions(+), 10 deletions(-)

-- 
1.9.3

This is useful for probing probing for cache files such as:
external_command ?ignore_error:(Some true) ~prog "ls
.cache/something.*"
will return command output (matched files) on its success or empty list
whenits exit code is other than 0 (there are no such files).
---
 mllib/common_utils.ml  | 15 ++++++++++-----
 mllib/common_utils.mli |  2 +-
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
index 76d8b79..9719b16 100644
--- a/mllib/common_utils.ml
+++ b/mllib/common_utils.ml
@@ -528,23 +528,28 @@ let compare_lvm2_uuids uuid1 uuid2    loop 0 0
 
 (* Run an external command, slurp up the output as a list of lines. *)
-let external_command ~prog cmd +let external_command ~prog ?(ignore_error =
false) cmd    let chan = Unix.open_process_in cmd in
   let lines = ref [] in
   (try while true do lines := input_line chan :: !lines done
    with End_of_file -> ());
   let lines = List.rev !lines in
   let stat = Unix.close_process_in chan in
-  (match stat with
-  | Unix.WEXITED 0 -> ()
+  match stat with
+  | Unix.WEXITED 0 ->
+    (* Command exited correctly, return its output *)
+    lines
+  | Unix.WEXITED i when ignore_error ->
+    (* Command failed. in case such as 'ls something*' when path
doesn't exist
+     * and desired behavior is to get empty list instead of error.
+     * This is useful for probing partial downloads *)
+    []
   | Unix.WEXITED i ->
     error ~prog (f_"external command '%s' exited with error
%d") cmd i
   | Unix.WSIGNALED i ->
     error ~prog (f_"external command '%s' killed by signal
%d") cmd i
   | Unix.WSTOPPED i ->
     error ~prog (f_"external command '%s' stopped by signal
%d") cmd i
-  );
-  lines
 
 (* Run uuidgen to return a random UUID. *)
 let uuidgen ~prog () diff --git a/mllib/common_utils.mli
b/mllib/common_utils.mli
index 28ba648..ce2242a 100644
--- a/mllib/common_utils.mli
+++ b/mllib/common_utils.mli
@@ -109,7 +109,7 @@ val compare_version : string -> string -> int
 val compare_lvm2_uuids : string -> string -> int
 (** Compare two LVM2 UUIDs, ignoring '-' characters. *)
 
-val external_command : prog:string -> string -> string list
+val external_command : prog:string -> ?ignore_error:bool -> string ->
string list
 (** Run an external command, slurp up the output as a list of lines. *)
 
 val uuidgen : prog:string -> unit -> string
-- 
1.9.3

Partially downloaded file is not deleted on exit anymore.
There is a check for partially downloaded image in cache directory
based on its name. When found, download_to crafts appropriate
options to continue its download.
---
 builder/downloader.ml | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/builder/downloader.ml b/builder/downloader.ml
index 8a23bdc..6e19ee4 100644
--- a/builder/downloader.ml
+++ b/builder/downloader.ml
@@ -65,11 +65,11 @@ let rec download ~prog t ?template ?progress_bar ?(proxy =
SystemProxy) uri         * If not, download it.
        *)
       if not (Sys.file_exists filename) then
-        download_to ~prog t ?progress_bar ~proxy uri filename;
+        download_to ~prog t ?progress_bar ?continue:(Some true) ~proxy uri
filename;
 
       (filename, false)
 
-and download_to ~prog t ?(progress_bar = false) ~proxy uri filename +and
download_to ~prog t ?(progress_bar = false) ?(continue = false) ~proxy uri
filename    let parseduri      try URI.parse_uri uri
     with Invalid_argument "URI.parse_uri" ->
@@ -82,7 +82,6 @@ and download_to ~prog t ?(progress_bar = false) ~proxy uri
filename     * atomically rename it to the final filename.
    *)
   let filename_new = filename ^ "." ^ string_random8 () in
-  unlink_on_exit filename_new;
 
   (match parseduri.URI.protocol with
   | "file" ->
@@ -115,11 +114,20 @@ and download_to ~prog t ?(progress_bar = false) ~proxy uri
filename      if bad_status_code status_code then
       error (f_"failed to download %s: HTTP status code %s") uri
status_code;
 
+    let cmd = sprintf "ls %s.* 2>/dev/null" filename in
+    let lines = if continue
+      then external_command ~prog ?ignore_error:(Some true) cmd
+      else [] in
+    let filename_new, continue_download = match List.length lines with
+      | 0 -> filename_new, ""
+      | _ -> List.hd lines, " -C -" in
+
     (* Now download the file. *)
-    let cmd = sprintf "%s%s%s -g -o %s %s"
+    let cmd = sprintf "%s%s%s%s -g -o %s %s"
       outenv
       t.curl
       (if t.verbose then "" else if progress_bar then " -#"
else " -s -S")
+      continue_download
       (quote filename_new) (quote uri) in
     if t.verbose then printf "%s\n%!" cmd;
     let r = Sys.command cmd in
-- 
1.9.3

On Wednesday 18 March 2015 17:20:07 Maros Zatko wrote:
> Partially downloaded file is not deleted on exit anymore.
> There is a check for partially downloaded image in cache directory
> based on its name. When found, download_to crafts appropriate
> options to continue its download.
> ---
>  builder/downloader.ml | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/builder/downloader.ml b/builder/downloader.ml
> index 8a23bdc..6e19ee4 100644
> --- a/builder/downloader.ml
> +++ b/builder/downloader.ml
> @@ -65,11 +65,11 @@ let rec download ~prog t ?template ?progress_bar
?(proxy = SystemProxy) uri >         * If not, download it.
>         *)
>        if not (Sys.file_exists filename) then
> -        download_to ~prog t ?progress_bar ~proxy uri filename;
> +        download_to ~prog t ?progress_bar ?continue:(Some true) ~proxy uri
filename;
~continue:true should work too.
>  
>        (filename, false)
>  
> -and download_to ~prog t ?(progress_bar = false) ~proxy uri filename >
+and download_to ~prog t ?(progress_bar = false) ?(continue = false) ~proxy uri
filename >    let parseduri >      try URI.parse_uri uri
>      with Invalid_argument "URI.parse_uri" ->
> @@ -82,7 +82,6 @@ and download_to ~prog t ?(progress_bar = false) ~proxy
uri filename >     * atomically rename it to the final filename.
>     *)
>    let filename_new = filename ^ "." ^ string_random8 () in
> -  unlink_on_exit filename_new;
>  
>    (match parseduri.URI.protocol with
>    | "file" ->
> @@ -115,11 +114,20 @@ and download_to ~prog t ?(progress_bar = false)
~proxy uri filename >      if bad_status_code status_code then
>        error (f_"failed to download %s: HTTP status code %s") uri
status_code;
>  
> +    let cmd = sprintf "ls %s.* 2>/dev/null" filename in
This should rather use Sys.readdir + List.filter.
> +    let lines = if continue
> +      then external_command ~prog ?ignore_error:(Some true) cmd
> +      else [] in
> +    let filename_new, continue_download = match List.length lines with
> +      | 0 -> filename_new, ""
> +      | _ -> List.hd lines, " -C -" in
> +
>      (* Now download the file. *)
> -    let cmd = sprintf "%s%s%s -g -o %s %s"
> +    let cmd = sprintf "%s%s%s%s -g -o %s %s"
>        outenv
>        t.curl
>        (if t.verbose then "" else if progress_bar then "
-#" else " -s -S")
> +      continue_download
>        (quote filename_new) (quote uri) in
>      if t.verbose then printf "%s\n%!" cmd;
>      let r = Sys.command cmd in
> 
-- 
Pino Toscano

On Wed, Mar 18, 2015 at 05:20:06PM +0100, Maros Zatko
wrote:
> This is useful for probing probing for cache files such as:
> external_command ?ignore_error:(Some true) ~prog "ls
.cache/something.*"
> will return command output (matched files) on its success or empty list
> whenits exit code is other than 0 (there are no such files).
I would write this as:

  let files = Sys.readdir ".cache" inm
  let files = Array.to_list files in
  let files = List.filter (fun s -> string_prefix s "something.")
in

  (*
  or if you just want to check existence of any something.* file:

  let file_exists = List.exists (fun s -> string_prefix s
"something.") in
  *)

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 Wed, Mar 18, 2015 at 05:20:07PM +0100, Maros Zatko
wrote:
> Partially downloaded file is not deleted on exit anymore.
> There is a check for partially downloaded image in cache directory
> based on its name. When found, download_to crafts appropriate
> options to continue its download.
> ---
>  builder/downloader.ml | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/builder/downloader.ml b/builder/downloader.ml
> index 8a23bdc..6e19ee4 100644
> --- a/builder/downloader.ml
> +++ b/builder/downloader.ml
> @@ -65,11 +65,11 @@ let rec download ~prog t ?template ?progress_bar
?(proxy = SystemProxy) uri >         * If not, download it.
>         *)
>        if not (Sys.file_exists filename) then
> -        download_to ~prog t ?progress_bar ~proxy uri filename;
> +        download_to ~prog t ?progress_bar ?continue:(Some true) ~proxy uri
filename;
To expand on what Pino said:

?foo arguments are passed as options, ie. None or Some ..  The
function has (hidden, generated by the compiler) code which does
effectively:

  let foo = match ?foo with None -> (* default value *) | Some x -> x in

When calling if you write the argument as ~foo:true then the
compiler automatically translates that to 'Some true'.
>        (filename, false)
>  
> -and download_to ~prog t ?(progress_bar = false) ~proxy uri filename >
+and download_to ~prog t ?(progress_bar = false) ?(continue = false) ~proxy uri
filename >    let parseduri >      try URI.parse_uri uri
>      with Invalid_argument "URI.parse_uri" ->
> @@ -82,7 +82,6 @@ and download_to ~prog t ?(progress_bar = false) ~proxy
uri filename >     * atomically rename it to the final filename.
>     *)
>    let filename_new = filename ^ "." ^ string_random8 () in
> -  unlink_on_exit filename_new;
>  
>    (match parseduri.URI.protocol with
>    | "file" ->
> @@ -115,11 +114,20 @@ and download_to ~prog t ?(progress_bar = false)
~proxy uri filename >      if bad_status_code status_code then
>        error (f_"failed to download %s: HTTP status code %s") uri
status_code;
>  
> +    let cmd = sprintf "ls %s.* 2>/dev/null" filename in
> +    let lines = if continue
> +      then external_command ~prog ?ignore_error:(Some true) cmd
> +      else [] in
> +    let filename_new, continue_download = match List.length lines with
> +      | 0 -> filename_new, ""
> +      | _ -> List.hd lines, " -C -" in
> +
>      (* Now download the file. *)
> -    let cmd = sprintf "%s%s%s -g -o %s %s"
> +    let cmd = sprintf "%s%s%s%s -g -o %s %s"
>        outenv
>        t.curl
>        (if t.verbose then "" else if progress_bar then "
-#" else " -s -S")
> +      continue_download
>        (quote filename_new) (quote uri) in
>      if t.verbose then printf "%s\n%!" cmd;
>      let r = Sys.command cmd in
I can't say I'm very happy to have partial downloads lying around in
the cache directory.

Let's see what Pino thinks of the updated patch however.

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