Libguestfs - Feb 2014 - [PATCH 0/3] virt-builder: copy local files instead of using curl

Hi,

this patch serie does a small optimisation in virt-builder, i.e. make
its internal Downloader just copy files when the source is a local URI,
instead of spawn curl in this case too.


Pino Toscano (3):
  builder: isolate C libraries in an own OCAMLCLIBS
  builder: prepare for different per-protocol download actions
  builder: do a copy when downloading local files

 builder/Makefile.am   | 13 ++++++-
 builder/downloader.ml | 95 +++++++++++++++++++++++++++++++--------------------
 2 files changed, 70 insertions(+), 38 deletions(-)

-- 
1.8.3.1

Just moving stuff within Makefile.am, no functional changes.
---
 builder/Makefile.am | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/builder/Makefile.am b/builder/Makefile.am
index 2be495b..78a9e72 100644
--- a/builder/Makefile.am
+++ b/builder/Makefile.am
@@ -113,10 +113,15 @@ endif
 OCAMLCFLAGS = -g -warn-error CDEFLMPSUVYZX $(OCAMLPACKAGES)
 OCAMLOPTFLAGS = $(OCAMLCFLAGS)
 
+OCAMLCLIBS  = \
+	$(LIBLZMA_LIBS) \
+	-pthread -lpthread \
+	-lncurses -lcrypt
+
 virt-builder: $(OBJECTS)
 	$(OCAMLFIND) ocamlopt $(OCAMLOPTFLAGS) \
 	  mlguestfs.cmxa -linkpkg $^ \
-	  -cclib '-pthread $(LIBLZMA_LIBS) -lncurses -lcrypt -lpthread' \
+	  -cclib '$(OCAMLCLIBS)' \
 	  $(OCAML_GCOV_LDFLAGS) \
 	  -o $@
 
-- 
1.8.3.1

Small refactor of Downloader.download_to to allow different download
actions depending on the protocol of the URI (which is now parsed).

No actual behaviour changes, just mostly code motion.
---
 builder/Makefile.am   |  6 ++++
 builder/downloader.ml | 84 ++++++++++++++++++++++++++++-----------------------
 2 files changed, 53 insertions(+), 37 deletions(-)

diff --git a/builder/Makefile.am b/builder/Makefile.am
index 78a9e72..9d2dbc5 100644
--- a/builder/Makefile.am
+++ b/builder/Makefile.am
@@ -80,6 +80,9 @@ OBJECTS = \
 	$(top_builddir)/mllib/password.cmx \
 	$(top_builddir)/mllib/planner.cmx \
 	$(top_builddir)/mllib/config.cmx \
+	$(top_builddir)/fish/guestfish-uri.o \
+	$(top_builddir)/mllib/uri-c.o \
+	$(top_builddir)/mllib/uRI.cmx \
 	index-scan.o \
 	index-struct.o \
 	index-parse.o \
@@ -115,6 +118,9 @@ OCAMLOPTFLAGS = $(OCAMLCFLAGS)
 
 OCAMLCLIBS  = \
 	$(LIBLZMA_LIBS) \
+	$(LIBXML2_LIBS) \
+	-L../src/.libs -lutils \
+	-L../gnulib/lib/.libs -lgnu \
 	-pthread -lpthread \
 	-lncurses -lcrypt
 
