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

[Pino Toscano]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170206/0fc9f954/attachment.sig>