Libguestfs - Mar 2023 - [V2V PATCH 1/1] convert_windows: add firstboot script to install drivers with pnputil

On Thu, Mar 09, 2023 at 03:47:10PM +0200, Andrey Drobyshev
wrote:
> On 3/8/23 22:52, Richard W.M. Jones wrote:
> > On Wed, Mar 08, 2023 at 08:05:35PM +0200, Andrey Drobyshev wrote:
> >> During conversion we copy the necessary drivers to the directory
> >> "%systemroot%\Drivers\Virtio", adding it to the
DevicePath registry
> >> value.  As documented in [1], this should be enough for Windows to
find
> >> device drivers and successfully install them.
> >>
> >> However, it doesn't always happen.  Commit 73e009c04
("v2v: windows:
> >> Document use of pnputil to install drivers.") describes such
issues with
> >> Win2k12R2.  I'm seeing the same problem with Win2k16 and
netkvm.sys
> >> driver not being installed.
> >>
> >> That same commit 73e009c04 suggests adding a firstboot script
invoking
> >> pnputil at an early stage to install all the drivers we put into
the
> >> drivers store.  So let's add such a script to make sure all
the
> >> necessary drivers are installed.
> >>
> >> [1]
https://learn.microsoft.com/en-us/windows-hardware/drivers/install/how-windows-selects-a-driver-for-a-device
> >>
> >> Signed-off-by: Andrey Drobyshev <andrey.drobyshev at
virtuozzo.com>
> >> ---
> >>  convert/convert_windows.ml | 16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/convert/convert_windows.ml
b/convert/convert_windows.ml
> >> index 6bc2343b..e15a5e62 100644
> >> --- a/convert/convert_windows.ml
> >> +++ b/convert/convert_windows.ml
> >> @@ -295,9 +295,11 @@ let convert (g : G.guestfs) _ inspect
i_firmware block_driver _ static_ips > >>      | Virt -> Virt
> >>  
> >>    and configure_firstboot () > >> -    (* Note that
pnp_wait.exe must be the first firstboot script as it
> >> -     * suppresses PnP for all following scripts.
> >> +    (* Run the firstboot script with pnputil.exe before the one
with
> >> +     * pnp_wait.exe as the latter suppresses PnP for all
following scripts.
> >>       *)
> >> +    configure_pnputil_install ();
> >> +
> >>      let tool_path = virt_tools_data_dir () //
"pnp_wait.exe" in
> >>      if Sys.file_exists tool_path then
> >>        configure_wait_pnp tool_path
> >> @@ -345,6 +347,16 @@ let convert (g : G.guestfs) _ inspect
i_firmware block_driver _ static_ips > >>                        strkey
name value
> >>      | None -> sprintf "reg delete \"%s\" /v %s
/f" strkey name
> >>  
> >> +  and configure_pnputil_install () > >> +    let
fb_script = "@echo off\n\
> >> +                     \n\
> >> +                     echo Wait for VirtIO drivers to be
installed\n\
> >> +                     %systemroot%\\Sysnative\\PnPutil -i -a \
> >> +                     %systemroot%\\Drivers\\Virtio\\*.inf
>\"%~dpn0.log\" 2>&1\
> >> +                    " in
> >> +    Firstboot.add_firstboot_script g inspect.i_root
> >> +      "pnputil install drivers" fb_script;
> >> +
> > 
> > I'm not sure I'm really qualified to comment on this, since
Windows is
> > crazy.  I guess the worst this can do is fail to run, but that
won't
> > stop anything else from happening.
> > 
> > Note that firstboot scripts already do logging, so I don't believe
> > writing to a separate log file is needed here.  All the output from
> > all firstboot scripts should go to
> > C:\Program Files\Guestfs\Firstboot\log.txt:
> > 
> >
https://github.com/libguestfs/libguestfs-common/blob/7acf991a25b3fd625eb1ff1fbd8dc9fedf245942/mlcustomize/firstboot.ml#L276
> 
> Right, but apart from having the log common for all firstboot scripts,
> some of them also utilise separate log files in scripts-done directory,
> e.g.:
> 
>
https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_windows.ml#L381
>
https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_windows.ml#L168
> 
> So I did the same considering that pnputil's output is relatively long.
I think I'd probably get rid of those other scripts too.  It's helpful
to have everything go to one place.  I'm not actually sure where those
extra files end up.
> All in all, if there're no other concerns, can we give this script a
go?
I think the patch is generally fine, so I'm just quibbling about
the logging.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit

On 3/9/23 15:48, Richard W.M. Jones wrote:
> On Thu, Mar 09, 2023 at 03:47:10PM +0200, Andrey Drobyshev wrote:
>> On 3/8/23 22:52, Richard W.M. Jones wrote:
>>> On Wed, Mar 08, 2023 at 08:05:35PM +0200, Andrey Drobyshev wrote:
>>>> During conversion we copy the necessary drivers to the
directory
>>>> "%systemroot%\Drivers\Virtio", adding it to the
DevicePath registry
>>>> value.  As documented in [1], this should be enough for Windows
to find
>>>> device drivers and successfully install them.
>>>>
>>>> However, it doesn't always happen.  Commit 73e009c04
("v2v: windows:
>>>> Document use of pnputil to install drivers.") describes
such issues with
>>>> Win2k12R2.  I'm seeing the same problem with Win2k16 and
netkvm.sys
>>>> driver not being installed.
>>>>
>>>> That same commit 73e009c04 suggests adding a firstboot script
invoking
>>>> pnputil at an early stage to install all the drivers we put
into the
>>>> drivers store.  So let's add such a script to make sure all
the
>>>> necessary drivers are installed.
>>>>
>>>> [1]
https://learn.microsoft.com/en-us/windows-hardware/drivers/install/how-windows-selects-a-driver-for-a-device
>>>>
>>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev at
virtuozzo.com>
>>>> ---
>>>>  convert/convert_windows.ml | 16 ++++++++++++++--
>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/convert/convert_windows.ml
b/convert/convert_windows.ml
>>>> index 6bc2343b..e15a5e62 100644
>>>> --- a/convert/convert_windows.ml
>>>> +++ b/convert/convert_windows.ml
>>>> @@ -295,9 +295,11 @@ let convert (g : G.guestfs) _ inspect
i_firmware block_driver _ static_ips >>>>      | Virt -> Virt
>>>>  
>>>>    and configure_firstboot () >>>> -    (* Note
that pnp_wait.exe must be the first firstboot script as it
>>>> -     * suppresses PnP for all following scripts.
>>>> +    (* Run the firstboot script with pnputil.exe before the
one with
>>>> +     * pnp_wait.exe as the latter suppresses PnP for all
following scripts.
>>>>       *)
>>>> +    configure_pnputil_install ();
>>>> +
>>>>      let tool_path = virt_tools_data_dir () //
"pnp_wait.exe" in
>>>>      if Sys.file_exists tool_path then
>>>>        configure_wait_pnp tool_path
>>>> @@ -345,6 +347,16 @@ let convert (g : G.guestfs) _ inspect
i_firmware block_driver _ static_ips >>>>                       
strkey name value
>>>>      | None -> sprintf "reg delete \"%s\" /v
%s /f" strkey name
>>>>  
>>>> +  and configure_pnputil_install () >>>> +    let
fb_script = "@echo off\n\
>>>> +                     \n\
>>>> +                     echo Wait for VirtIO drivers to be
installed\n\
>>>> +                     %systemroot%\\Sysnative\\PnPutil -i -a \
>>>> +                     %systemroot%\\Drivers\\Virtio\\*.inf
>\"%~dpn0.log\" 2>&1\
>>>> +                    " in
>>>> +    Firstboot.add_firstboot_script g inspect.i_root
>>>> +      "pnputil install drivers" fb_script;
>>>> +
>>>
>>> I'm not sure I'm really qualified to comment on this, since
Windows is
>>> crazy.  I guess the worst this can do is fail to run, but that
won't
>>> stop anything else from happening.
>>>
>>> Note that firstboot scripts already do logging, so I don't
believe
>>> writing to a separate log file is needed here.  All the output from
>>> all firstboot scripts should go to
>>> C:\Program Files\Guestfs\Firstboot\log.txt:
>>>
>>>
https://github.com/libguestfs/libguestfs-common/blob/7acf991a25b3fd625eb1ff1fbd8dc9fedf245942/mlcustomize/firstboot.ml#L276
>>
>> Right, but apart from having the log common for all firstboot scripts,
>> some of them also utilise separate log files in scripts-done directory,
>> e.g.:
>>
>>
https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_windows.ml#L381
>>
https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_windows.ml#L168
>>
>> So I did the same considering that pnputil's output is relatively
long.
> 
> I think I'd probably get rid of those other scripts too.  It's
helpful
> to have everything go to one place.  I'm not actually sure where those
> extra files end up.
The common log is set to %%firstboot%%\\log.txt, with %%firstboot%%
being C:\Program Files\Guestfs\Firstboot:

https://github.com/libguestfs/libguestfs-common/blob/7acf991a25/mlcustomize/firstboot.ml#L285

The scripts are given a priority and a relative number, resulting in a
prefix like "5000-0001-":, and put into %%firstboot%%\\scripts:

https://github.com/libguestfs/libguestfs-common/blob/7acf991a25/mlcustomize/firstboot.ml#L356

The script is moved from scripts to scripts-done right before getting run:

https://github.com/libguestfs/libguestfs-common/blob/7acf991a25/mlcustomize/firstboot.ml#L302

And "%~dpn0" in the script evaluates to Drive+Path+Name of the script
itself (apparently, with no .bat extension):

https://stackoverflow.com/a/112135

This results in having both the script
5000-0001-pnputil-install-drivers.bat and the log
5000-0001-pnputil-install-drivers.log ending up in C:\Program
Files\Guestfs\Firstboot\scripts-done.
> 
>> All in all, if there're no other concerns, can we give this script
a go?
> 
> I think the patch is generally fine, so I'm just quibbling about
> the logging.
Alright, then let me send v2 with the logging part fixed.
> 
> Rich.
>

On 3/9/23 14:48, Richard W.M. Jones wrote:
> On Thu, Mar 09, 2023 at 03:47:10PM +0200, Andrey Drobyshev wrote:
>> On 3/8/23 22:52, Richard W.M. Jones wrote:
>>> On Wed, Mar 08, 2023 at 08:05:35PM +0200, Andrey Drobyshev wrote:
>>>> During conversion we copy the necessary drivers to the
directory
>>>> "%systemroot%\Drivers\Virtio", adding it to the
DevicePath registry
>>>> value.  As documented in [1], this should be enough for Windows
to find
>>>> device drivers and successfully install them.
>>>>
>>>> However, it doesn't always happen.  Commit 73e009c04
("v2v: windows:
>>>> Document use of pnputil to install drivers.") describes
such issues with
>>>> Win2k12R2.  I'm seeing the same problem with Win2k16 and
netkvm.sys
>>>> driver not being installed.
>>>>
>>>> That same commit 73e009c04 suggests adding a firstboot script
invoking
>>>> pnputil at an early stage to install all the drivers we put
into the
>>>> drivers store.  So let's add such a script to make sure all
the
>>>> necessary drivers are installed.
>>>>
>>>> [1]
https://learn.microsoft.com/en-us/windows-hardware/drivers/install/how-windows-selects-a-driver-for-a-device
>>>>
>>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev at
virtuozzo.com>
>>>> ---
>>>>  convert/convert_windows.ml | 16 ++++++++++++++--
>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/convert/convert_windows.ml
b/convert/convert_windows.ml
>>>> index 6bc2343b..e15a5e62 100644
>>>> --- a/convert/convert_windows.ml
>>>> +++ b/convert/convert_windows.ml
>>>> @@ -295,9 +295,11 @@ let convert (g : G.guestfs) _ inspect
i_firmware block_driver _ static_ips >>>>      | Virt -> Virt
>>>>  
>>>>    and configure_firstboot () >>>> -    (* Note
that pnp_wait.exe must be the first firstboot script as it
>>>> -     * suppresses PnP for all following scripts.
>>>> +    (* Run the firstboot script with pnputil.exe before the
one with
>>>> +     * pnp_wait.exe as the latter suppresses PnP for all
following scripts.
>>>>       *)
>>>> +    configure_pnputil_install ();
>>>> +
>>>>      let tool_path = virt_tools_data_dir () //
"pnp_wait.exe" in
>>>>      if Sys.file_exists tool_path then
>>>>        configure_wait_pnp tool_path
>>>> @@ -345,6 +347,16 @@ let convert (g : G.guestfs) _ inspect
i_firmware block_driver _ static_ips >>>>                       
strkey name value
>>>>      | None -> sprintf "reg delete \"%s\" /v
%s /f" strkey name
>>>>  
>>>> +  and configure_pnputil_install () >>>> +    let
fb_script = "@echo off\n\
>>>> +                     \n\
>>>> +                     echo Wait for VirtIO drivers to be
installed\n\
>>>> +                     %systemroot%\\Sysnative\\PnPutil -i -a \
>>>> +                     %systemroot%\\Drivers\\Virtio\\*.inf
>\"%~dpn0.log\" 2>&1\
>>>> +                    " in
>>>> +    Firstboot.add_firstboot_script g inspect.i_root
>>>> +      "pnputil install drivers" fb_script;
>>>> +
>>>
>>> I'm not sure I'm really qualified to comment on this, since
Windows is
>>> crazy.  I guess the worst this can do is fail to run, but that
won't
>>> stop anything else from happening.
>>>
>>> Note that firstboot scripts already do logging, so I don't
believe
>>> writing to a separate log file is needed here.  All the output from
>>> all firstboot scripts should go to
>>> C:\Program Files\Guestfs\Firstboot\log.txt:
>>>
>>>
https://github.com/libguestfs/libguestfs-common/blob/7acf991a25b3fd625eb1ff1fbd8dc9fedf245942/mlcustomize/firstboot.ml#L276
>>
>> Right, but apart from having the log common for all firstboot scripts,
>> some of them also utilise separate log files in scripts-done directory,
>> e.g.:
>>
>>
https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_windows.ml#L381
>>
https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_windows.ml#L168
>>
>> So I did the same considering that pnputil's output is relatively
long.
> 
> I think I'd probably get rid of those other scripts too.  It's
helpful
> to have everything go to one place.  I'm not actually sure where those
> extra files end up.
> 
>> All in all, if there're no other concerns, can we give this script
a go?
> 
> I think the patch is generally fine, so I'm just quibbling about
> the logging.
I'm surprised by this patch (which doesn't say much of course, because I
know precious little about Windows). What I'm noticing is "run
pnputil.exe before pnp_wait.exe before everything else".

We do queue a script earlier than those, at priority 2500, called
"v2vnetcf.ps1". See configure_network_interfaces ->
add_firstboot_powershell, and the reference to RHBZ 1788823.

And that script is exactly what tends *not* to work, and we've been
unable to get to the bottom of that issue. (See also RH 2068361 and
1963032.)

So... This patch could be orthogonal to that pre-existent symptom... but
could the patch perhaps *fix* the prior symptom? Is it possible that, if
we "forcibly install" the virtio drivers first, and *then* try to
configure IP addresses for the virtio-net NICs, then the latter will
start working reliably?

I have no idea really.

Anyway, once Rich is happy with the logging here, I have nothing against
the patch.

Acked-by: Laszlo Ersek <lersek at redhat.com>