[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 3/4] qemu: Report better errors from broken backing chains




On 09/18/2014 05:54 AM, Peter Krempa wrote:
> Request erroring out from the backing chain traveller and drop qemu's
> internal backing chain integrity tester.
> 
> The backin chain traveller reports errors by itself with possibly more

s/backin/backing

> detail than qemuDiskChainCheckBroken ever could.
> 
> We also need to make sure that we reconnect to existing qemu instances
> even at the cost of losing the backing chain info (this really should be
> stored in the XML rather than reloaded from disk, but that needs some
> work).
> ---
>  src/qemu/qemu_domain.c  | 29 ++++-------------------------
>  src/qemu/qemu_domain.h  |  3 ++-
>  src/qemu/qemu_driver.c  | 10 +++++-----
>  src/qemu/qemu_hotplug.c |  2 +-
>  src/qemu/qemu_process.c | 11 +++++++----
>  5 files changed, 19 insertions(+), 36 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 515bcac..b93e0d5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2476,27 +2476,6 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
>      return -1;
>  }
> 
> -static int
> -qemuDiskChainCheckBroken(virDomainDiskDefPtr disk)
> -{
> -    char *brokenFile = NULL;
> -
> -    if (!virDomainDiskGetSource(disk))
> -        return 0;
> -
> -    if (virStorageFileChainGetBroken(disk->src, &brokenFile) < 0)
> -        return -1;
> -
> -    if (brokenFile) {
> -        virReportError(VIR_ERR_INVALID_ARG,
> -                       _("Backing file '%s' of image '%s' is missing."),
> -                       brokenFile, virDomainDiskGetSource(disk));
> -        VIR_FREE(brokenFile);
> -        return -1;
> -    }
> -
> -    return 0;
> -}
> 
>  int
>  qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
> @@ -2524,8 +2503,7 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
>              virFileExists(virDomainDiskGetSource(disk)))
>              continue;
> 
> -        if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0 &&
> -            qemuDiskChainCheckBroken(disk) >= 0)
> +        if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) >= 0)
>              continue;
> 
>          if (disk->startupPolicy &&
> @@ -2670,7 +2648,8 @@ int
>  qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>                               virDomainObjPtr vm,
>                               virDomainDiskDefPtr disk,
> -                             bool force_probe)
> +                             bool force_probe,
> +                             bool report_broken)
>  {
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      int ret = 0;
> @@ -2692,7 +2671,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>      if (virStorageFileGetMetadata(disk->src,
>                                    uid, gid,
>                                    cfg->allowDiskFormatProbing,
> -                                  false) < 0)
> +                                  report_broken) < 0)
>          ret = -1;
> 
>   cleanup:
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 494e9f8..cb0115a 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -366,7 +366,8 @@ int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
>  int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>                                   virDomainObjPtr vm,
>                                   virDomainDiskDefPtr disk,
> -                                 bool force_probe);
> +                                 bool force_probe,
> +                                 bool report_broken);
> 
>  int qemuDomainStorageFileInit(virQEMUDriverPtr driver,
>                                virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5fd89a3..d0aff1a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6688,7 +6688,7 @@ qemuDomainChangeDiskMediaLive(virConnectPtr conn,
>      if (virStorageTranslateDiskSourcePool(conn, disk) < 0)
>          goto end;
> 
> -    if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> +    if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
>          goto end;
> 
>      switch (disk->device) {
> @@ -13067,7 +13067,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
>      for (i = 0; i < snap->def->ndisks; i++) {
>          if (snap->def->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
>              continue;
> -        qemuDomainDetermineDiskChain(driver, vm, vm->def->disks[i], true);
> +        qemuDomainDetermineDiskChain(driver, vm, vm->def->disks[i], true, true);

We want to return failure immediately, but do nothing about it?  At the
very least an ignore_value()

>      }
>      if (orig_err) {
>          virSetError(orig_err);
> @@ -14875,7 +14875,7 @@ qemuDomainBlockPivot(virConnectPtr conn,
>          oldsrc = disk->src;
>          disk->src = disk->mirror;
> 
> -        if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> +        if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
>              goto cleanup;
> 
>          if (disk->mirror->format &&
> @@ -15388,7 +15388,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>          goto endjob;
>      }
> 
> -    if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> +    if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
>          goto endjob;
> 
>      if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) &&
> @@ -15757,7 +15757,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
>                         disk->dst);
>          goto endjob;
>      }
> -    if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> +    if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
>          goto endjob;
> 
>      if (!top)
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 7bc19cd..d631887 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -779,7 +779,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>      if (qemuSetUnprivSGIO(dev) < 0)
>          goto end;
> 
> -    if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
> +    if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
>          goto end;
> 
>      switch (disk->device)  {
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 8853d27..0f2a34f 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1090,7 +1090,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>              save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
>              disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
>              disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
> -            qemuDomainDetermineDiskChain(driver, vm, disk, true);
> +            qemuDomainDetermineDiskChain(driver, vm, disk, true, true);

Same here... We want to report_broken, but do nothing about it. At least
an ignore_value().

>          } else if (disk->mirror &&
>                     (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
>                      type == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)) {
> @@ -3428,9 +3428,12 @@ qemuProcessReconnect(void *opaque)
>          if (virStorageTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0)
>              goto error;
> 
> -        /* XXX we should be able to restore all data from XML in the future */
> -        if (qemuDomainDetermineDiskChain(driver, obj,
> -                                         obj->def->disks[i], true) < 0)
> +        /* XXX we should be able to restore all data from XML in the future.
> +         * This should be the only place that calls qemuDomainDetermineDiskChain
> +         * with @report_broken == false to guarantee best-effort domain
> +         * reconnect*/

NIT: Add space between 't' and '*'

ACK with the nits.

John

> +        if (qemuDomainDetermineDiskChain(driver, obj, obj->def->disks[i],
> +                                         true, false) < 0)
>              goto error;
> 
>          dev.type = VIR_DOMAIN_DEVICE_DISK;
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]