Richard W.M. Jones
2022-Apr-15 09:13 UTC
[Libguestfs] [v2v PATCH] output_qemu: rewrite output disk mapping
On Fri, Apr 15, 2022 at 10:47:28AM +0200, Laszlo Ersek wrote:> The current code handles some nonexistent cases (such as SCSI floppies, > virtio-block CD-ROMs), and does not create proper drives (that is, > back-ends with no media inserted) for removable devices (floppies, > CD-ROMs). > > Rewrite the whole logic: > > - handle only those scenarios that QEMU can support, > > - separate the back-end (-drive) and front-end (-device) options, > > - wherever / whenever a host-bus adapter front-end is needed > (virtio-scsi-pci, isa-fdc), create it, > > - assign front-ends to buses (= parent front-ends) and back-ends > explicitly. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2074805 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > --- > > Notes: > v1: > > - do not pick up Rich's R-b yet, because of the changes below > > - replace "parent_ctrl_needed" with Array.exists [Rich] > > - reference 'virt-v2v(1) section "BUGS"' in the "this should not happen" > warnings [Rich] > > - add "bus=scsi0.0" property to "scsi-hd" and "scsi-cd" devices > > - created test scenarios > <https://bugzilla.redhat.com/show_bug.cgi?id=2074805#c17> and tested > them > > output/output_qemu.ml | 197 ++++++++++++++++---- > 1 file changed, 160 insertions(+), 37 deletions(-) > > diff --git a/output/output_qemu.ml b/output/output_qemu.ml > index da7278b88b67..da8bd475e56e 100644 > --- a/output/output_qemu.ml > +++ b/output/output_qemu.ml > @@ -127,7 +127,7 @@ module QEMU = struct > machine, false in > let smm = secure_boot_required in > > - let machine > + let machine_str > match machine with > | I440FX -> "pc" > | Q35 -> "q35" > @@ -153,7 +153,7 @@ module QEMU = struct > arg_list "-device" ["vmgenid"; sprintf "guid=%s" genid; "id=vmgenid0"] > ); > > - arg_list "-machine" (machine :: > + arg_list "-machine" (machine_str :: > (if smm then ["smm=on"] else []) @ > ["accel=kvm:tcg"]); > > @@ -184,52 +184,175 @@ module QEMU = struct > ); > ); > > - let make_disk if_name i = function > - | BusSlotEmpty -> () > + (* For IDE disks, IDE CD-ROMs, SCSI disks, SCSI CD-ROMs, and floppies, we > + * need host-bus adapters (HBAs) between these devices and the PCI(e) root > + * bus. Some machine types create these HBAs automatically (despite > + * "-no-user-config -nodefaults"), some don't... > + *) > + let disk_cdrom_filter > + function > + | BusSlotDisk _ > + | BusSlotRemovable { s_removable_type = CDROM } -> true > + | _ -> false > + and floppy_filter > + function > + | BusSlotRemovable { s_removable_type = Floppy } -> true > + | _ -> false in > + let ide_ctrl_needed > + Array.exists disk_cdrom_filter target_buses.target_ide_bus > + and scsi_ctrl_needed > + Array.exists disk_cdrom_filter target_buses.target_scsi_bus > + and floppy_ctrl_needed > + Array.exists floppy_filter target_buses.target_floppy_bus in > > - | BusSlotDisk d -> > - (* Find the corresponding target disk. *) > - let outdisk = disk_path output_storage output_name d.s_disk_id in > + if ide_ctrl_needed then ( > + match machine with > + | I440FX -> () > + (* The PC machine has a built-in controller of type "piix3-ide" > + * providing buses "ide.0" and "ide.1", with each bus fitting two > + * devices. > + *) > + | Q35 -> () > + (* The Q35 machine has a built-in controller of type "ich9-ahci" > + * providing buses "ide.0" through "ide.5", with each bus fitting one > + * device. > + *) > + | Virt -> warning (f_"The Virt machine has no support for IDE. Please \ > + report a bug for virt-v2v -- refer to virt-v2v(1) \ > + section \"BUGS\".") > + ); > > - arg_list "-drive" ["file=" ^ outdisk; "format=" ^ output_format; > - "if=" ^ if_name; "index=" ^ string_of_int i; > - "media=disk"] > + if scsi_ctrl_needed then > + (* We need to add the virtio-scsi HBA on all three machine types. The bus > + * provided by this device will be called "scsi0.0". > + *) > + arg_list "-device" [ "virtio-scsi-pci"; "id=scsi0" ]; > > - | BusSlotRemovable { s_removable_type = CDROM } -> > - arg_list "-drive" ["format=raw"; "if=" ^ if_name; > - "index=" ^ string_of_int i; "media=cdrom"] > + if floppy_ctrl_needed then ( > + match machine with > + | I440FX -> () > + (* The PC machine has a built-in controller of type "isa-fdc" > + * providing bus "floppy-bus.0", fitting two devices. > + *) > + | Q35 -> arg_list "-device" [ "isa-fdc"; "id=floppy-bus" ] > + (* On the Q35 machine, we need to add the same HBA manually. Note that > + * the bus name will have ".0" appended automatically. > + *) > + | Virt -> warning (f_"The Virt machine has no support for floppies. \ > + Please report a bug for virt-v2v -- refer to \ > + virt-v2v(1) section \"BUGS\".") > + ); > > - | BusSlotRemovable { s_removable_type = Floppy } -> > - arg_list "-drive" ["format=raw"; "if=" ^ if_name; > - "index=" ^ string_of_int i; "media=floppy"] > - in > - Array.iteri (make_disk "virtio") target_buses.target_virtio_blk_bus; > - Array.iteri (make_disk "ide") target_buses.target_ide_bus; > + let add_disk_backend disk_id backend_name > + (* Add a drive (back-end) for a "virtio-blk-pci", "ide-hd", or "scsi-hd" > + * device (front-end). The drive has a backing file, identified by > + * "disk_id". > + *) > + let outdisk = disk_path output_storage output_name disk_id in > + arg_list "-drive" [ "file=" ^ outdisk; "format=" ^ output_format; > + "if=none"; "id=" ^ backend_name; "media=disk" ] > > - let make_scsi i = function > - | BusSlotEmpty -> () > + and add_cdrom_backend backend_name > + (* Add a drive (back-end) for an "ide-cd" or "scsi-cd" device (front-end). > + * The drive is empty -- there is no backing file. > + *) > + arg_list "-drive" [ "if=none"; "id=" ^ backend_name; "media=cdrom" ] > > - | BusSlotDisk d -> > - (* Find the corresponding target disk. *) > - let outdisk = disk_path output_storage output_name d.s_disk_id in > + and add_floppy_backend backend_name > + (* Add a drive (back-end) for a "floppy" device (front-end). The drive is > + * empty -- there is no backing file. *) > + arg_list "-drive" [ "if=none"; "id=" ^ backend_name; "media=disk" ] in > > - arg_list "-drive" ["file=" ^ outdisk; "format=" ^ output_format; > - "if=scsi"; "bus=0"; "unit=" ^ string_of_int i; > - "media=disk"] > + let add_virtio_blk disk_id frontend_ctr > + (* Create a "virtio-blk-pci" device (front-end), together with its drive > + * (back-end). The disk identifier is mandatory. > + *) > + let backend_name = sprintf "drive-vblk-%d" frontend_ctr in > + add_disk_backend disk_id backend_name; > + arg_list "-device" [ "virtio-blk-pci"; "drive=" ^ backend_name ] > > - | BusSlotRemovable { s_removable_type = CDROM } -> > - arg_list "-drive" ["format=raw"; "if=scsi"; "bus=0"; > - "unit=" ^ string_of_int i; "media=cdrom"] > + and add_ide disk_id frontend_ctr > + (* Create an "ide-hd" or "ide-cd" device (front-end), together with its > + * drive (back-end). If a disk identifier is passed in, then "ide-hd" is > + * created (with a non-empty drive); otherwise, "ide-cd" is created (with > + * an empty drive). > + *) > + let backend_name = sprintf "drive-ide-%d" frontend_ctr > + and ide_bus, ide_unit > + match machine with > + | I440FX -> frontend_ctr / 2, frontend_ctr mod 2 > + | Q35 -> frontend_ctr, 0 > + | Virt -> 0, 0 (* should never happen, see warning above *) in > + let common_props = [ sprintf "bus=ide.%d" ide_bus; > + sprintf "unit=%d" ide_unit; > + "drive=" ^ backend_name ] in > + (match disk_id with > + | Some id -> > + add_disk_backend id backend_name; > + arg_list "-device" ([ "ide-hd" ] @ common_props) > + | None -> > + add_cdrom_backend backend_name; > + arg_list "-device" ([ "ide-cd" ] @ common_props)) > + > + and add_scsi disk_id frontend_ctr > + (* Create a "scsi-hd" or "scsi-cd" device (front-end), together with its > + * drive (back-end). If a disk identifier is passed in, then "scsi-hd" is > + * created (with a non-empty drive); otherwise, "scsi-cd" is created (with > + * an empty drive). > + *) > + let backend_name = sprintf "drive-scsi-%d" frontend_ctr in > + let common_props = [ "bus=scsi0.0"; > + sprintf "lun=%d" frontend_ctr; > + "drive=" ^ backend_name ] in > + (match disk_id with > + | Some id -> > + add_disk_backend id backend_name; > + arg_list "-device" ([ "scsi-hd" ] @ common_props) > + | None -> > + add_cdrom_backend backend_name; > + arg_list "-device" ([ "scsi-cd" ] @ common_props)) > > - | BusSlotRemovable { s_removable_type = Floppy } -> > - arg_list "-drive" ["format=raw"; "if=scsi"; "bus=0"; > - "unit=" ^ string_of_int i; "media=floppy"] > - in > - Array.iteri make_scsi target_buses.target_scsi_bus; > + and add_floppy frontend_ctr > + (* Create a "floppy" (front-end), together with its empty drive > + * (back-end). > + *) > + let backend_name = sprintf "drive-floppy-%d" frontend_ctr in > + add_floppy_backend backend_name; > + arg_list "-device" [ "floppy"; "bus=floppy-bus.0"; > + sprintf "unit=%d" frontend_ctr; > + "drive=" ^ backend_name ] in > > - (* XXX Highly unlikely that anyone cares, but the current > - * code ignores target_buses.target_floppy_bus. > + (* Add virtio-blk-pci devices for BusSlotDisk elements on > + * "target_virtio_blk_bus". > *) > + Array.iteri > + (fun frontend_ctr disk -> > + match disk with > + | BusSlotDisk d -> add_virtio_blk d.s_disk_id frontend_ctr > + | _ -> ()) > + target_buses.target_virtio_blk_bus; > + > + let add_disk_or_cdrom bus_adder frontend_ctr slot > + (* Add a disk or CD-ROM front-end to the IDE or SCSI bus. *) > + match slot with > + | BusSlotDisk d -> > + bus_adder (Some d.s_disk_id) frontend_ctr > + | BusSlotRemovable { s_removable_type = CDROM } -> > + bus_adder None frontend_ctr > + | _ -> () in > + > + (* Add disks and CD-ROMs to the IDE and SCSI buses. *) > + Array.iteri (add_disk_or_cdrom add_ide) target_buses.target_ide_bus; > + Array.iteri (add_disk_or_cdrom add_scsi) target_buses.target_scsi_bus; > + > + (* Add floppies. *) > + Array.iteri > + (fun frontend_ctr disk -> > + match disk with > + | BusSlotRemovable { s_removable_type = Floppy } -> > + add_floppy frontend_ctr > + | _ -> ()) > + target_buses.target_floppy_bus; > > let net_bus > match guestcaps.gcaps_net_bus with > > base-commit: b3a9dd6442ea2aff31bd93e85aa130f432e24932A style point: When defining things which are functions (even if they are curried) I usually put the "in" on the next line as a small reminder that it's a function and not a value. eg: let disk_cdrom_filter function | BusSlotDisk _ | BusSlotRemovable { s_removable_type = CDROM } -> true | _ -> false and floppy_filter function | BusSlotRemovable { s_removable_type = Floppy } -> true | _ -> false in <-- here There are a few places in the code that don't follow that style. Reviewed-by: Richard W.M. Jones <rjones at redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Laszlo Ersek
2022-Apr-17 07:13 UTC
[Libguestfs] [v2v PATCH] output_qemu: rewrite output disk mapping
On 04/15/22 11:13, Richard W.M. Jones wrote:> On Fri, Apr 15, 2022 at 10:47:28AM +0200, Laszlo Ersek wrote: >> The current code handles some nonexistent cases (such as SCSI floppies, >> virtio-block CD-ROMs), and does not create proper drives (that is, >> back-ends with no media inserted) for removable devices (floppies, >> CD-ROMs). >> >> Rewrite the whole logic: >> >> - handle only those scenarios that QEMU can support, >> >> - separate the back-end (-drive) and front-end (-device) options, >> >> - wherever / whenever a host-bus adapter front-end is needed >> (virtio-scsi-pci, isa-fdc), create it, >> >> - assign front-ends to buses (= parent front-ends) and back-ends >> explicitly. >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2074805 >> Signed-off-by: Laszlo Ersek <lersek at redhat.com> >> --- >> >> Notes: >> v1: >> >> - do not pick up Rich's R-b yet, because of the changes below >> >> - replace "parent_ctrl_needed" with Array.exists [Rich] >> >> - reference 'virt-v2v(1) section "BUGS"' in the "this should not happen" >> warnings [Rich] >> >> - add "bus=scsi0.0" property to "scsi-hd" and "scsi-cd" devices >> >> - created test scenarios >> <https://bugzilla.redhat.com/show_bug.cgi?id=2074805#c17> and tested >> them >> >> output/output_qemu.ml | 197 ++++++++++++++++---- >> 1 file changed, 160 insertions(+), 37 deletions(-) >> >> diff --git a/output/output_qemu.ml b/output/output_qemu.ml >> index da7278b88b67..da8bd475e56e 100644 >> --- a/output/output_qemu.ml >> +++ b/output/output_qemu.ml >> @@ -127,7 +127,7 @@ module QEMU = struct >> machine, false in >> let smm = secure_boot_required in >> >> - let machine >> + let machine_str >> match machine with >> | I440FX -> "pc" >> | Q35 -> "q35" >> @@ -153,7 +153,7 @@ module QEMU = struct >> arg_list "-device" ["vmgenid"; sprintf "guid=%s" genid; "id=vmgenid0"] >> ); >> >> - arg_list "-machine" (machine :: >> + arg_list "-machine" (machine_str :: >> (if smm then ["smm=on"] else []) @ >> ["accel=kvm:tcg"]); >> >> @@ -184,52 +184,175 @@ module QEMU = struct >> ); >> ); >> >> - let make_disk if_name i = function >> - | BusSlotEmpty -> () >> + (* For IDE disks, IDE CD-ROMs, SCSI disks, SCSI CD-ROMs, and floppies, we >> + * need host-bus adapters (HBAs) between these devices and the PCI(e) root >> + * bus. Some machine types create these HBAs automatically (despite >> + * "-no-user-config -nodefaults"), some don't... >> + *) >> + let disk_cdrom_filter >> + function >> + | BusSlotDisk _ >> + | BusSlotRemovable { s_removable_type = CDROM } -> true >> + | _ -> false >> + and floppy_filter >> + function >> + | BusSlotRemovable { s_removable_type = Floppy } -> true >> + | _ -> false in >> + let ide_ctrl_needed >> + Array.exists disk_cdrom_filter target_buses.target_ide_bus >> + and scsi_ctrl_needed >> + Array.exists disk_cdrom_filter target_buses.target_scsi_bus >> + and floppy_ctrl_needed >> + Array.exists floppy_filter target_buses.target_floppy_bus in >> >> - | BusSlotDisk d -> >> - (* Find the corresponding target disk. *) >> - let outdisk = disk_path output_storage output_name d.s_disk_id in >> + if ide_ctrl_needed then ( >> + match machine with >> + | I440FX -> () >> + (* The PC machine has a built-in controller of type "piix3-ide" >> + * providing buses "ide.0" and "ide.1", with each bus fitting two >> + * devices. >> + *) >> + | Q35 -> () >> + (* The Q35 machine has a built-in controller of type "ich9-ahci" >> + * providing buses "ide.0" through "ide.5", with each bus fitting one >> + * device. >> + *) >> + | Virt -> warning (f_"The Virt machine has no support for IDE. Please \ >> + report a bug for virt-v2v -- refer to virt-v2v(1) \ >> + section \"BUGS\".") >> + ); >> >> - arg_list "-drive" ["file=" ^ outdisk; "format=" ^ output_format; >> - "if=" ^ if_name; "index=" ^ string_of_int i; >> - "media=disk"] >> + if scsi_ctrl_needed then >> + (* We need to add the virtio-scsi HBA on all three machine types. The bus >> + * provided by this device will be called "scsi0.0". >> + *) >> + arg_list "-device" [ "virtio-scsi-pci"; "id=scsi0" ]; >> >> - | BusSlotRemovable { s_removable_type = CDROM } -> >> - arg_list "-drive" ["format=raw"; "if=" ^ if_name; >> - "index=" ^ string_of_int i; "media=cdrom"] >> + if floppy_ctrl_needed then ( >> + match machine with >> + | I440FX -> () >> + (* The PC machine has a built-in controller of type "isa-fdc" >> + * providing bus "floppy-bus.0", fitting two devices. >> + *) >> + | Q35 -> arg_list "-device" [ "isa-fdc"; "id=floppy-bus" ] >> + (* On the Q35 machine, we need to add the same HBA manually. Note that >> + * the bus name will have ".0" appended automatically. >> + *) >> + | Virt -> warning (f_"The Virt machine has no support for floppies. \ >> + Please report a bug for virt-v2v -- refer to \ >> + virt-v2v(1) section \"BUGS\".") >> + ); >> >> - | BusSlotRemovable { s_removable_type = Floppy } -> >> - arg_list "-drive" ["format=raw"; "if=" ^ if_name; >> - "index=" ^ string_of_int i; "media=floppy"] >> - in >> - Array.iteri (make_disk "virtio") target_buses.target_virtio_blk_bus; >> - Array.iteri (make_disk "ide") target_buses.target_ide_bus; >> + let add_disk_backend disk_id backend_name >> + (* Add a drive (back-end) for a "virtio-blk-pci", "ide-hd", or "scsi-hd" >> + * device (front-end). The drive has a backing file, identified by >> + * "disk_id". >> + *) >> + let outdisk = disk_path output_storage output_name disk_id in >> + arg_list "-drive" [ "file=" ^ outdisk; "format=" ^ output_format; >> + "if=none"; "id=" ^ backend_name; "media=disk" ] >> >> - let make_scsi i = function >> - | BusSlotEmpty -> () >> + and add_cdrom_backend backend_name >> + (* Add a drive (back-end) for an "ide-cd" or "scsi-cd" device (front-end). >> + * The drive is empty -- there is no backing file. >> + *) >> + arg_list "-drive" [ "if=none"; "id=" ^ backend_name; "media=cdrom" ] >> >> - | BusSlotDisk d -> >> - (* Find the corresponding target disk. *) >> - let outdisk = disk_path output_storage output_name d.s_disk_id in >> + and add_floppy_backend backend_name >> + (* Add a drive (back-end) for a "floppy" device (front-end). The drive is >> + * empty -- there is no backing file. *) >> + arg_list "-drive" [ "if=none"; "id=" ^ backend_name; "media=disk" ] in >> >> - arg_list "-drive" ["file=" ^ outdisk; "format=" ^ output_format; >> - "if=scsi"; "bus=0"; "unit=" ^ string_of_int i; >> - "media=disk"] >> + let add_virtio_blk disk_id frontend_ctr >> + (* Create a "virtio-blk-pci" device (front-end), together with its drive >> + * (back-end). The disk identifier is mandatory. >> + *) >> + let backend_name = sprintf "drive-vblk-%d" frontend_ctr in >> + add_disk_backend disk_id backend_name; >> + arg_list "-device" [ "virtio-blk-pci"; "drive=" ^ backend_name ] >> >> - | BusSlotRemovable { s_removable_type = CDROM } -> >> - arg_list "-drive" ["format=raw"; "if=scsi"; "bus=0"; >> - "unit=" ^ string_of_int i; "media=cdrom"] >> + and add_ide disk_id frontend_ctr >> + (* Create an "ide-hd" or "ide-cd" device (front-end), together with its >> + * drive (back-end). If a disk identifier is passed in, then "ide-hd" is >> + * created (with a non-empty drive); otherwise, "ide-cd" is created (with >> + * an empty drive). >> + *) >> + let backend_name = sprintf "drive-ide-%d" frontend_ctr >> + and ide_bus, ide_unit >> + match machine with >> + | I440FX -> frontend_ctr / 2, frontend_ctr mod 2 >> + | Q35 -> frontend_ctr, 0 >> + | Virt -> 0, 0 (* should never happen, see warning above *) in >> + let common_props = [ sprintf "bus=ide.%d" ide_bus; >> + sprintf "unit=%d" ide_unit; >> + "drive=" ^ backend_name ] in >> + (match disk_id with >> + | Some id -> >> + add_disk_backend id backend_name; >> + arg_list "-device" ([ "ide-hd" ] @ common_props) >> + | None -> >> + add_cdrom_backend backend_name; >> + arg_list "-device" ([ "ide-cd" ] @ common_props)) >> + >> + and add_scsi disk_id frontend_ctr >> + (* Create a "scsi-hd" or "scsi-cd" device (front-end), together with its >> + * drive (back-end). If a disk identifier is passed in, then "scsi-hd" is >> + * created (with a non-empty drive); otherwise, "scsi-cd" is created (with >> + * an empty drive). >> + *) >> + let backend_name = sprintf "drive-scsi-%d" frontend_ctr in >> + let common_props = [ "bus=scsi0.0"; >> + sprintf "lun=%d" frontend_ctr; >> + "drive=" ^ backend_name ] in >> + (match disk_id with >> + | Some id -> >> + add_disk_backend id backend_name; >> + arg_list "-device" ([ "scsi-hd" ] @ common_props) >> + | None -> >> + add_cdrom_backend backend_name; >> + arg_list "-device" ([ "scsi-cd" ] @ common_props)) >> >> - | BusSlotRemovable { s_removable_type = Floppy } -> >> - arg_list "-drive" ["format=raw"; "if=scsi"; "bus=0"; >> - "unit=" ^ string_of_int i; "media=floppy"] >> - in >> - Array.iteri make_scsi target_buses.target_scsi_bus; >> + and add_floppy frontend_ctr >> + (* Create a "floppy" (front-end), together with its empty drive >> + * (back-end). >> + *) >> + let backend_name = sprintf "drive-floppy-%d" frontend_ctr in >> + add_floppy_backend backend_name; >> + arg_list "-device" [ "floppy"; "bus=floppy-bus.0"; >> + sprintf "unit=%d" frontend_ctr; >> + "drive=" ^ backend_name ] in >> >> - (* XXX Highly unlikely that anyone cares, but the current >> - * code ignores target_buses.target_floppy_bus. >> + (* Add virtio-blk-pci devices for BusSlotDisk elements on >> + * "target_virtio_blk_bus". >> *) >> + Array.iteri >> + (fun frontend_ctr disk -> >> + match disk with >> + | BusSlotDisk d -> add_virtio_blk d.s_disk_id frontend_ctr >> + | _ -> ()) >> + target_buses.target_virtio_blk_bus; >> + >> + let add_disk_or_cdrom bus_adder frontend_ctr slot >> + (* Add a disk or CD-ROM front-end to the IDE or SCSI bus. *) >> + match slot with >> + | BusSlotDisk d -> >> + bus_adder (Some d.s_disk_id) frontend_ctr >> + | BusSlotRemovable { s_removable_type = CDROM } -> >> + bus_adder None frontend_ctr >> + | _ -> () in >> + >> + (* Add disks and CD-ROMs to the IDE and SCSI buses. *) >> + Array.iteri (add_disk_or_cdrom add_ide) target_buses.target_ide_bus; >> + Array.iteri (add_disk_or_cdrom add_scsi) target_buses.target_scsi_bus; >> + >> + (* Add floppies. *) >> + Array.iteri >> + (fun frontend_ctr disk -> >> + match disk with >> + | BusSlotRemovable { s_removable_type = Floppy } -> >> + add_floppy frontend_ctr >> + | _ -> ()) >> + target_buses.target_floppy_bus; >> >> let net_bus >> match guestcaps.gcaps_net_bus with >> >> base-commit: b3a9dd6442ea2aff31bd93e85aa130f432e24932 > > A style point: When defining things which are functions (even if they > are curried) I usually put the "in" on the next line as a small > reminder that it's a function and not a value. eg: > > let disk_cdrom_filter > function > | BusSlotDisk _ > | BusSlotRemovable { s_removable_type = CDROM } -> true > | _ -> false > and floppy_filter > function > | BusSlotRemovable { s_removable_type = Floppy } -> true > | _ -> false > in <-- here > > There are a few places in the code that don't follow that style.Yeah, I'll fix this up before pushing. Another style point / simplification has occurred to me the other day too: in the patch I use the following expression four times: ([ device_model ] @ common_props) that should be just (device_model :: common_props) IIUC. Thanks, Laszlo> > Reviewed-by: Richard W.M. Jones <rjones at redhat.com> > > Rich. >