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

2023-03-13 Thread Laszlo Ersek
On 3/13/23 10:57, Andrey Drobyshev wrote:
> On 3/13/23 11:28, Laszlo Ersek wrote:
>> 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 
>> ---
>>  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 

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

2023-03-13 Thread Andrey Drobyshev
On 3/13/23 11:28, Laszlo Ersek wrote:
> 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 
> ---
>  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 

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

2023-03-13 Thread Laszlo Ersek
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 
 ---
  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 

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

2023-03-09 Thread Andrey Drobyshev
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 
 ---
  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

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

2023-03-09 Thread Richard W.M. Jones
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 
> >> ---
> >>  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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs



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

2023-03-09 Thread Andrey Drobyshev
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 
>> ---
>>  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.

All in all, if there're no other concerns, can we give this script a go?

> 
> Rich.
> 

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs



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

2023-03-08 Thread Richard W.M. Jones
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 
> ---
>  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

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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs



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

2023-03-08 Thread Andrey Drobyshev
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 
---
 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;
+
   and configure_wait_pnp tool_path =
 (* Prevent destructive interactions of firstboot with PnP. *)
 
-- 
2.31.1

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs