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

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.

On Mon, Oct 05, 2015 at 06:52:55PM +0300, Roman Kagan
wrote:
> 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?
It's like in C.  The following two if statements do different things:

  if foo then (
    stmt_1;
    stmt_2;
  )

  if foo then
    stmt_1;
    stmt_2;

The second one runs stmt_2 unconditionally.
> > > - 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?
Oh I see - yes it's broken upstream too :-(
(See the comment "XXX This won't work if paths contain
non-ASCII.")

Since String.lowercase assumes the string is ISO-8859-1, it could
actually corrupt UTF-8 paths.

As usual we're in a hard place here because we don't want to add
dependencies on external libraries like Camomile
(https://github.com/yoriyuki/Camomile) which would be the obvious
choice, but no one has written a working replacement for
mllib/common_utils.ml.

Whatever you do, make sure it's obvious that it's still broken and
needs to be fixed.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Mon, Oct 05, 2015 at 05:11:52PM +0100, Richard W.M. Jones
wrote:
> Oh I see - yes it's broken upstream too :-(
> (See the comment "XXX This won't work if paths contain
non-ASCII.")
> 
> Since String.lowercase assumes the string is ISO-8859-1, it could
> actually corrupt UTF-8 paths.
> 
> As usual we're in a hard place here because we don't want to add
> dependencies on external libraries like Camomile
> (https://github.com/yoriyuki/Camomile) which would be the obvious
> choice, but no one has written a working replacement for
> mllib/common_utils.ml.
If you have a look at the patch series I just posted:

https://www.redhat.com/archives/libguestfs/2015-October/msg00068.html

You can now use String.lowercase_ascii which is safe.

In fact you won't be able to use String.lowercase any longer since
that patch series hides the function.

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 Mon, Oct 05, 2015 at 05:11:52PM +0100, Richard W.M. Jones
wrote:
> On Mon, Oct 05, 2015 at 06:52:55PM +0300, Roman Kagan wrote:
> > 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?
> 
> It's like in C.  The following two if statements do different things:
> 
>   if foo then (
>     stmt_1;
>     stmt_2;
>   )
> 
>   if foo then
>     stmt_1;
>     stmt_2;
> 
> The second one runs stmt_2 unconditionally.
Looks right...  The problem is that the statement which runs
unconditionally is

    g#cp source target

where, in case the condition evaluates to false, "target" is
undefined,
and "source" is anything returned from g#find vio_root, including
directories.  I thought that would case an exception one way or another,
but it doesn't...
> > > > - 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?
> 
> Oh I see - yes it's broken upstream too :-(
> (See the comment "XXX This won't work if paths contain
non-ASCII.")
> 
> Since String.lowercase assumes the string is ISO-8859-1, it could
> actually corrupt UTF-8 paths.
> 
> As usual we're in a hard place here because we don't want to add
> dependencies on external libraries like Camomile
> (https://github.com/yoriyuki/Camomile) which would be the obvious
> choice, but no one has written a working replacement for
> mllib/common_utils.ml.
> 
> Whatever you do, make sure it's obvious that it's still broken and
> needs to be fixed.
The XXX comment was kept unmodified, and is now sitting at the top of
this function.  So I guess I can consider this concern addressed, can I?

I just came across another problem in the last patch of the series,
though: I accidentally configured libguestfs to use direct backend, and
immediately discovered that reusing the main guestfs handle doesn't work
with it due to the lack of support for hotplugging drives.  So I'll need
to reconsider it somehow (perhaps try with hotplug and fall back to
creating a new handle on exception).

Thanks,
Roman.