[Libguestfs] [PATCH v2] resize: support non-local output disks (RHBZ#1404182)

[Richard W.M. Jones]

On Thu, Feb 02, 2017 at 02:34:24PM +0100, Pino Toscano wrote:
> Parse the output disk as URI, and use all its attributes just like
> it is done for the input disk.  The only change is that the fsync of the
> output disk is limited now for local URIs only, since it will not work
> with remote protocols.
> ---
>  resize/resize.ml       | 43 +++++++++++++++++++++++++++++++------------
>  resize/virt-resize.pod |  6 +++---
>  2 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/resize/resize.ml b/resize/resize.ml
> index 19cd8df..7e16cb5 100644
> --- a/resize/resize.ml
> +++ b/resize/resize.ml
> @@ -315,6 +315,13 @@ read the man page virt-resize(1).
>          error (f_"error parsing URI '%s'. Look for error messages printed above.")
>            infile in
>  
> +    (* outfile can be a URI. *)
> +    let outfile =
> +      try (outfile, URI.parse_uri outfile)
> +      with Invalid_argument "URI.parse_uri" ->
> +        error (f_"error parsing URI '%s'. Look for error messages printed above.")
> +          outfile in

This matches what we do with 'infile', but I wonder if that isn't
wrong, ie. we should warn or even ignore if we couldn't parse the
infile / outfile as a URI and construct a local file URI.

[...]

The following would be a bit safer if all the new variables
it introduces were scoped, ie:

  let () =
> +    let _, { URI.path = path; protocol = protocol;
> +             server = server; username = username;
> +             password = password } = outfile in
>      (* The output disk is being created, so use cache=unsafe here. *)
>      g#add_drive ?format:output_format ~readonly:false ~cachemode:"unsafe"
> -      outfile;
> +      ~protocol ?server ?username ?secret:password path;
>      if not (quiet ()) then Progress.set_up_progress_bar ~machine_readable g;
>      g#launch ()
                  ^ in
[...]

and the same here:

>        let g = open_guestfs () in
> +      let _, { URI.path = path; protocol = protocol;
> +               server = server; username = username;
> +               password = password } = outfile in
>        (* The output disk is being created, so use cache=unsafe here. *)
>        g#add_drive ?format:output_format ~readonly:false ~cachemode:"unsafe"
> -        outfile;
> +        ~protocol ?server ?username ?secret:password path;
>        if not (quiet ()) then Progress.set_up_progress_bar ~machine_readable g;
>        g#launch ();
> +  (* Try to sync the destination disk only if it is a local file. *)
> +  let () =
> +    let _, { URI.protocol = protocol; path = path } = outfile in
> +    match protocol with
> +    | "" | "file" ->

Simpler to do:

   match outfile with
   | _, { URI.protocol = ("" | "file"); path = path } ->
> +      (* Because we used cache=unsafe when writing the output file, the
> +       * file might not be committed to disk.  This is a problem if qemu is
> +       * immediately used afterwards with cache=none (which uses O_DIRECT
> +       * and therefore bypasses the host cache).  In general you should not
> +       * use cache=none.
> +       *)
> +      Fsync.file path
> +    | _ -> () in

ACK with the changes above.

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