Libguestfs - Oct 2015 - Re: [PATCH 0/4] v2v: simplify driver copying from virtio-win iso

On Thu, Oct 01, 2015 at 06:04:03PM +0300, Roman Kagan
wrote:
> On Mon, Aug 10, 2015 at 06:55:28PM +0300, Roman Kagan wrote:
> > Libguestfs supports passing an ISO image as a source of virtio windows
> > drivers to v2v.
> > 
> > That support, however, looks too heavy-weight: in order to access
those
> > drivers, a separate guestfs handle is created (and thus a new emulator
> > process is started), which runs until v2v completes.
> > 
> > This series attempts to make it simpler and lighter-weight, by making
> > the relevant code more local, and by hot-adding the image into the
main
> > guestfs handle.
> > 
> > Roman Kagan (4):
> >   v2v: drop useless forced gc
> >   v2v: consolidate virtio-win file copying
> >   v2v: copy virtio drivers without guestfs handle leak
> >   v2v: reuse main guestfs for virtio win drivers iso
> > 
> >  v2v/convert_windows.ml | 184 ++++++++++++++++++++--------------------
> >  v2v/utils.ml           | 224
++++++++++++++++---------------------------------
> >  v2v/v2v.ml             |   8 --
> >  3 files changed, 163 insertions(+), 253 deletions(-)
> 
> I just realized that, although (at least two of) these patches got
> "likely ACK", they are not in the libguestfs tree.
> 
> The patches still apply to master as of today.
> 
> Is there anything I can do to get them merged?
Actually better to post them again so I can review them again.

I spotted a few problems

