7:39 a.m.
On 9/20/19 6:25 AM, Erik Skultety wrote:
On Thu, Sep 19, 2019 at 05:04:47PM -0300, Daniel Henrique Barboza
wrote:
> A common operation in qemu_domain_address is comparing a
> virPCIDeviceAddress and assigning domain, bus, slot and function
> to a specific value. The former can be done with the existing
> virPCIDeviceAddressEqual() helper.
>
> A new virpci helper called virPCIDeviceSetAddress() was created to
> make it easier to assign the same domain/bus/slot/function of an
> already existent virPCIDeviceAddress. Using both in
> qemu_domain_address made the code tidier, since the object
> was already created to use virPCIDeviceAddressEqual().
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
> ---
>
> I wasn't sure if this change was worth contributing it back,
> since it feels more like a 'look and feel' change. But
> since it made the code inside qemu_domain_address tidier,
> I decided to take a shot.
I actually think it does enhance readability, so why not. However...
[...]
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -1729,45 +1729,41 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
> /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */
> for (i = 0; i < def->ncontrollers; i++) {
> virDomainControllerDefPtr cont = def->controllers[i];
> + virPCIDeviceAddress primaryIDEAddr = {.domain = 0, .bus = 0,
> + .slot = 1, .function = 1};
> + virPCIDeviceAddress piix3USBAddr = {.domain = 0, .bus = 0,
> + .slot = 1, .function = 2};
>
> /* First IDE controller lives on the PIIX3 at slot=1, function=1 */
> if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
> cont->idx == 0) {
> if (virDeviceInfoPCIAddressIsPresent(&cont->info)) {
> - if (cont->info.addr.pci.domain != 0 ||
> - cont->info.addr.pci.bus != 0 ||
> - cont->info.addr.pci.slot != 1 ||
> - cont->info.addr.pci.function != 1) {
> + if (!virPCIDeviceAddressEqual(&cont->info.addr.pci,
> + &primaryIDEAddr)) {
I think this virPCIDeviceAddressEqual consolidation might be a separate patch
(matter of taste, feel free to disagree, but to me the changes are not strictly
related and can be easily split).
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Primary IDE controller must have PCI
address 0:0:1.1"));
> + _("Primary IDE controller must have PCI
"
> + "address 0:0:1.1"));
Unrelated...but it can stay...
> return -1;
> }
> } else {
> cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> - cont->info.addr.pci.domain = 0;
> - cont->info.addr.pci.bus = 0;
> - cont->info.addr.pci.slot = 1;
> - cont->info.addr.pci.function = 1;
> + virPCIDeviceSetAddress(&cont->info.addr.pci,
&primaryIDEAddr);
So, given the signature, it feels more like a Copy rather than Set. I imagine a
setter to enumerate the scalar types and the destination where the values
should end up in, in this regard, it's not a proper setter because the struct
has a few more elements that technically can be set, but if you created such a
setter I'd say we didn't gain anything in terms of readability at all,
especially when you could do:
/* provided that I'm interpreting 6.7.8.19 [1] of the C99 standard correctly,
* everything else we're not using should be 0, otherwise we can't do this.
* Alternatively, you'd need to reset the other members as well */
cont->info.addr.pci = primaryIDEAddr;
That was my first idea before trying to come out with this "partial setter"
so to speak - since we're setting only domain/bus/slot/function, I wanted
to avoid setting the other values as well.
But now that I thought bit more about the context I believe the direct
assignment here is fine indeed. Here's a snippet of the code I'm
changing:
if (virDeviceInfoPCIAddressIsPresent(&cont->info)) {
(....code to check if address is equal to X ...)
} else {
/* assign address to X */
AddressIsPresent checks for info->type=TYPE_PCI and if
addr.pci{domain|bus|slot}
is not zero*. It didn't check if multi,ext and zpci was filled, but the
current
code doesn't care much either (otherwise the existing code would be
setting those
as well). My conclusion is that, at this point, these other attributes
are either
irrelevant or will be set up to later on, thus the default 0 values are fine
here.
I'll re-send this dude with direct assignment and without this extra setter
function I came up with inside vircpi.c. Thanks for the review!
DHB
I'm not completely sold on the helper the way it is now, but I'm up for the idea
of the patch. Creating a proper Copy helper would bring the obvious issue of
needing to retain the original settings of .multi and zPCI to make it universal,
that's why I'm opting for the direct assignment, because all of the relevant
code in the patch should be x86 only and multifunction would be reset to 0,
which is also the default setting anyway, so that should play nicely as well.
[1] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf
Erik