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: Lack of the event become a problem for virt-manager https://bugzilla.redhat.com/show_bug.cgi?id=1081148 Without the event, it seems virt-manager doesn't "see" the change and presents the data prior to snapshot John >> --- >> src/qemu/qemu_driver.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 783a7cd..1474eaa 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -15512,7 +15512,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, >> VIR_DOMAIN_EVENT_STARTED, >> detail); >> if (rc < 0) >> - goto endjob; >> + goto defined; >> } >> >> /* Touch up domain state. */ >> @@ -15534,7 +15534,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, >> if (!virDomainObjIsActive(vm)) { >> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >> _("guest unexpectedly quit")); >> - goto endjob; >> + goto defined; >> } >> rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn, >> VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, >> @@ -15636,6 +15636,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, >> >> ret = 0; >> >> + defined: >> + if (config) { >> + virObjectEventPtr define_event; >> + define_event = virDomainEventLifecycleNewFromObj(vm, >> + VIR_DOMAIN_EVENT_DEFINED, >> + VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT); >> + qemuDomainEventQueue(driver, define_event); >> + } > > This will emit the event even if the configuration itself didn't change. > We do a similar thing when you switch off the VM. The next-start config > replaces the current config. This is implied by the events of starting > and stopping of the VM. > > Since the internal snapshot code doesn't restart qemu in some cases, > that's when the configuration can't change (which actually might be > implemented wrong in a few places). For snapshot state transitions that > change domain state, the appropriate events are already used. > > As of the reasons above, I'd like you to provide some justification why > this is a good thing to do. > > Peter > > > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list