[libvirt] [PATCH] qemu: save config after pivot only if domain is persistent

[Peter Krempa]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160104/a837ae0d/attachment-0001.sig>