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

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-wi... >>>> >>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com&gt; >>>> --- >>>> 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/7acf991a25b3fd625eb1... >> >> 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_w... >> https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_w... >> >> 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(a)redhat.com&gt;