Re: [Libguestfs] [V2V PATCH v2 1/5] Revert "Remove guestcaps_block_type Virtio_SCSI"

On 3/13/23 09:22, Andrey Drobyshev wrote: > On 3/13/23 10:13, Laszlo Ersek wrote: >> On 3/7/23 20:40, Andrey Drobyshev wrote: >>> This code is needed to check whether virtio-scsi driver was installed. >>> >>> This reverts commit f0afc439524853508938b2bfc758896f053462e3. >>> --- >>> convert/convert.ml | 2 +- >>> convert/convert_linux.ml | 9 +++++++-- >>> convert/target_bus_assignment.ml | 1 + >>> lib/create_ovf.ml | 1 + >>> lib/types.ml | 3 ++- >>> lib/types.mli | 2 +- >>> output/openstack_image_properties.ml | 7 +++++++ >>> 7 files changed, 20 insertions(+), 5 deletions(-) >>> >>> diff --git a/convert/convert.ml b/convert/convert.ml >>> index 0aa0e5cd..084619c8 100644 >>> --- a/convert/convert.ml >>> +++ b/convert/convert.ml >>> @@ -252,7 +252,7 @@ and do_convert g source inspect i_firmware keep_serial_console interfaces = >>> (* Did we manage to install virtio drivers? *) >>> if not (quiet ()) then ( >>> match guestcaps.gcaps_block_bus with >>> - | Virtio_blk -> >>> + | Virtio_blk | Virtio_SCSI -> >>> info (f_"This guest has virtio drivers installed.") >>> | IDE -> >>> info (f_"This guest does not have virtio drivers installed.") >>> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml >>> index d5c0f24d..dab4f36d 100644 >>> --- a/convert/convert_linux.ml >>> +++ b/convert/convert_linux.ml >>> @@ -1068,8 +1068,12 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = >>> (* Update 'alias scsi_hostadapter ...' *) >>> let paths = augeas_modprobe ". =~ regexp('scsi_hostadapter.*')" in >>> (match block_type with >>> - | Virtio_blk -> >>> - let block_module = "virtio_blk" in >>> + | Virtio_blk | Virtio_SCSI -> >>> + let block_module = >>> + match block_type with >>> + | Virtio_blk -> "virtio_blk" >>> + | Virtio_SCSI -> "virtio_scsi" >>> + | IDE -> assert false in >>> >>> if paths <> [] then ( >>> (* There's only 1 scsi controller in the converted guest. >>> @@ -1142,6 +1146,7 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = >>> let block_prefix_after_conversion = >>> match block_type with >>> | Virtio_blk -> "vd" >>> + | Virtio_SCSI -> "sd" >>> | IDE -> ide_block_prefix in >>> >>> let map = >>> diff --git a/convert/target_bus_assignment.ml b/convert/target_bus_assignment.ml >>> index 54c9516b..d13340c7 100644 >>> --- a/convert/target_bus_assignment.ml >>> +++ b/convert/target_bus_assignment.ml >>> @@ -35,6 +35,7 @@ let rec target_bus_assignment source_disks source_removables guestcaps = >>> let bus = >>> match guestcaps.gcaps_block_bus with >>> | Virtio_blk -> virtio_blk_bus >>> + | Virtio_SCSI -> scsi_bus >>> | IDE -> ide_bus in >>> List.iteri ( >>> fun i d -> >>> diff --git a/lib/create_ovf.ml b/lib/create_ovf.ml >>> index 5e444868..f0b32e01 100644 >>> --- a/lib/create_ovf.ml >>> +++ b/lib/create_ovf.ml >>> @@ -920,6 +920,7 @@ and add_disks sizes guestcaps output_alloc output_format >>> "ovf:disk-interface", >>> (match guestcaps.gcaps_block_bus with >>> | Virtio_blk -> "VirtIO" >>> + | Virtio_SCSI -> "VirtIO_SCSI" >>> | IDE -> "IDE"); >>> "ovf:disk-type", "System"; (* RHBZ#744538 *) >>> "ovf:boot", if is_bootable_drive then "True" else "False"; >>> diff --git a/lib/types.ml b/lib/types.ml >>> index e16da007..75c14fd4 100644 >>> --- a/lib/types.ml >>> +++ b/lib/types.ml >>> @@ -400,12 +400,13 @@ type guestcaps = { >>> gcaps_arch_min_version : int; >>> gcaps_virtio_1_0 : bool; >>> } >>> -and guestcaps_block_type = Virtio_blk | IDE >>> +and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE >>> and guestcaps_net_type = Virtio_net | E1000 | RTL8139 >>> and guestcaps_machine = I440FX | Q35 | Virt >>> >>> let string_of_block_type = function >>> | Virtio_blk -> "virtio-blk" >>> + | Virtio_SCSI -> "virtio-scsi" >>> | IDE -> "ide" >>> let string_of_net_type = function >>> | Virtio_net -> "virtio-net" >>> diff --git a/lib/types.mli b/lib/types.mli >>> index 4a183dd3..65ef2e35 100644 >>> --- a/lib/types.mli >>> +++ b/lib/types.mli >>> @@ -280,7 +280,7 @@ type guestcaps = { >>> } >>> (** Guest capabilities after conversion. eg. Was virtio found or installed? *) >>> >>> -and guestcaps_block_type = Virtio_blk | IDE >>> +and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE >>> and guestcaps_net_type = Virtio_net | E1000 | RTL8139 >>> and guestcaps_machine = I440FX | Q35 | Virt >>> >>> diff --git a/output/openstack_image_properties.ml b/output/openstack_image_properties.ml >>> index c75d72fe..c76ad913 100644 >>> --- a/output/openstack_image_properties.ml >>> +++ b/output/openstack_image_properties.ml >>> @@ -35,6 +35,7 @@ let create source inspect { target_buses; guestcaps; target_firmware } = >>> "hw_disk_bus", >>> (match guestcaps.gcaps_block_bus with >>> | Virtio_blk -> "virtio" >>> + | Virtio_SCSI -> "scsi" >>> | IDE -> "ide"); >>> "hw_vif_model", >>> (match guestcaps.gcaps_net_bus with >>> @@ -69,6 +70,12 @@ let create source inspect { target_buses; guestcaps; target_firmware } = >>> List.push_back properties ("hw_cpu_threads", string_of_int threads); >>> ); >>> >>> + (match guestcaps.gcaps_block_bus with >>> + | Virtio_SCSI -> >>> + List.push_back properties ("hw_scsi_model", "virtio-scsi") >>> + | Virtio_blk | IDE -> () >>> + ); >>> + >>> (match inspect.i_major_version, inspect.i_minor_version with >>> | 0, 0 -> () >>> | x, 0 -> List.push_back properties ("os_version", string_of_int x) >> >> Looks like a reasonable revert to me. >> >> Acked-by: Laszlo Ersek <lersek(a)redhat.com&gt; >> > > Hi Laszlo, > > You're reviewing v2, but the latest series which Richard has approved is v3: > > https://listman.redhat.com/archives/libguestfs/2023-March/031035.html > https://listman.redhat.com/archives/libguestfs/2023-March/031041.html > > I added you to Cc, but maybe it wasn't delivered? > I had an emergency last Wednesday mid-review and could only resume email processing this morning. Whenever I resume email processing, I relatively firmly stick with a FIFO method. In other words, I usually don't fetch new email until I process all previous, pending email. Sometimes this causes me to do double work or to create a bit of confusion. On the other hand, it very safely ensures that no email falls through the cracks. The LIFO method (process most recent email first) runs a very high risk of losing old email. "Old" does not translate to "irrelevant". So yes, I fully expected this morning that you could have posted a v3 meanwhile. Now someone could be tempted to suggest, "why don't you fetch email in the morning, then prioritize everything properly, and *then* work in a priority queue order". The problem is that prioritzing everything itself is huge work. After having been out for 2 work days, I've found 330 new emails this morning. For me, the only thing that works is delaying the fetching of new email, until I'm done with the old batch. So, I can be slow, or I can be lossy. I prefer slow. Laszlo