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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 16, 2021 at 21:16:03 -0400, Laine Stump wrote:
> On 9/13/21 11:38 AM, Peter Krempa wrote:
> > On Fri, Sep 10, 2021 at 11:18:20 -0400, Laine Stump wrote:
> > > On 9/10/21 3:30 AM, Peter Krempa wrote:
> > > > On Thu, Sep 09, 2021 at 13:46:41 -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:

> 1) originally the alias was completely ignored in device-update, since it
> was always set automatically by libvirt, and so it was a read-only attribute
> from the application's PoV.
> 
> 2) Then user-specified aliases were added, with no associated change to the
> update-device code, and QE testing found that a requested change to alias
> during live device update was silently ignored:
> 
>    https://bugzilla.redhat.com/1585108

Same for the ACPI index. It was added, and device-update was neglected.

> 
> 3) This led to a patch that checked for any change in alias in the
> update-device XML, with no special case for "not specified" (i.e. basically
> what you're suggesting the code should be):
> 
> 
> https://gitlab.com/libvirt/libvirt/-/commit/4ad54a417a1bbe10aeffcce783f4381d8d43f799

Yup, that's also exactly what this patch is doing.

> 4) That patch resulted in a bug filed by RHV saying that the behavior of the
> update-device API had changed, which broke their existing code for changing
> the media in a CD drive or something like that (they don't read back the
> normalized XML after they've started the guest, so they don't have any info
> about auto-generated values, and thus weren't including alias in the XML
> they sent to update-device (this was before RHV started specifying their own
> aliases):
> 
>    https://bugzilla.redhat.com/1621910

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.


> 5) *That* bug resulted in the code being changed to what it is today, with
> this patch:
> 
> 
> https://gitlab.com/libvirt/libvirt/-/commit/b48d9e939bcf32a8d6e571313637e2eefe52e117
> 
> So essentially we are trapped into this "ignore unspecified alias" semantic
> because alias previously existed but wasn't user-specified, and suddenly
> requiring it broke existing applications trying to do things completely
> unrelated to aliases.

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.




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux