11:42 p.m.
When pivoting after a completed block job, only save the domain's
persistent configuration if the domain is actually marked persistent.
This commit also refactors the logic surrounding the copying of the new
disk definition into vm->newDef to avoid a NULL pointer dereference if
virStorageSourceCopy were to fail to allocate memory.
Signed-off-by: Michael Chapman <mike(a)very.puzzling.org>
---
src/qemu/qemu_blockjob.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 1d5b7ce..ae936a2 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -116,26 +116,20 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
switch ((virConnectDomainEventBlockJobStatus) status) {
case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {
- if (vm->newDef) {
- virStorageSourcePtr copy = NULL;
-
- if ((persistDisk = virDomainDiskByName(vm->newDef,
- disk->dst, false))) {
- copy = virStorageSourceCopy(disk->mirror, false);
- if (virStorageSourceInitChainElement(copy,
- persistDisk->src,
- true) < 0) {
- VIR_WARN("Unable to update persistent definition "
- "on vm %s after block job",
- vm->def->name);
- virStorageSourceFree(copy);
- copy = NULL;
- persistDisk = NULL;
- }
- }
- if (copy) {
+ if (vm->newDef &&
+ (persistDisk = virDomainDiskByName(vm->newDef, disk->dst, false)))
{
+ virStorageSourcePtr copy = virStorageSourceCopy(disk->mirror, false);
+ if (copy && virStorageSourceInitChainElement(copy,
+ persistDisk->src,
+ true) == 0) {
virStorageSourceFree(persistDisk->src);
persistDisk->src = copy;
+ } else {
+ VIR_WARN("Unable to update persistent definition "
+ "on vm %s after block job",
+ vm->def->name);
+ virStorageSourceFree(copy);
+ persistDisk = NULL;
}
}
@@ -188,8 +182,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
VIR_WARN("Unable to save status on vm %s after block job",
vm->def->name);
- if (persistDisk && virDomainSaveConfig(cfg->configDir,
- vm->newDef) < 0)
+ if (vm->persistent && persistDisk &&
+ virDomainSaveConfig(cfg->configDir, vm->newDef) < 0)
VIR_WARN("Unable to update persistent definition on vm %s "
"after block job", vm->def->name);
}
--
2.4.3
9:18 a.m.
On Thu, Dec 31, 2015 at 16:42:52 +1100, Michael Chapman wrote:
When pivoting after a completed block job, only save the
domain's
persistent configuration if the domain is actually marked persistent.
This commit also refactors the logic surrounding the copying of the new
disk definition into vm->newDef to avoid a NULL pointer dereference if
virStorageSourceCopy were to fail to allocate memory.
This commit is fixing two separate things in one commit, which is not
really good practice. Please repost this as two patches.
Signed-off-by: Michael Chapman <mike(a)very.puzzling.org>
---
src/qemu/qemu_blockjob.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 1d5b7ce..ae936a2 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -116,26 +116,20 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
switch ((virConnectDomainEventBlockJobStatus) status) {
case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {
- if (vm->newDef) {
- virStorageSourcePtr copy = NULL;
-
- if ((persistDisk = virDomainDiskByName(vm->newDef,
- disk->dst, false))) {
- copy = virStorageSourceCopy(disk->mirror, false);
- if (virStorageSourceInitChainElement(copy,
- persistDisk->src,
- true) < 0) {
- VIR_WARN("Unable to update persistent definition "
- "on vm %s after block job",
- vm->def->name);
- virStorageSourceFree(copy);
- copy = NULL;
- persistDisk = NULL;
- }
- }
- if (copy) {
+ if (vm->newDef &&
+ (persistDisk = virDomainDiskByName(vm->newDef, disk->dst, false)))
{
+ virStorageSourcePtr copy = virStorageSourceCopy(disk->mirror,
false);
+ if (copy && virStorageSourceInitChainElement(copy,
+ persistDisk->src,
+ true) == 0) {a
The new code will result in a line exceeding 80 cols. Since it's not
very long, it's acceptable though.
virStorageSourceFree(persistDisk->src);
persistDisk->src = copy;
+ } else {
+ VIR_WARN("Unable to update persistent definition "
+ "on vm %s after block job",
+ vm->def->name);
+ virStorageSourceFree(copy);
+ persistDisk = NULL;
}
}
@@ -188,8 +182,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
VIR_WARN("Unable to save status on vm %s after block job",
vm->def->name);
- if (persistDisk && virDomainSaveConfig(cfg->configDir,
- vm->newDef) < 0)
+ if (vm->persistent && persistDisk &&
I'm not quite sure how this would happen. If there isn't a persistent
configuration, how did we get to having the persistDisk object?
If this happened in some way, the problem then is that vm->newDef was
not freed or is present in any way so we should fix the origin and not
this symptom.
+ virDomainSaveConfig(cfg->configDir, vm->newDef)
< 0)
VIR_WARN("Unable to update persistent definition on vm %s "
"after block job", vm->def->name);
}
Peter
6:38 p.m.
On Mon, 4 Jan 2016, Peter Krempa wrote:
On Thu, Dec 31, 2015 at 16:42:52 +1100, Michael Chapman wrote:
> When pivoting after a completed block job, only save the domain's
> persistent configuration if the domain is actually marked persistent.
>
> This commit also refactors the logic surrounding the copying of the new
> disk definition into vm->newDef to avoid a NULL pointer dereference if
> virStorageSourceCopy were to fail to allocate memory.
This commit is fixing two separate things in one commit, which is not
really good practice. Please repost this as two patches.
No problem, I can do that.
>
> Signed-off-by: Michael Chapman <mike(a)very.puzzling.org>
> ---
> src/qemu/qemu_blockjob.c | 34 ++++++++++++++--------------------
> 1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index 1d5b7ce..ae936a2 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -116,26 +116,20 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
> switch ((virConnectDomainEventBlockJobStatus) status) {
> case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
> if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {
> - if (vm->newDef) {
> - virStorageSourcePtr copy = NULL;
> -
> - if ((persistDisk = virDomainDiskByName(vm->newDef,
> - disk->dst, false))) {
> - copy = virStorageSourceCopy(disk->mirror, false);
> - if (virStorageSourceInitChainElement(copy,
> - persistDisk->src,
> - true) < 0) {
> - VIR_WARN("Unable to update persistent definition
"
> - "on vm %s after block job",
> - vm->def->name);
> - virStorageSourceFree(copy);
> - copy = NULL;
> - persistDisk = NULL;
> - }
> - }
> - if (copy) {
> + if (vm->newDef &&
> + (persistDisk = virDomainDiskByName(vm->newDef, disk->dst,
false))) {
> + virStorageSourcePtr copy = virStorageSourceCopy(disk->mirror,
false);
> + if (copy && virStorageSourceInitChainElement(copy,
> + persistDisk->src,
> + true) == 0) {a
The new code will result in a line exceeding 80 cols. Since it's not
very long, it's acceptable though.
> virStorageSourceFree(persistDisk->src);
> persistDisk->src = copy;
> + } else {
> + VIR_WARN("Unable to update persistent definition "
> + "on vm %s after block job",
> + vm->def->name);
> + virStorageSourceFree(copy);
> + persistDisk = NULL;
> }
> }
>
> @@ -188,8 +182,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
> VIR_WARN("Unable to save status on vm %s after block job",
> vm->def->name);
> - if (persistDisk && virDomainSaveConfig(cfg->configDir,
> - vm->newDef) < 0)
> + if (vm->persistent && persistDisk &&
I'm not quite sure how this would happen. If there isn't a persistent
configuration, how did we get to having the persistDisk object?
If this happened in some way, the problem then is that vm->newDef was
not freed or is present in any way so we should fix the origin and not
this symptom.
I spent a lot of time trying to work this out myself, and although I can
see what the code is doing I don't really understand the rationale or
history behind it all.
vm->newDef is supposed to be the "new domain definition to activate on
shutdown", according to domain_conf.h. What's confusing though is that it
is possible for this to exist even on transient domains.
qemuProcessInit calls virDomainObjSetDefTransient ("so any part of the
startup process can add runtime state to vm->def that won't be
persisted"), and virDomainObjSetDefTransient actually *sets* vm->newDef.
If the domain is subsequently undefined, vm->persistent is set to 0 but
vm->newDef is left alone.
So it's possible for vm->newDef to be non-NULL even though
vm->persistent is 0.
I had initially thought about putting the vm->persistent test at the top
of this code block, so persistDisk was never set if the domain was
transient. However the problem with that approach is that it means
vm->newDef never gets updated at the completion of the block job, yet it's
still possible to get this "inactive" domain XML via virDomainGetXMLDesc.
I thought it would be simplest and safest to keep updating vm->newDef as
before, but just not write out the config to disk if the domain wasn't
actually persistent.
> + virDomainSaveConfig(cfg->configDir,
vm->newDef) < 0)
> VIR_WARN("Unable to update persistent definition on vm %s "
> "after block job", vm->def->name);
> }
Peter
- Michael
12:01 a.m.
On Tue, Jan 05, 2016 at 11:38:07 +1100, Michael Chapman wrote:
On Mon, 4 Jan 2016, Peter Krempa wrote:
> On Thu, Dec 31, 2015 at 16:42:52 +1100, Michael Chapman wrote:
...
>> @@ -188,8 +182,8 @@
qemuBlockJobEventProcess(virQEMUDriverPtr driver,
>> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) <
0)
>> VIR_WARN("Unable to save status on vm %s after block
job",
>> vm->def->name);
>> - if (persistDisk && virDomainSaveConfig(cfg->configDir,
>> - vm->newDef) < 0)
>> + if (vm->persistent && persistDisk &&
>
> I'm not quite sure how this would happen. If there isn't a persistent
> configuration, how did we get to having the persistDisk object?
>
> If this happened in some way, the problem then is that vm->newDef was
> not freed or is present in any way so we should fix the origin and not
> this symptom.
I spent a lot of time trying to work this out myself, and although I can
see what the code is doing I don't really understand the rationale or
history behind it all.
vm->newDef is supposed to be the "new domain definition to activate on
shutdown", according to domain_conf.h. What's confusing though is that it
is possible for this to exist even on transient domains.
That is not confusing but a bug. The newDef should not exist if the
domain is transient.
Basically the relationship between vm->persistent and vm->def and
vm->newDef should be that vm->newDef implies vm->persistent. (Offline
vms can't be transient)
qemuProcessInit calls virDomainObjSetDefTransient ("so any part of the
startup process can add runtime state to vm->def that won't be
persisted"), and virDomainObjSetDefTransient actually *sets* vm->newDef.
If the domain is subsequently undefined, vm->persistent is set to 0 but
vm->newDef is left alone.
So this is probably the buggy code path. Setting vm->newDef in
virDomainObjSetDefTransient is desired so that we can fill in a lot of
runtime stuff into the definition object without persisting it. The
function even checks whether the object is persistent and does not
perform the copy in such place.
So it's possible for vm->newDef to be non-NULL even though
vm->persistent is 0.
Bug.
I had initially thought about putting the vm->persistent test at the top
of this code block, so persistDisk was never set if the domain was
transient. However the problem with that approach is that it means
vm->newDef never gets updated at the completion of the block job, yet it's
still possible to get this "inactive" domain XML via virDomainGetXMLDesc.
Not really. To be consistent the part that sets the domain to be
transient should kill the persistent definition. Otherwise that would
imply that we need a lot of fallback code stuff to handle the corner
case when the vm was undefined while running.
I thought it would be simplest and safest to keep updating vm->newDef as
before, but just not write out the config to disk if the domain wasn't
actually persistent.
As a side note, all the naming of def, newDef and
virDomainObjSetDefTransient is very unfortunate. Having a persistentDef
and liveDef or similarly named pointers would be more clear, easier to
differentiate states and would additionally allow to get rid of
vm->persistent since it would be equal to vm->persistentDef being set or
not.
If you want to fix the undefine bug you are welcome, otherwise I'll do
it in a few days.
Peter
9:11 p.m.
On Wed, 6 Jan 2016, Peter Krempa wrote:
On Tue, Jan 05, 2016 at 11:38:07 +1100, Michael Chapman wrote:
> On Mon, 4 Jan 2016, Peter Krempa wrote:
>> On Thu, Dec 31, 2015 at 16:42:52 +1100, Michael Chapman wrote:
...
>>> @@ -188,8 +182,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
>>> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm)
< 0)
>>> VIR_WARN("Unable to save status on vm %s after block
job",
>>> vm->def->name);
>>> - if (persistDisk && virDomainSaveConfig(cfg->configDir,
>>> - vm->newDef) < 0)
>>> + if (vm->persistent && persistDisk &&
>>
>> I'm not quite sure how this would happen. If there isn't a persistent
>> configuration, how did we get to having the persistDisk object?
>>
>> If this happened in some way, the problem then is that vm->newDef was
>> not freed or is present in any way so we should fix the origin and not
>> this symptom.
>
> I spent a lot of time trying to work this out myself, and although I can
> see what the code is doing I don't really understand the rationale or
> history behind it all.
>
> vm->newDef is supposed to be the "new domain definition to activate on
> shutdown", according to domain_conf.h. What's confusing though is that it
> is possible for this to exist even on transient domains.
That is not confusing but a bug. The newDef should not exist if the
domain is transient.
Well, it was certainly confusing to me, but that's probably because I was
assuming the code was correct and it was just my understanding of it that
was wrong. :-)
[...]
As a side note, all the naming of def, newDef and
virDomainObjSetDefTransient is very unfortunate. Having a persistentDef
and liveDef or similarly named pointers would be more clear, easier to
differentiate states and would additionally allow to get rid of
vm->persistent since it would be equal to vm->persistentDef being set or
not.
If you want to fix the undefine bug you are welcome, otherwise I'll do
it in a few days.
Peter
I would be more comfortable if you were to fix up the undefine bug. You've
clearly got a better understanding of how it's all supposed to work.
However, I will send through a patch fixing the possible NULL dereference
should virStorageSourceCopy fail.
- Michael
3242
days inactive
3249
days old
4 comments
2 participants
participants (2)
-
Michael Chapman -
Peter Krempa