On Wed, Dec 16, 2015 at 07:28:43 -0500, John Ferlan wrote: > > > On 12/16/2015 07:01 AM, Peter Krempa wrote: > > On Wed, Dec 16, 2015 at 12:09:42 +0300, Dmitry Andreev wrote: > >> If config file was changed VIR_DOMAIN_EVENT_DEFINED should be emitted > > > > drop "file" here. The important part is the config itself being changed. > > The file is just a implication of that. > > > > Additionally, commit messages here or in the previous patch don't > > provide any justification for this change. I'd like to have a statement > > why is this important to change. > > > > FWIW: This is what I got from the cover letter: The cover letter is not commited to the repo so it shouldn't be the only place to put such information. I usualy don't even bother to read the cover letter. > > Lack of the event become a problem for virt-manager > https://bugzilla.redhat.com/show_bug.cgi?id=1081148 Okay, that's starting to be reasonable let's say, but ... > > > Without the event, it seems virt-manager doesn't "see" the change and > presents the data prior to snapshot With internal snapshots there's quite a few state changes that can happen according to the source and target state that need to be taken into account. If you follow the snapshot code you'll see that in some cases qemu has to be restarted and in some cases not. In case when qemu is not restarted during reversion we should not emit a lifecycle event. Stop/start of qemu is based on the fact that the "ABI stability" check of the current and target config will not match. This leaves a loophole where e.g. change of a disk source would not be properly tracked. This problem will most likely happen when combining external and internal snapshots ( https://bugzilla.redhat.com/show_bug.cgi?id=880565 ), but also other legitimate changes of config (mostly change of disk source) will not be tracked properly. This is a bug in the snapshot handling code though and qemu should be restarted at that point since it needs to open different files and libvirt needs to label them prior to that. As said, this is a bug in the snapshot code and patching it by doing an event is not right. Additionally if you are going to revert a transient VM snapshot from running state to running state the DEFINED event is totaly bogus since it doesn't have a persistent definition, which is where we refer to it as "DEFINED". In case of offline target states this approach may be valid since the config changes without any notification which is actually what the bug is complaining about. Using the defined event should be used only in such case. One thing to consider then is whether it's worth fixing what bz 1081148 is tracking with this (but the other cases described above should not emit the event then), or whether a different approach is desired. That's why I asked for justification. Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list