diff --git a/builder/downloader.ml b/builder/downloader.ml
index 77f48ae..95b5817 100644
--- a/builder/downloader.ml
+++ b/builder/downloader.ml
@@ -69,50 +69,60 @@ let rec download ~prog t ?template ?progress_bar uri       
(filename, false)
 
 and download_to ~prog t ?(progress_bar = false) uri filename -  (* Get the
status code first to ensure the file exists. *)
-  let cmd = sprintf "%s%s -g -o /dev/null -I -w '%%{http_code}'
%s"
-    t.curl
-    (if t.debug then "" else " -s -S")
-    (quote uri) in
-  if t.debug then eprintf "%s\n%!" cmd;
-  let lines = external_command ~prog cmd in
-  if List.length lines < 1 then (
-    eprintf (f_"%s: unexpected output from curl command, enable debug and
look at previous messages\n") prog;
-    exit 1
-  );
-  let status_code = List.hd lines in
-  let bad_status_code = function
-    | "" -> true
-    | s when s.[0] = '4' -> true (* 4xx *)
-    | s when s.[0] = '5' -> true (* 5xx *)
-    | _ -> false
-  in
-  if bad_status_code status_code then (
-    eprintf (f_"%s: failed to download %s: HTTP status code %s\n")
-      prog uri status_code;
-    exit 1
-  );
+  let parseduri +    try URI.parse_uri uri
+    with Invalid_argument "URI.parse_uri" ->
+      eprintf (f_"Error parsing URI '%s'. Look for error messages
printed above.\n") uri;
+      exit 1 in
 
-  (* Now download the file.
-   * 
-   * Note because there may be parallel virt-builder instances running
+  (* Note because there may be parallel virt-builder instances running
    * and also to avoid partial downloads in the cachedir if the network
    * fails, we download to a random name in the cache and then
    * atomically rename it to the final filename.
    *)
   let filename_new = filename ^ "." ^ string_random8 () in
   unlink_on_exit filename_new;
-  let cmd = sprintf "%s%s -g -o %s %s"
-    t.curl
-    (if t.debug then "" else if progress_bar then " -#"
else " -s -S")
-    (quote filename_new) (quote uri) in
-  if t.debug then eprintf "%s\n%!" cmd;
-  let r = Sys.command cmd in
-  if r <> 0 then (
-    eprintf (f_"%s: curl (download) command failed downloading
'%s'\n")
-      prog uri;
-    exit 1
+
+  (match parseduri.URI.protocol with
+  | _ -> (* Any other protocol. *)
+    (* Get the status code first to ensure the file exists. *)
+    let cmd = sprintf "%s%s -g -o /dev/null -I -w '%%{http_code}'
%s"
+      t.curl
+      (if t.debug then "" else " -s -S")
+      (quote uri) in
+    if t.debug then eprintf "%s\n%!" cmd;
+    let lines = external_command ~prog cmd in
+    if List.length lines < 1 then (
+      eprintf (f_"%s: unexpected output from curl command, enable debug
and look at previous messages\n")
+        prog;
+      exit 1
+    );
+    let status_code = List.hd lines in
+    let bad_status_code = function
+      | "" -> true
+      | s when s.[0] = '4' -> true (* 4xx *)
+      | s when s.[0] = '5' -> true (* 5xx *)
+      | _ -> false
+    in
+    if bad_status_code status_code then (
+      eprintf (f_"%s: failed to download %s: HTTP status code %s\n")
+        prog uri status_code;
+      exit 1
+    );
+
+    (* Now download the file. *)
+    let cmd = sprintf "%s%s -g -o %s %s"
+      t.curl
+      (if t.debug then "" else if progress_bar then " -#"
else " -s -S")
+      (quote filename_new) (quote uri) in
+    if t.debug then eprintf "%s\n%!" cmd;
+    let r = Sys.command cmd in
+    if r <> 0 then (
+      eprintf (f_"%s: curl (download) command failed downloading
'%s'\n")
+        prog uri;
+      exit 1
+    )
   );
 
-  (* Rename the file if curl was successful. *)
+  (* Rename the file if the download was successful. *)
   rename filename_new filename
-- 
1.8.3.1

Instead of spawning curl even to "download" file:// URIs, just copy
them.
---
 builder/downloader.ml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/builder/downloader.ml b/builder/downloader.ml
index 95b5817..e386c06 100644
--- a/builder/downloader.ml
+++ b/builder/downloader.ml
@@ -84,6 +84,17 @@ and download_to ~prog t ?(progress_bar = false) uri filename 
unlink_on_exit filename_new;
 
   (match parseduri.URI.protocol with
+  | "file" ->
+    let path = parseduri.URI.path in
+    let cmd = sprintf "cp%s %s %s"
+      (if t.debug then " -v" else "")
+      (quote path) (quote filename_new) in
+    let r = Sys.command cmd in
+    if r <> 0 then (
+      eprintf (f_"%s: cp (download) command failed copying
'%s'\n")
+        prog path;
+      exit 1
+    )
   | _ -> (* Any other protocol. *)
     (* Get the status code first to ensure the file exists. *)
     let cmd = sprintf "%s%s -g -o /dev/null -I -w '%%{http_code}'
%s"
-- 
1.8.3.1

On Tue, Feb 11, 2014 at 01:09:34PM +0100, Pino Toscano
wrote:
> Hi,
> 
> this patch serie does a small optimisation in virt-builder, i.e. make
> its internal Downloader just copy files when the source is a local URI,
> instead of spawn curl in this case too.
I sort of wonder if it makes any difference?

Anyway patches look fine, so ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
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