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