7 a.m.
v2 of:
https://www.redhat.com/archives/libvir-list/2017-August/msg00221.html
diff to v1:
- instead of removing the file, abort the whole operation
Michal Privoznik (2):
qemuDomainUndefineFlags: Grab QEMU_JOB_MODIFY
qemuDomainUndefineFlags: Forbid undefine of active domain with NVRAM
src/qemu/qemu_driver.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
--
2.13.0
7 a.m.
New subject: [libvirt] [PATCH v2 1/2] qemuDomainUndefineFlags: Grab QEMU_JOB_MODIFY
This API is definitely modifying state of @vm. Therefore it
should grab a job.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b3f65f440..574c351ae 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7325,10 +7325,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (virDomainUndefineFlagsEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+ goto cleanup;
+
if (!vm->persistent) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("cannot undefine transient domain"));
- goto cleanup;
+ goto endjob;
}
if (!virDomainObjIsActive(vm) &&
@@ -7338,15 +7341,15 @@ qemuDomainUndefineFlags(virDomainPtr dom,
_("cannot delete inactive domain with %d "
"snapshots"),
nsnapshots);
- goto cleanup;
+ goto endjob;
}
if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0)
- goto cleanup;
+ goto endjob;
}
name = qemuDomainManagedSavePath(driver, vm);
if (name == NULL)
- goto cleanup;
+ goto endjob;
if (virFileExists(name)) {
if (flags & VIR_DOMAIN_UNDEFINE_MANAGED_SAVE) {
@@ -7354,13 +7357,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Failed to remove domain managed "
"save image"));
- goto cleanup;
+ goto endjob;
}
} else {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("Refusing to undefine while domain managed "
"save image exists"));
- goto cleanup;
+ goto endjob;
}
}
@@ -7372,17 +7375,17 @@ qemuDomainUndefineFlags(virDomainPtr dom,
virReportSystemError(errno,
_("failed to remove nvram: %s"),
vm->def->os.loader->nvram);
- goto cleanup;
+ goto endjob;
}
} else if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot delete inactive domain with nvram"));
- goto cleanup;
+ goto endjob;
}
}
if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
- goto cleanup;
+ goto endjob;
event = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_UNDEFINED,
@@ -7399,6 +7402,8 @@ qemuDomainUndefineFlags(virDomainPtr dom,
qemuDomainRemoveInactive(driver, vm);
ret = 0;
+ endjob:
+ qemuDomainObjEndJob(driver, vm);
cleanup:
VIR_FREE(name);
--
2.13.0
7 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemuDomainUndefineFlags: Forbid undefine of active domain with NVRAM
https://bugzilla.redhat.com/show_bug.cgi?id=1467245
Currently, there's a bug when undefining a domain with NVRAM
store. Basically, the unlink() of the NVRAM store file happens
during the undefine procedure iff domain is inactive. So, if
domain is running and undefine is called the file is left behind.
It won't be removed in the domain cleanup process either
(qemuProcessStop). To avoid this forbid undefining domain with
NVRAM file.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 574c351ae..dc5b924ef 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7367,10 +7367,15 @@ qemuDomainUndefineFlags(virDomainPtr dom,
}
}
- if (!virDomainObjIsActive(vm) &&
- vm->def->os.loader && vm->def->os.loader->nvram
&&
+ if (vm->def->os.loader &&
+ vm->def->os.loader->nvram &&
virFileExists(vm->def->os.loader->nvram)) {
if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
+ if (virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot delete active domain with nvram"));
+ goto endjob;
+ }
if (unlink(vm->def->os.loader->nvram) < 0) {
virReportSystemError(errno,
_("failed to remove nvram: %s"),
--
2.13.0
7:14 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemuDomainUndefineFlags: Forbid undefine of active domain with NVRAM
On Wed, Aug 09, 2017 at 02:00:06PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1467245
Currently, there's a bug when undefining a domain with NVRAM
store. Basically, the unlink() of the NVRAM store file happens
during the undefine procedure iff domain is inactive. So, if
domain is running and undefine is called the file is left behind.
It won't be removed in the domain cleanup process either
(qemuProcessStop). To avoid this forbid undefining domain with
NVRAM file.
Why do we need to forbid it ? Even if QEMU still has an open
file handle, it can continue to write to it after we unlink
it.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
7:42 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemuDomainUndefineFlags: Forbid undefine of active domain with NVRAM
On Wed, Aug 09, 2017 at 01:14:59PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 09, 2017 at 02:00:06PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1467245
>
> Currently, there's a bug when undefining a domain with NVRAM
> store. Basically, the unlink() of the NVRAM store file happens
> during the undefine procedure iff domain is inactive. So, if
> domain is running and undefine is called the file is left behind.
> It won't be removed in the domain cleanup process either
> (qemuProcessStop). To avoid this forbid undefining domain with
> NVRAM file.
Why do we need to forbid it ? Even if QEMU still has an open
file handle, it can continue to write to it after we unlink
it.
It was a suggestion, because that's what will people expect, IMHO.
Feel free to go with the first version if that's what Dan prefers.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:55 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemuDomainUndefineFlags: Forbid undefine of active domain with NVRAM
On 08/09/2017 02:14 PM, Daniel P. Berrange wrote:
On Wed, Aug 09, 2017 at 02:00:06PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1467245
>
> Currently, there's a bug when undefining a domain with NVRAM
> store. Basically, the unlink() of the NVRAM store file happens
> during the undefine procedure iff domain is inactive. So, if
> domain is running and undefine is called the file is left behind.
> It won't be removed in the domain cleanup process either
> (qemuProcessStop). To avoid this forbid undefining domain with
> NVRAM file.
Why do we need to forbid it ? Even if QEMU still has an open
file handle, it can continue to write to it after we unlink
it.
That's what my v1 does. Anyway, there's third option: just recently
Jirka added possibility to do some actions when domain is destroyed. He
needed it for some migration work, but the design is broad enough to fit
this problem too. What we can do is:
if (flags & VIR_DOMAIN_UNDEFINE_NVRAM):
if domain is running:
register the callback /* that merely just unlinks the file */
else:
unlink
else:
if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)):
error
What do you guys think of this one?
Michal
8:02 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemuDomainUndefineFlags: Forbid undefine of active domain with NVRAM
On Wed, Aug 09, 2017 at 02:55:36PM +0200, Michal Privoznik wrote:
On 08/09/2017 02:14 PM, Daniel P. Berrange wrote:
> On Wed, Aug 09, 2017 at 02:00:06PM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1467245
>>
>> Currently, there's a bug when undefining a domain with NVRAM
>> store. Basically, the unlink() of the NVRAM store file happens
>> during the undefine procedure iff domain is inactive. So, if
>> domain is running and undefine is called the file is left behind.
>> It won't be removed in the domain cleanup process either
>> (qemuProcessStop). To avoid this forbid undefining domain with
>> NVRAM file.
>
> Why do we need to forbid it ? Even if QEMU still has an open
> file handle, it can continue to write to it after we unlink
> it.
>
>
That's what my v1 does. Anyway, there's third option: just recently
Jirka added possibility to do some actions when domain is destroyed. He
needed it for some migration work, but the design is broad enough to fit
this problem too. What we can do is:
if (flags & VIR_DOMAIN_UNDEFINE_NVRAM):
if domain is running:
register the callback /* that merely just unlinks the file */
else:
unlink
else:
if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)):
error
What do you guys think of this one?
The callback will be lost if libvirtd restarts.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
8:20 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemuDomainUndefineFlags: Forbid undefine of active domain with NVRAM
On 08/09/2017 03:02 PM, Daniel P. Berrange wrote:
On Wed, Aug 09, 2017 at 02:55:36PM +0200, Michal Privoznik wrote:
> On 08/09/2017 02:14 PM, Daniel P. Berrange wrote:
>> On Wed, Aug 09, 2017 at 02:00:06PM +0200, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1467245
>>>
>>> Currently, there's a bug when undefining a domain with NVRAM
>>> store. Basically, the unlink() of the NVRAM store file happens
>>> during the undefine procedure iff domain is inactive. So, if
>>> domain is running and undefine is called the file is left behind.
>>> It won't be removed in the domain cleanup process either
>>> (qemuProcessStop). To avoid this forbid undefining domain with
>>> NVRAM file.
>>
>> Why do we need to forbid it ? Even if QEMU still has an open
>> file handle, it can continue to write to it after we unlink
>> it.
>>
>>
>
> That's what my v1 does. Anyway, there's third option: just recently
> Jirka added possibility to do some actions when domain is destroyed. He
> needed it for some migration work, but the design is broad enough to fit
> this problem too. What we can do is:
>
> if (flags & VIR_DOMAIN_UNDEFINE_NVRAM):
> if domain is running:
> register the callback /* that merely just unlinks the file */
> else:
> unlink
> else:
> if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)):
> error
>
>
> What do you guys think of this one?
The callback will be lost if libvirtd restarts.
Ah, good point. In that case all I need is an ACK to v1 then ;-)
Michal
2699
days inactive
2699
days old
7 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Michal Privoznik