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@xxxxxxxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list