Libguestfs - Mar 2023 - [V2V PATCH 0/5] Bring support for virtio-scsi back to Windows

On Thu, Mar 02, 2023 at 08:52:33PM +0200, Andrey Drobyshev
wrote:
> On 3/2/23 20:36, Richard W.M. Jones wrote:
> > I can probably do an outline of a patch if you need it, but
> > not today.
> 
> Yes, sure.  But speaking of the ways to influence the behaviour of a
> command line tool: there're also env variables, which are being used by
> v2v as well (for instance, for setting the tools ISO location).  So what
> if we introduce an env variable here instead?  I presume it would be
> much easier to implement.  Smth like:
The environment variable is a bit of a mistake too.  It'd be better to
specify that through the command line; and in fact that's what I went
with for the new virt-customize --inject-virtio-win option:

       --inject-virtio-win METHOD
           Inject virtio-win drivers into a Windows guest.  These drivers add
           virtio accelerated drivers suitable when running on top of a
           hypervisor that supports virtio (such as qemu/KVM).  The operation
           also adjusts the Windows Registry so that the drivers are installed
           when the guest boots.

           The parameter can be one of:

           ISO The path to the ISO image containing the virtio-win drivers
               (eg. /usr/share/virtio-win/virtio-win.iso).
   [etc]
> > diff --git a/mlcustomize/inject_virtio_win.ml
b/mlcustomize/inject_virtio_win.ml
> > index 62f7710..562f055 100644
> > --- a/mlcustomize/inject_virtio_win.ml
> > +++ b/mlcustomize/inject_virtio_win.ml
> > @@ -177,6 +177,11 @@ let rec inject_virtio_win_drivers ({ g } as t)
reg > >      (* Can we install the block driver? *)
> >      let block : block_type > >        let filenames =
["virtio_blk"; "vrtioblk"; "viostor"] in
> > +      let viostor_drv_env = Sys.getenv_opt "VIOSTOR_DRIVER"
in
> > +      let filenames > > +        match viostor_drv_env with
> > +        | None -> filenames
> > +        | Some str -> str :: filenames in
> >        let viostor_driver = try (
> >          Some (
> >            List.find (
> 
> As you can see here the default doesn't change if the variable
isn't
> provided at all (or it is, but is set to a non-existing driver).
> What do you think of such a solution?
I prefer the command line option even though it's a little bit more
work.  Offer above still stands if you can't work out the complexities
of OCaml command line parsing.

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 3/2/23 22:59, Richard W.M. Jones wrote:
> On Thu, Mar 02, 2023 at 08:52:33PM +0200, Andrey Drobyshev wrote:
>> On 3/2/23 20:36, Richard W.M. Jones wrote:
>>> I can probably do an outline of a patch if you need it, but
>>> not today.
>>
>> Yes, sure.  But speaking of the ways to influence the behaviour of a
>> command line tool: there're also env variables, which are being
used by
>> v2v as well (for instance, for setting the tools ISO location).  So
what
>> if we introduce an env variable here instead?  I presume it would be
>> much easier to implement.  Smth like:
> 
> The environment variable is a bit of a mistake too.  It'd be better to
> specify that through the command line; and in fact that's what I went
> with for the new virt-customize --inject-virtio-win option:
> 
>        --inject-virtio-win METHOD
>            Inject virtio-win drivers into a Windows guest.  These drivers
add
>            virtio accelerated drivers suitable when running on top of a
>            hypervisor that supports virtio (such as qemu/KVM).  The
operation
>            also adjusts the Windows Registry so that the drivers are
installed
>            when the guest boots.
> 
>            The parameter can be one of:
> 
>            ISO The path to the ISO image containing the virtio-win drivers
>                (eg. /usr/share/virtio-win/virtio-win.iso).
>    [etc]
> 
>>> diff --git a/mlcustomize/inject_virtio_win.ml
b/mlcustomize/inject_virtio_win.ml
>>> index 62f7710..562f055 100644
>>> --- a/mlcustomize/inject_virtio_win.ml
>>> +++ b/mlcustomize/inject_virtio_win.ml
>>> @@ -177,6 +177,11 @@ let rec inject_virtio_win_drivers ({ g } as t)
reg >>>      (* Can we install the block driver? *)
>>>      let block : block_type >>>        let filenames =
["virtio_blk"; "vrtioblk"; "viostor"] in
>>> +      let viostor_drv_env = Sys.getenv_opt
"VIOSTOR_DRIVER" in
>>> +      let filenames >>> +        match viostor_drv_env
with
>>> +        | None -> filenames
>>> +        | Some str -> str :: filenames in
>>>        let viostor_driver = try (
>>>          Some (
>>>            List.find (
>>
>> As you can see here the default doesn't change if the variable
isn't
>> provided at all (or it is, but is set to a non-existing driver).
>> What do you think of such a solution?
> 
> I prefer the command line option even though it's a little bit more
> work.  Offer above still stands if you can't work out the complexities
> of OCaml command line parsing.
It's not about the command line parsing complexities, but rather about
the proper way to pass that option from the top level of the call stack
(convert ()) down to the inject_virtio_win_drivers ().  That'd require
adding one more extra argument to all the calls along this call stack,
which might look a bit clumsy.  Isn't there a more elegant way?

So having a sketch in that regard would be indeed much appreciated.
> 
> Rich.
>