Re: [libvirt PATCH 2/2] conf: log error on attempts to modify ACPI index of active device

On Thu, Sep 16, 2021 at 21:16:03 -0400, Laine Stump wrote: Look, my problem is that we have an utterly inconsistent user story in regards to device update and we are repeating our own mistakes. If we are stating that the user should use the FULL xml for update, then they should use the full XML and we should not arbitrarily choose what we allow. Similarly if we want to allow partial XML then that's fine as long as we are consistent with ourselves, the user expectations and the docs. Now for the repeating of our mistake part: Same for the ACPI index. It was added, and device-update was neglected. Yup, that's also exactly what this patch is doing. So this is still going to be a hypothetical. Imagine a management application that uses partial XML when updating a device because they generate it rather than taking a snippet from dumpxml and modifying it. (We do have an example at least in terms of disk update here.) The same way as in libvirt, they figure out that they need to start using the ACPI index for some reason. They implement it in their full domain XML generator, but similarly to libvirt where we neglect the update-device code they don't modify the generator that generates the update XML. So now they are using the ACPI index when defining the XML but not when updating the device. The update-device code in libvirt thinks now that they are attempting to change the ACPI index, which would seem to be a regression, because nobody actually touched the update device XML generator. This is also amplified by the fact libvirt actually fixed a similar bug before. Bam a new BZ gets filed for this. I grant you a mitigating circumstance that ACPI index is not really a commonly used feature, so there's a chance that the scenario above might not happen, but that doesn't mean that it's not a latent problem given what we've done before. See, we've acknowledged actually that partial XMLs are fine and thus there was my reasoning that the same logic as with the alias should be applied, otherwise it's opening a gate for another history repeat.