1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] qemu: Yet another backingStore terminator fix (blockdev-add saga)
  4. View patch

[libvirt] [PATCH 1/3] qemu: domain: Refactor control flow in qemuDomainDetermineDiskChain

Peter Krempa posted 3 patches 7 years, 5 months ago
[libvirt] [PATCH 1/3] qemu: domain: Refactor control flow in qemuDomainDetermineDiskChain
Posted by Peter Krempa 7 years, 5 months ago 
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(-)

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);

-                    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
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemu: domain: Refactor control flow in qemuDomainDetermineDiskChain
Posted by John Ferlan 7 years, 5 months ago 

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@redhat.com>

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemu: domain: Refactor control flow in qemuDomainDetermineDiskChain
Posted by Peter Krempa 7 years, 5 months ago 
On Wed, Nov 29, 2017 at 21:25:08 -0500, John Ferlan wrote:
> 
> 
> 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@redhat.com>
> 
> 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.

Yes exactly. That is desired since we need to detect the backing chain
anyways and backingStoreRaw and relPath are necessary in that case.

Also they should be NULL at this point anyways, since only the backing
chain detection code fills them.

> 
> 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?

Umm, good point, since we have the parent there we could extract it
right from it.

Semantically I've done it similarly to virStorageSourceNewFromBackingAbsolute
where the new virStorageSource is constructed only from the string.

In case of the relative relationship, we need to copy some stuff from
the parent and thus I pass it in as well.

> 
> > 
> > -                    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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.