Libguestfs - Jan 2015 - Re: [PATCH] v2v: -o vdsm should assume data domain at -os path

On 20.01.15 12:59, Richard W.M. Jones wrote:
> On Tue, Jan 20, 2015 at 02:53:43PM +0200, Shahar Havivi wrote:
> 
> > >From 87f448fc8b3243ad5d01acb65efb0c710d811a93 Mon Sep 17 00:00:00
2001
> > From: Shahar Havivi <shaharh@redhat.com>
> > Date: Tue, 20 Jan 2015 14:41:23 +0200
> > Subject: [PATCH] v2v: -o vdsm should assume data domain at -os path
> > 
> > Unlike -o rhev which have only one data domin, -o rhev can and usually
> 
>                                                  ^^ vdsm?
> 
> > does have multiple data domain.
> > The path to vdsm is pre mounted so no need to assume nfs path with -os
> > Example:
> > -o vdsm -os
/rhev/data-center/<data-center-uuid>/<data-doamin-uuid>
> 
> s/doamin/domain/
> 
> > 
> > Bug-Url: https://bugzilla.redhat.com/1176591
> > Signed-off-by: Shahar Havivi <shaharh@redhat.com>
> > ---
> >  v2v/output_vdsm.ml | 10 +++++-----
> >  v2v/virt-v2v.pod   |  3 ++-
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/v2v/output_vdsm.ml b/v2v/output_vdsm.ml
> > index 492f586..17f10a5 100644
> > --- a/v2v/output_vdsm.ml
> > +++ b/v2v/output_vdsm.ml
> > @@ -63,9 +63,8 @@ object
> >     * name of the target files that eventually get written by the main
> >     * code.
> >     *
> > -   * 'os' is the output storage (-os nfs:/export). 
'source' contains a
> > -   * few useful fields such as the guest name.  'targets'
describes the
> > -   * destination files.  We modify and return this list.
> > +   * 'os' is the output storage domain (-os
/rhev/data/<data center>/<data domain>)
> > +   * this is already mounted path.
> >     *
> >     * Note it's good to fail here (early) if there are any
problems, since
> >     * the next time we are called (in {!create_metadata}) we have
already
> > @@ -78,8 +77,9 @@ object
> >        error (f_"the number of '--vdsm-image-uuid' and
'--vdsm-vol-uuid' parameters passed on the command line has to match the
number of guest disk images (for this guest: %d)")
> >          (List.length targets);
> >  
> > -    let mp, uuid > > -     
Output_rhev.mount_and_check_storage_domain verbose (s_"Data Domain")
os in
> > +    let pos = (String.length os) - 36 in
> > +    let mp = String.sub os 0 (pos-1) in
> > +    let uuid = String.sub os pos 36 in
> 
> What is the significance of 36 here?
the length of uuid.
> 
> >      dd_mp <- mp;
> >      dd_uuid <- uuid;
> >      if verbose then
> > diff --git a/v2v/virt-v2v.pod b/v2v/virt-v2v.pod
> > index a48bf59..cfd55fc 100644
> > --- a/v2v/virt-v2v.pod
> > +++ b/v2v/virt-v2v.pod
> > @@ -456,7 +456,8 @@ See L</OUTPUT TO RHEV> below.
> >  
> >  Set the output method to I<vdsm>.
> >  
> > -This mode is similar to I<-o rhev> but is only used by RHEV
VDSM
> > +This mode is similar to I<-o rhev> but assume data domain as
well
> > +as data center
/rhev/data-center/<data-center-uuid>/<data-domain-uuid>
> >  when it runs virt-v2v under VDSM control.
> 
> You can't use < and > in POD.
> 
> Probably better to phrase this as:
> 
>   This mode is similar to I<-o rhev>, but the full path to the
>   data domain must be given:
>  
C</rhev/data-center/E<lt>data-center-uuidE<gt>/E<lt>data-domain-uuidE<gt>>.
>   This mode is only used when virt-v2v runs under VDSM control.
> 
> Rich.
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-builder quickly builds VMs from scratch
> http://libguestfs.org/virt-builder.1.html
Sure

On Tue, Jan 20, 2015 at 04:25:20PM +0200, Shahar Havivi
wrote:
> > > +    let pos = (String.length os) - 36 in
> > > +    let mp = String.sub os 0 (pos-1) in
> > > +    let uuid = String.sub os pos 36 in
> > 
> > What is the significance of 36 here?
> the length of uuid.
That code above will throw an exception if you pass a too-short -os
parameter.

How about this, which should be a little bit easier to understand:

let mp, uuid   let fields = string_nsplit "/" os in (* ...
"data-center" "UUID" *)
  let fields = List.rev fields in      (* "UUID"
"data-center" ... *)
  match fields with
  | "" :: uuid :: rest                 (* handles trailing
"/" case *)
  | uuid :: rest
      when String.length uuid = 36 ->
    let mp = String.concat "/" (List.rev rest) in
    mp, uuid
  | _ ->
    error (f_"vdsm: invalid -os parameter does not contain a valid UUID:
%s")
      os

Or perhaps something with regexps ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On 20.01.15 14:48, Richard W.M. Jones wrote:
> On Tue, Jan 20, 2015 at 04:25:20PM +0200, Shahar Havivi wrote:
> > > > +    let pos = (String.length os) - 36 in
> > > > +    let mp = String.sub os 0 (pos-1) in
> > > > +    let uuid = String.sub os pos 36 in
> > > 
> > > What is the significance of 36 here?
> > the length of uuid.
> 
> That code above will throw an exception if you pass a too-short -os
> parameter.
> 
> How about this, which should be a little bit easier to understand:
> 
> let mp, uuid >   let fields = string_nsplit "/" os in (* ...
"data-center" "UUID" *)
>   let fields = List.rev fields in      (* "UUID"
"data-center" ... *)
>   match fields with
>   | "" :: uuid :: rest                 (* handles trailing
"/" case *)
>   | uuid :: rest
>       when String.length uuid = 36 ->
>     let mp = String.concat "/" (List.rev rest) in
>     mp, uuid
>   | _ ->
>     error (f_"vdsm: invalid -os parameter does not contain a valid
UUID: %s")
>       os
> 
Looks better, I will give it a try
> Or perhaps something with regexps ...
> 
> Rich.
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> libguestfs lets you edit virtual machines.  Supports shell scripting,
> bindings from many languages.  http://libguestfs.org