- There's a missing pair of parentheses around the then clause in:

         g2#mount_ro "/dev/sda" vio_root;
         let paths = g2#find vio_root in
         Array.iter (
           fun path ->
             let source = vio_root // path in
             if ((g2#is_file source ~followsymlinks:false) &&
                 (match_vio_path_with_os path inspect.i_arch
                     inspect.i_major_version inspect.i_minor_version
                     inspect.i_product_variant)) then
                                                     ^^^

  which makes me think this code can't possibly work.

- Calling String.lowercase is unsafe (because the function is just
  broken in OCaml) and unnecessary.  Just remove it.

- (Stylistic) You don't need parentheses around if conditionals.

- (Stylistic) You don't need parentheses around the arguments of &&

- (Stylistic) You don't need parentheses around the arguments of //

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 Thu, Oct 01, 2015 at 04:22:14PM +0100, Richard W.M. Jones
wrote:
> On Thu, Oct 01, 2015 at 06:04:03PM +0300, Roman Kagan wrote:
> > On Mon, Aug 10, 2015 at 06:55:28PM +0300, Roman Kagan wrote:
> > > Libguestfs supports passing an ISO image as a source of virtio
windows
> > > drivers to v2v.
> > > 
> > > That support, however, looks too heavy-weight: in order to access
those
> > > drivers, a separate guestfs handle is created (and thus a new
emulator
> > > process is started), which runs until v2v completes.
> > > 
> > > This series attempts to make it simpler and lighter-weight, by
making
> > > the relevant code more local, and by hot-adding the image into
the main
> > > guestfs handle.
> > > 
> > > Roman Kagan (4):
> > >   v2v: drop useless forced gc
> > >   v2v: consolidate virtio-win file copying
> > >   v2v: copy virtio drivers without guestfs handle leak
> > >   v2v: reuse main guestfs for virtio win drivers iso
> > > 
> > >  v2v/convert_windows.ml | 184
++++++++++++++++++++--------------------
> > >  v2v/utils.ml           | 224
++++++++++++++++---------------------------------
> > >  v2v/v2v.ml             |   8 --
> > >  3 files changed, 163 insertions(+), 253 deletions(-)
> > 
> > I just realized that, although (at least two of) these patches got
> > "likely ACK", they are not in the libguestfs tree.
> > 
> > The patches still apply to master as of today.
> > 
> > Is there anything I can do to get them merged?
> 
> Actually better to post them again so I can review them again.
OK will do.
> I spotted a few problems
> 
> - There's a missing pair of parentheses around the then clause in:
> 
>          g2#mount_ro "/dev/sda" vio_root;
>          let paths = g2#find vio_root in
>          Array.iter (
>            fun path ->
>              let source = vio_root // path in
>              if ((g2#is_file source ~followsymlinks:false) &&
>                  (match_vio_path_with_os path inspect.i_arch
>                      inspect.i_major_version inspect.i_minor_version
>                      inspect.i_product_variant)) then
>                                                      ^^^
> 
>   which makes me think this code can't possibly work.
Indeed.  It failed neither in compilation nor in tests, though.  Looks
like I need to patch v2v/test-v2v-windows-conversion.sh, too.

Roman.

On Thu, Oct 01, 2015 at 07:09:02PM +0300, Roman Kagan
wrote:
> On Thu, Oct 01, 2015 at 04:22:14PM +0100, Richard W.M. Jones wrote:
> > On Thu, Oct 01, 2015 at 06:04:03PM +0300, Roman Kagan wrote:
> > > On Mon, Aug 10, 2015 at 06:55:28PM +0300, Roman Kagan wrote:
> > > > Libguestfs supports passing an ISO image as a source of
virtio windows
> > > > drivers to v2v.
> > > > 
> > > > That support, however, looks too heavy-weight: in order to
access those
> > > > drivers, a separate guestfs handle is created (and thus a
new emulator
> > > > process is started), which runs until v2v completes.
> > > > 
> > > > This series attempts to make it simpler and lighter-weight,
by making
> > > > the relevant code more local, and by hot-adding the image
into the main
> > > > guestfs handle.
> > > > 
> > > > Roman Kagan (4):
> > > >   v2v: drop useless forced gc
> > > >   v2v: consolidate virtio-win file copying
> > > >   v2v: copy virtio drivers without guestfs handle leak
> > > >   v2v: reuse main guestfs for virtio win drivers iso
> > > > 
> > > >  v2v/convert_windows.ml | 184
++++++++++++++++++++--------------------
> > > >  v2v/utils.ml           | 224
++++++++++++++++---------------------------------
> > > >  v2v/v2v.ml             |   8 --
> > > >  3 files changed, 163 insertions(+), 253 deletions(-)
> > > 
> > > I just realized that, although (at least two of) these patches
got
> > > "likely ACK", they are not in the libguestfs tree.
> > > 
> > > The patches still apply to master as of today.
> > > 
> > > Is there anything I can do to get them merged?
> > 
> > Actually better to post them again so I can review them again.
> 
> OK will do.
> 
> > I spotted a few problems
> > 
> > - There's a missing pair of parentheses around the then clause in:
> > 
> >          g2#mount_ro "/dev/sda" vio_root;
> >          let paths = g2#find vio_root in
> >          Array.iter (
> >            fun path ->
> >              let source = vio_root // path in
> >              if ((g2#is_file source ~followsymlinks:false) &&
> >                  (match_vio_path_with_os path inspect.i_arch
> >                      inspect.i_major_version inspect.i_minor_version
> >                      inspect.i_product_variant)) then
> >                                                      ^^^
> > 
> >   which makes me think this code can't possibly work.
> 
> Indeed.  It failed neither in compilation nor in tests, though.  Looks
> like I need to patch v2v/test-v2v-windows-conversion.sh, too.
When I replied I didn't already remember for certain how I verified that
the code did achieve its goal; however now that I have a test for it
(submitted today, needs fixing the stylistic comments but working
otherwise) I claim that it passes with this code, i.e. all the drivers
do get copied into the guest upon conversion.

So this code does work as intended; being totally clueless in OCaml I'd
appreciate being explained -- how?

> > - Calling String.lowercase is unsafe (because the function is just
> >   broken in OCaml) and unnecessary.  Just remove it.
I can't figure out why it's unnecessary and how to remove it.  The only
place where String.lowercase is called is in match_vio_path_with_os()
which is basically unchanged in the patch, it's only factored out into a
separate function.  It tries to do case-insensitive comparison of the
path elements to constant strings; is there a way to do it other than
downcasing both sides of the comparison?

Thanks,
Roman.