Libguestfs - Mar 2023 - [PATCH v2v] convert: Allow preferred block driver to be specified on the command line

On 3/6/23 17:52, Richard W.M. Jones wrote:
> diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
> index 9d8d271d05..f36b486359 100644
> --- a/convert/convert_windows.ml
> +++ b/convert/convert_windows.ml
> @@ -38,7 +38,7 @@ module G = Guestfs
>   * time the Windows VM is booted on KVM.
>   *)
>  
> -let convert (g : G.guestfs) _ inspect i_firmware _ static_ips > +let
convert (g : G.guestfs) _ inspect i_firmware block_driver _ static_ips >   
(*----------------------------------------------------------------------*)
>    (* Inspect the Windows guest. *)
>  
> @@ -47,6 +47,16 @@ let convert (g : G.guestfs) _ inspect i_firmware _
static_ips >     *)
>    let virtio_win >      Inject_virtio_win.from_environment g
inspect.i_root Config.datadir in
> +  (match block_driver with
> +   | Virtio_blk -> () (* the default, no need to do anything *)
> +(*
> +   | Virtio_scsi ->
> +      let drivers = Inject_virtio_win.get_block_driver_priority virtio_win
in
> +      let drivers = "vioscsi" :: drivers in
> +      Inject_virtio_win.set_block_driver_priority virtio_win drivers
> +*)
> +   | IDE -> assert false (* not possible - but maybe ...? *)
> +  );
>  
>    (* If the Windows guest appears to be using group policy.
>     *
So ["virtio_blk"; "vrtioblk"; "viostor"] in
"common/mlcustomize/inject_virtio_win.ml" would be replaced with a
reference cell, and get_block_driver_priority /
set_block_driver_priority would manipulate that.

This looks OK to me.

That said, I'd prefer that this patch be split up a bit (by Andrey).
Patch#1 could introduce the new field and make sure it is passed around
(incl. setting the default virtio-blk value); that's purely boilerplate,
so the patch would be well-focused. Patch #2 could glue the new field to
the new Inject_virtio_win APIs, in "convert_windows.ml". Patch#3 could
implement the command line changes and update the documentation.

Laszlo

On Tue, Mar 07, 2023 at 08:48:56AM +0100, Laszlo Ersek
wrote:
> On 3/6/23 17:52, Richard W.M. Jones wrote:
> 
> > diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
> > index 9d8d271d05..f36b486359 100644
> > --- a/convert/convert_windows.ml
> > +++ b/convert/convert_windows.ml
> > @@ -38,7 +38,7 @@ module G = Guestfs
> >   * time the Windows VM is booted on KVM.
> >   *)
> >  
> > -let convert (g : G.guestfs) _ inspect i_firmware _ static_ips >
> +let convert (g : G.guestfs) _ inspect i_firmware block_driver _ static_ips
> >   
(*----------------------------------------------------------------------*)
> >    (* Inspect the Windows guest. *)
> >  
> > @@ -47,6 +47,16 @@ let convert (g : G.guestfs) _ inspect i_firmware _
static_ips > >     *)
> >    let virtio_win > >      Inject_virtio_win.from_environment g
inspect.i_root Config.datadir in
> > +  (match block_driver with
> > +   | Virtio_blk -> () (* the default, no need to do anything *)
> > +(*
> > +   | Virtio_scsi ->
> > +      let drivers = Inject_virtio_win.get_block_driver_priority
virtio_win in
> > +      let drivers = "vioscsi" :: drivers in
> > +      Inject_virtio_win.set_block_driver_priority virtio_win drivers
> > +*)
> > +   | IDE -> assert false (* not possible - but maybe ...? *)
> > +  );
> >  
> >    (* If the Windows guest appears to be using group policy.
> >     *
> 
> So ["virtio_blk"; "vrtioblk"; "viostor"] in
> "common/mlcustomize/inject_virtio_win.ml" would be replaced with
a
> reference cell, and get_block_driver_priority /
> set_block_driver_priority would manipulate that.
References are really just structures with mutable fields and some
syntactic sugar:

https://github.com/ocaml/ocaml/blob/66db964f48869c6470b8ccd1ed672e244ba9d680/stdlib/stdlib.ml#L237

The reason mutable fields of structures in general have to be
specially marked with the "mutable" keyword is because of the
"remembered set", an obscure corner of generational garbage
collection:

https://www.cs.cornell.edu/courses/cs3110/2012sp/lectures/lec26-gc/lec26.html

If mutable fields didn't exist then you could never have a pointer
from the old generation into the new generation, because new
generation objects are always younger than old generation objects, and
as all fields have to be set at object creation time (no mutation),
there cannot exist pointers from old to new.

Since mutable fields _do_ exist, it's possible for such pointers to
exist.  When you do a minor GC of the younger generation, to avoid
having to scan over the whole of the old heap to find these pointers,
when a mutable field is mutated a record is added to the "remembered
set", which is treated as an additional set of roots for the minor GC.

After each minor GC the remembered set can be cleared.  But there's
still some overhead here, so marking fields in a struct as mutable is
not cost free.  (Of course in this specific case the cost is
infinitesimally small, but in general it can be a concern.)

This is a long way of saying that it's not a reference cell, but a
plain field in the 't' struct, with extra runtime overhead and code
generated for when it is mutated.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v