[PATCH v3 3/4] qemu: adopt to VIR_DRV_SUPPORTS_FEATURE return -1

Nikolay Shirokovskiy posted 4 patches 5 years, 1 month ago
[PATCH v3 3/4] qemu: adopt to VIR_DRV_SUPPORTS_FEATURE return -1
Posted by Nikolay Shirokovskiy 5 years, 1 month ago
Otherwise in some places we can mistakenly report 'unsupported' error instead
of root cause. So let's handle root cause explicitly from the macro.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 90b0ec9..fca21be 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -4706,12 +4706,13 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver,
 {
     int ret = -1;
     g_autoptr(virConnect) dconn = NULL;
-    bool p2p;
+    int p2p;
     virErrorPtr orig_err = NULL;
     bool offline = !!(flags & VIR_MIGRATE_OFFLINE);
-    bool dstOffline = false;
+    int dstOffline;
     g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-    bool useParams;
+    int useParams;
+    int rc;
 
     VIR_DEBUG("driver=%p, sconn=%p, vm=%p, xmlin=%s, dconnuri=%s, uri=%s, "
               "graphicsuri=%s, listenAddress=%s, nmigrate_disks=%zu, "
@@ -4771,17 +4772,27 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver,
     qemuDomainObjEnterRemote(vm);
     p2p = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
                                    VIR_DRV_FEATURE_MIGRATION_P2P);
-        /* v3proto reflects whether the caller used Perform3, but with
-         * p2p migrate, regardless of whether Perform2 or Perform3
-         * were used, we decide protocol based on what target supports
-         */
-    *v3proto = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
-                                        VIR_DRV_FEATURE_MIGRATION_V3);
+    if (p2p < 0)
+        goto cleanup;
+    /* v3proto reflects whether the caller used Perform3, but with
+     * p2p migrate, regardless of whether Perform2 or Perform3
+     * were used, we decide protocol based on what target supports
+     */
+    rc = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
+                                  VIR_DRV_FEATURE_MIGRATION_V3);
+    if (rc < 0)
+        goto cleanup;
+    *v3proto = !!rc;
     useParams = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
                                          VIR_DRV_FEATURE_MIGRATION_PARAMS);
-    if (offline)
+    if (useParams < 0)
+        goto cleanup;
+    if (offline) {
         dstOffline = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
                                               VIR_DRV_FEATURE_MIGRATION_OFFLINE);
+        if (dstOffline < 0)
+            goto cleanup;
+    }
     if (qemuDomainObjExitRemote(vm, !offline) < 0)
         goto cleanup;
 
@@ -4819,7 +4830,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver,
                                                 persist_xml, dname, uri, graphicsuri,
                                                 listenAddress, nmigrate_disks, migrate_disks,
                                                 nbdPort, nbdURI, migParams, resource,
-                                                useParams, flags);
+                                                !!useParams, flags);
     } else {
         ret = qemuMigrationSrcPerformPeer2Peer2(driver, sconn, dconn, vm,
                                                 dconnuri, flags, dname, resource,
-- 
1.8.3.1

Re: [PATCH v3 3/4] qemu: adopt to VIR_DRV_SUPPORTS_FEATURE return -1
Posted by Daniel P. Berrangé 5 years, 1 month ago
On Fri, Dec 18, 2020 at 09:56:47AM +0300, Nikolay Shirokovskiy wrote:
> Otherwise in some places we can mistakenly report 'unsupported' error instead
> of root cause. So let's handle root cause explicitly from the macro.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>  src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 3/4] qemu: adopt to VIR_DRV_SUPPORTS_FEATURE return -1
Posted by Daniel P. Berrangé 5 years, 1 month ago
On Fri, Dec 18, 2020 at 09:56:47AM +0300, Nikolay Shirokovskiy wrote:
> Otherwise in some places we can mistakenly report 'unsupported' error instead
> of root cause. So let's handle root cause explicitly from the macro.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>  src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 90b0ec9..fca21be 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -4706,12 +4706,13 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver,
>  {
>      int ret = -1;
>      g_autoptr(virConnect) dconn = NULL;
> -    bool p2p;
> +    int p2p;
>      virErrorPtr orig_err = NULL;
>      bool offline = !!(flags & VIR_MIGRATE_OFFLINE);
> -    bool dstOffline = false;
> +    int dstOffline;

Loosing the initializer made the compiler unhappy it seems

../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c: In function ‘qemuMigrationSrcPerformJob’:
../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c:4814:20: error: ‘dstOffline’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 4814 |     if (offline && !dstOffline) {
      |                    ^~~~~~~~~~~
../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c:4712:9: note: ‘dstOffline’ was declared here
 4712 |     int dstOffline;
      |         ^~~~~~~~~~


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 3/4] qemu: adopt to VIR_DRV_SUPPORTS_FEATURE return -1
Posted by Nikolay Shirokovskiy 5 years, 1 month ago
Sorry, looks like I'm compiling with debug options and missed this error.
It was a deliberate change but compiler don't like it.

The build fix is pushed.

Nikolay

On Wed, Jan 6, 2021 at 5:44 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, Dec 18, 2020 at 09:56:47AM +0300, Nikolay Shirokovskiy wrote:
> > Otherwise in some places we can mistakenly report 'unsupported' error
> instead
> > of root cause. So let's handle root cause explicitly from the macro.
> >
> > Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> > ---
> >  src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++-----------
> >  1 file changed, 22 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 90b0ec9..fca21be 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -4706,12 +4706,13 @@
> qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver,
> >  {
> >      int ret = -1;
> >      g_autoptr(virConnect) dconn = NULL;
> > -    bool p2p;
> > +    int p2p;
> >      virErrorPtr orig_err = NULL;
> >      bool offline = !!(flags & VIR_MIGRATE_OFFLINE);
> > -    bool dstOffline = false;
> > +    int dstOffline;
>
> Loosing the initializer made the compiler unhappy it seems
>
> ../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c: In function
> ‘qemuMigrationSrcPerformJob’:
> ../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c:4814:20: error:
> ‘dstOffline’ may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>  4814 |     if (offline && !dstOffline) {
>       |                    ^~~~~~~~~~~
> ../dist-unpack/libvirt-7.0.0/src/qemu/qemu_migration.c:4712:9: note:
> ‘dstOffline’ was declared here
>  4712 |     int dstOffline;
>       |         ^~~~~~~~~~
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>