[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
participants (2)
-
Pino Toscano -
Richard W.M. Jones