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

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
+
     infile, outfile, align_first, alignment, copy_boot_loader,
     deletes,
     dryrun, expand, expand_content, extra_partition, format, ignores,
@@ -333,9 +340,12 @@ read the man page virt-resize(1).
              server = server; username = username;
              password = password } = infile in
     g#add_drive ?format ~readonly:true ~protocol ?server ?username
?secret:password path;
+    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 ();
 
@@ -368,7 +378,7 @@ read the man page virt-resize(1).
     let insize = g#blockdev_getsize64 "/dev/sda" in
     let outsize = g#blockdev_getsize64 "/dev/sdb" in
     debug "%s size %Ld bytes" (fst infile) insize;
-    debug "%s size %Ld bytes" outfile outsize;
+    debug "%s size %Ld bytes" (fst outfile) outsize;
     sectsize, insize, outsize in
 
   let max_bootloader @@ -390,7 +400,7 @@ read the man page virt-resize(1).
       (fst infile) insize;
   if outsize < Int64.of_int max_bootloader then
     error (f_"%s: file is too small to be a disk image (%Ld bytes)")
-      outfile outsize;
+      (fst outfile) outsize;
 
   (* Get the source partition type. *)
   let parttype, parttype_string @@ -983,7 +993,7 @@ read the man page
virt-resize(1).
     (* Try hard to initialize the partition table.  This might involve
      * relaunching another handle.
      *)
-    message (f_"Setting up initial partition table on %s") outfile;
+    message (f_"Setting up initial partition table on %s") (fst
outfile);
 
     let last_error = ref "" in
     let rec initialize_partition_table g attempts @@ -1300,9 +1310,12 @@ read
the man page virt-resize(1).
       g#close ();
 
       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 ();
 
@@ -1378,13 +1391,19 @@ read the man page virt-resize(1).
   g#shutdown ();
   g#close ();
 
-  (* 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 outfile;
+  (* 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" ->
+      (* 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
 
   if not (quiet ()) then (
     print_newline ();
diff --git a/resize/virt-resize.pod b/resize/virt-resize.pod
index 2344056..98c4b10 100644
--- a/resize/virt-resize.pod
+++ b/resize/virt-resize.pod
@@ -121,9 +121,9 @@ Or use L<virsh(1)> vol-create-as to create a libvirt
storage volume:
 
 =item 5. Resize
 
-virt-resize takes two mandatory parameters, the input disk
-(eg. device, file, or a URI to a remote disk) and the output disk.
-The output disk is the one created in the previous step.
+virt-resize takes two mandatory parameters, the input disk and the
+output disk (both can be e.g. a device, a file, or a URI to a remote
+disk).  The output disk is the one created in the previous step.
 
  # virt-resize indisk outdisk
 
-- 
2.9.3

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

On Friday, 3 February 2017 15:27:42 CET Richard W.M. Jones
wrote:
> 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.
I am not sure about that -- IMHO it is much better to tell the user
right away that the URI specified is not valid, and thus virt-resize
(and the other tools too) cannot do anything with them.  My gut feeling
is that trying to use the invalid URI as local file might do more harm
than good -- after all, local files (absolute and relative paths) are
supported just fine already.
> 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
Indeed -- I was trying to use currying here, but with labelled arguments
it is kind of messy and overly complicated (for not much as real
benefit).

Thanks,
-- 
Pino Toscano