Re: [Libguestfs] [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-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.