Re: [libvirt] [PATCH v1 3/4] bhyve: fix SATA address allocation

On 01/05/2017 09:46 AM, Roman Bogorodskiy wrote: > As bhyve for a long time didn't have a notion of the explicit SATA > controller and created a controller for each drive, the bhyve driver > in libvirt acted in a similar way and didn't care about the SATA > controllers and assigned PCI addresses to drives directly, as > the generated command will look like this anyway: > > 2:0,ahci-hd,somedisk.img > > This no longer makes sense because: > > 1. After commit c07d1c1c4f it's not possible to assign > PCI addresses to disks > 2. Bhyve now supports multiple disk drives for a controller, > so it's going away from 1:1 controller:disk mapping, so > the controller object starts to make more sense now > > So, this patch does the following: > > - Assign PCI address to SATA controllers (previously we didn't do this) > - Assign disk addresses instead of PCI addresses for disks. Now, when > building a bhyve command, we take PCI address not from the disk > itself but from its controller > - Assign addresses at XML parsing time using the > assignAddressesCallback. This is done mainly for being able to > verify address allocation via xml2xml tests > - Adjust existing bhyvexml2{xml,argv} tests to chase the new > address allocation > > This patch is largely based on work of Fabian Freyer. > --- > po/POTFILES.in | 1 + > src/bhyve/bhyve_command.c | 143 ++++++++++++++++----- > src/bhyve/bhyve_device.c | 33 ++--- > src/bhyve/bhyve_domain.c | 60 ++++++++- > .../bhyvexml2argvdata/bhyvexml2argv-acpiapic.args | 4 +- > tests/bhyvexml2argvdata/bhyvexml2argv-base.args | 4 +- > .../bhyvexml2argv-bhyveload-bootorder.args | 5 +- > .../bhyvexml2argv-bhyveload-bootorder1.args | 5 +- > .../bhyvexml2argv-bhyveload-bootorder3.args | 5 +- > .../bhyvexml2argv-bhyveload-explicitargs.args | 4 +- > tests/bhyvexml2argvdata/bhyvexml2argv-console.args | 2 +- > .../bhyvexml2argv-custom-loader.args | 4 +- > .../bhyvexml2argv-disk-cdrom-grub.args | 4 +- > .../bhyvexml2argv-disk-cdrom.args | 4 +- > .../bhyvexml2argv-grub-bootorder.args | 6 +- > .../bhyvexml2argv-grub-bootorder2.args | 6 +- > .../bhyvexml2argv-grub-defaults.args | 4 +- > .../bhyvexml2argvdata/bhyvexml2argv-localtime.args | 4 +- > tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.args | 4 +- > .../bhyvexml2argv-serial-grub-nocons.args | 2 +- > .../bhyvexml2argv-serial-grub.args | 2 +- > tests/bhyvexml2argvdata/bhyvexml2argv-serial.args | 2 +- > tests/bhyvexml2argvtest.c | 2 +- > .../bhyvexml2xmlout-acpiapic.xml | 4 +- > tests/bhyvexml2xmloutdata/bhyvexml2xmlout-base.xml | 4 +- > .../bhyvexml2xmlout-bhyveload-bootorder.xml | 4 +- > .../bhyvexml2xmlout-bhyveload-bootorder1.xml | 4 +- > .../bhyvexml2xmlout-bhyveload-bootorder2.xml | 4 +- > .../bhyvexml2xmlout-bhyveload-bootorder3.xml | 4 +- > .../bhyvexml2xmlout-bhyveload-bootorder4.xml | 4 +- > .../bhyvexml2xmlout-bhyveload-explicitargs.xml | 4 +- > .../bhyvexml2xmlout-console.xml | 4 +- > .../bhyvexml2xmlout-custom-loader.xml | 4 +- > .../bhyvexml2xmlout-disk-cdrom-grub.xml | 4 +- > .../bhyvexml2xmlout-disk-cdrom.xml | 4 +- > .../bhyvexml2xmlout-grub-bootorder.xml | 4 +- > .../bhyvexml2xmlout-grub-bootorder2.xml | 4 +- > .../bhyvexml2xmlout-grub-defaults.xml | 4 +- > .../bhyvexml2xmlout-localtime.xml | 4 +- > .../bhyvexml2xmlout-macaddr.xml | 4 +- > .../bhyvexml2xmlout-metadata.xml | 5 +- > .../bhyvexml2xmlout-serial-grub-nocons.xml | 4 +- > .../bhyvexml2xmlout-serial-grub.xml | 4 +- > .../bhyvexml2xmloutdata/bhyvexml2xmlout-serial.xml | 4 +- > tests/bhyvexml2xmltest.c | 2 + > 45 files changed, 279 insertions(+), 118 deletions(-) Aside from these minor nits, my only concern is that of compatibility when migrating forward and backward, but since I don't have a system with bhyve I can't test that, so I'll assume that you've done your due diligence in that regard. (Also, I'm assuming that you've successfully run make syntax-check in addition to make check). Based on those assumptions, ACK.