On Thu, Oct 14, 2021 at 15:46:10 +0530, Ani Sinha wrote: > https://gitlab.com/libvirt/libvirt/-/commit/bdc3e8f47be108fa552b72a6d913528869e61097 > > I think you completely missed the logic here Okay so let me explain my thoughtst first. I've noticed that the QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE is present for all supported qemu versions (qemu-2.11 and up) which makes it pointless: tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml: <flag name='piix4.acpi-hotplug-bridge'/> Thus I've set up to delete it as we don't add feature flags unless necessary. This was probably missed in the review of your series, but since there is no upstream release which would contain it it's okay and we can delete it. So there are two uses which check the flag, one in validation: size_t i; bool q35Dom = qemuDomainIsQ35(def); bool q35cap = q35Dom && virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE); if (def->features[feature] == VIR_TRISTATE_SWITCH_ABSENT) return 0; for (i = 0; i < VIR_DOMAIN_PCI_LAST; i++) { if (def->pci_features[i] == VIR_TRISTATE_SWITCH_ABSENT) continue; switch ((virDomainPCI) i) { case VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG: if (!ARCH_IS_X86(def->os.arch)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("acpi-bridge-hotplug is not available " "for architecture '%s'"), virArchToString(def->os.arch)); return -1; } if (!q35cap && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("acpi-bridge-hotplug is not available " "with this QEMU binary")); return -1; } break; case VIR_DOMAIN_PCI_LAST: break; } And one in the command line generator: if (acpihp_br != VIR_TRISTATE_SWITCH_ABSENT) { const char *pm_object = NULL; if (!qemuDomainIsQ35(def) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE)) pm_object = "PIIX4_PM"; if (qemuDomainIsQ35(def) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE)) pm_object = "ICH9-LPC"; if (pm_object != NULL) { virCommandAddArg(cmd, "-global"); virCommandAddArgFormat(cmd, "%s.acpi-pci-hotplug-with-bridge-support=%s", pm_object, virTristateSwitchTypeToString(acpihp_br)); } } So now we know that QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE is always present. Thus in the validation code: if (!q35cap && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("acpi-bridge-hotplug is not available " "with this QEMU binary")); return -1; } the virQEMUCapsGet call always returns true, which is inverted and used in a logic and clause. This makes it always false, so that whole condition can be removed. This also makes 'q35cap' unused which in turn makes 'q35dom' unused. So after deleting all of that we don't have anything validating QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE. Given how the command line generator looks: if (!qemuDomainIsQ35(def) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE)) pm_object = "PIIX4_PM"; if (qemuDomainIsQ35(def) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE)) pm_object = "ICH9-LPC"; it's apparent that for q35 we need QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE and for i440fx QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE. Looking back at the validation, the validation was actually checking something else ergo I've assumed that the validation is wrong. This was also amplified by the fact that in the review of the original patch removing the capability: https://listman.redhat.com/archives/libvir-list/2021-October/msg00521.html it was pointed out to me that the error message reported is changed to something which doesn't seem to be relevant for the tested code. This prompted me to actually first convert the test cases from fake-capability tests (DO_TEST(..., CAPS)) to DO_TEST_CAPS_(VER|LASTED). We mandate that all new code should use the real capabilities test as it prevents surprises mostly. So in converting those tests: https://listman.redhat.com/archives/libvir-list/2021-October/msg00542.html I've took care to preserve the negative cases (those which still were applicable) by using the following invocations specifically DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-hotplug-bridge-disable"); was deleted as there is no real qemu ersion which would fail this test (which is the reason explained at the beginning) and DO_TEST_PARSE_ERROR_NOCAPS("q35-acpi-hotplug-bridge-disable"); was converted into: DO_TEST_CAPS_VER_PARSE_ERROR("q35-acpi-hotplug-bridge-disable", "6.0.0"); Now when I've originally converted it I've noticed that the negative test failed, because libvirt actually sucessfully generated a commandline for it. Thus I've looked back at the validation which didn't have any specific comment about any non-obvious logic being used. The command line generator logic then verified that there's a flaw in the validator. Now please explain to me where I made an error. > and you are rewriting a patch without cc'ing the original author. > > I'm sorry but this is rude. I'm sorry I've offended you this way. Generally we don't CC original authors when modifying the code because it's very tedious and most authors are subscribed to the list. I'll try to remember it for the future.
