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

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.