On 11/24/2017 07:21 AM, Peter Krempa wrote:
> Split out clearing of the backing chain prior to other code since it
> will be required later and optimize few layers of nested conditions and
> loops.
> ---
> src/qemu/qemu_domain.c | 37 +++++++++++++++++--------------------
> 1 file changed, 17 insertions(+), 20 deletions(-)
>
The usage of IsBacking and HasBacking is an eye-test... Some thoughts
left below inline - to be sure I'm reading properly and also a hmmm
moment I had while reviewing...
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
John
I'm OK for freeze since this is part of a series fixing are regression
introduced during the 3.9 release (e.g. patch 3 ref to commmit id
'a693fdba').
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2bda4a726b..0e6ebdc0a8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6151,29 +6151,26 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> - if (virStorageSourceHasBacking(src)) {
> - if (force_probe) {
> - virStorageSourceBackingStoreClear(src);
> - } else {
> - /* skip to the end of the chain */
> - while (virStorageSourceIsBacking(src)) {
> - if (report_broken &&
> - virStorageFileSupportsAccess(src)) {
> -
> - if (qemuDomainStorageFileInit(driver, vm, src, disk->src) < 0)
> - goto cleanup;
> -
> - if (virStorageFileAccess(src, F_OK) < 0) {
> - virStorageFileReportBrokenChain(errno, src, disk->src);
> - virStorageFileDeinit(src);
> - goto cleanup;
> - }
> + if (force_probe)
> + virStorageSourceBackingStoreClear(src);
A "slight variation" in the new code is that for force_probe the Clear
would be tried regardless of whether there is a backingStore and the
src->type could be NONE meaning that the VIR_FREE on src->relPath and
src->backingStoreRaw would happen which is I think expected because we'd
be about to at least fill in backingStoreRaw and relPath is a derivation
of that anyway.
As an aside looking at virStorageSourceNewFromBackingRelative (where
relPath can be filled in) - I'm wondering why parent->backingStoreRaw is
passed as @rel but also STRDUP'd into ret->relPath... IOW: is passing
backingStoreRaw necessary?
>
> - virStorageFileDeinit(src);
> - }
> - src = src->backingStore;
> + /* skip to the end of the chain if there is any */
> + while (virStorageSourceHasBacking(src)) {
> + if (report_broken &&
> + virStorageFileSupportsAccess(src)) {
> +
> + if (qemuDomainStorageFileInit(driver, vm, src, disk->src) < 0)
> + goto cleanup;
> +
> + if (virStorageFileAccess(src, F_OK) < 0) {
> + virStorageFileReportBrokenChain(errno, src, disk->src);
> + virStorageFileDeinit(src);
> + goto cleanup;
> }
> +
> + virStorageFileDeinit(src);
> }
> + src = src->backingStore;
> }
>
> /* We skipped to the end of the chain. Skip detection if there's the
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
