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.
