[libvirt] [PATCH 1/2] qemu: make explicit that formatting migratable imposes secure

Nikolay Shirokovskiy posted 2 patches 8 years, 4 months ago
[libvirt] [PATCH 1/2] qemu: make explicit that formatting migratable imposes secure
Posted by Nikolay Shirokovskiy 8 years, 4 months ago
qemu code always set VIR_DOMAIN_XML_SECURE flags when VIR_DOMAIN_XML_MIGRATABLE
is set. At the same time qemu code itself does not analyse VIR_DOMAIN_XML_SECURE
presense in any way. Thus we can just append secure flag conditionally
down the stack before call to virDomainDefFormatInternal which actually checks
secure flag. This drops a couple of adhoc macros also.

I would even put appending secure flag if migratable is present into
virDomainDefFormatConvertXMLFlags but not all hypervisors drivers agree on
this point.
---
 src/qemu/qemu_domain.c           | 27 ++++++++++++++-------------
 src/qemu/qemu_domain.h           |  4 ----
 src/qemu/qemu_driver.c           |  5 ++---
 src/qemu/qemu_migration.c        |  4 +---
 src/qemu/qemu_migration_cookie.c |  1 -
 5 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 50b536e..6b458c1 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4682,6 +4682,9 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
     }
 
  format:
+    if (flags & VIR_DOMAIN_XML_MIGRATABLE)
+        flags |= VIR_DOMAIN_XML_SECURE;
+
     ret = virDomainDefFormatInternal(def, caps,
                                      virDomainDefFormatConvertXMLFlags(flags),
                                      buf);
@@ -4762,10 +4765,10 @@ qemuDomainDefFormatLive(virQEMUDriverPtr driver,
                         bool inactive,
                         bool compatible)
 {
-    unsigned int flags = QEMU_DOMAIN_FORMAT_LIVE_FLAGS;
+    unsigned int flags = VIR_DOMAIN_XML_UPDATE_CPU;
 
     if (inactive)
-        flags |= VIR_DOMAIN_XML_INACTIVE;
+        flags |= VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE;
     if (compatible)
         flags |= VIR_DOMAIN_XML_MIGRATABLE;
 
@@ -5212,8 +5215,8 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
     virUUIDFormat(vm->def->uuid, uuidstr);
     newxml = virDomainSnapshotDefFormat(
         uuidstr, snapshot->def, caps, xmlopt,
-        virDomainDefFormatConvertXMLFlags(QEMU_DOMAIN_FORMAT_LIVE_FLAGS),
-        1);
+        virDomainDefFormatConvertXMLFlags(VIR_DOMAIN_XML_SECURE |
+                                          VIR_DOMAIN_XML_UPDATE_CPU), 1);
     if (newxml == NULL)
         return -1;
 
@@ -6263,9 +6266,6 @@ qemuDomainMigratableDefCheckABIStability(virQEMUDriverPtr driver,
 }
 
 
-#define COPY_FLAGS (VIR_DOMAIN_XML_SECURE | \
-                    VIR_DOMAIN_XML_MIGRATABLE)
-
 bool
 qemuDomainDefCheckABIStability(virQEMUDriverPtr driver,
                                virDomainDefPtr src,
@@ -6275,8 +6275,10 @@ qemuDomainDefCheckABIStability(virQEMUDriverPtr driver,
     virDomainDefPtr migratableDefDst = NULL;
     bool ret = false;
 
-    if (!(migratableDefSrc = qemuDomainDefCopy(driver, src, COPY_FLAGS)) ||
-        !(migratableDefDst = qemuDomainDefCopy(driver, dst, COPY_FLAGS)))
+    if (!(migratableDefSrc = qemuDomainDefCopy(driver, src,
+                                               VIR_DOMAIN_XML_MIGRATABLE)) ||
+        !(migratableDefDst = qemuDomainDefCopy(driver, dst,
+                                               VIR_DOMAIN_XML_MIGRATABLE)))
         goto cleanup;
 
     ret = qemuDomainMigratableDefCheckABIStability(driver,
@@ -6300,9 +6302,10 @@ qemuDomainCheckABIStability(virQEMUDriverPtr driver,
     char *xml = NULL;
     bool ret = false;
 
-    if (!(xml = qemuDomainFormatXML(driver, vm, COPY_FLAGS)) ||
+    if (!(xml = qemuDomainFormatXML(driver, vm, VIR_DOMAIN_XML_MIGRATABLE)) ||
         !(migratableSrc = qemuDomainDefFromXML(driver, xml)) ||
-        !(migratableDst = qemuDomainDefCopy(driver, dst, COPY_FLAGS)))
+        !(migratableDst = qemuDomainDefCopy(driver, dst,
+                                            VIR_DOMAIN_XML_MIGRATABLE)))
         goto cleanup;
 
     ret = qemuDomainMigratableDefCheckABIStability(driver,
@@ -6316,8 +6319,6 @@ qemuDomainCheckABIStability(virQEMUDriverPtr driver,
     return ret;
 }
 
-#undef COPY_FLAGS
-
 
 bool
 qemuDomainAgentAvailable(virDomainObjPtr vm,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index b291dc3..7c2f91a 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -39,10 +39,6 @@
 # include "virobject.h"
 # include "logging/log_manager.h"
 
-# define QEMU_DOMAIN_FORMAT_LIVE_FLAGS      \
-    (VIR_DOMAIN_XML_SECURE |                \
-     VIR_DOMAIN_XML_UPDATE_CPU)
-
 # if ULONG_MAX == 4294967295
 /* QEMU has a 64-bit limit, but we are limited by our historical choice of
  * representing bandwidth in a long instead of a 64-bit int.  */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9a21995..d9dff93 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6282,7 +6282,7 @@ qemuDomainSaveImageUpdateDef(virQEMUDriverPtr driver,
 
     if (!(newdef_migr = qemuDomainDefCopy(driver,
                                           newdef,
-                                          QEMU_DOMAIN_FORMAT_LIVE_FLAGS |
+                                          VIR_DOMAIN_XML_UPDATE_CPU |
                                           VIR_DOMAIN_XML_MIGRATABLE)))
         goto cleanup;
 
@@ -6789,7 +6789,6 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path,
 
     if (!(data->xml = qemuDomainDefFormatXML(driver, newdef,
                                              VIR_DOMAIN_XML_INACTIVE |
-                                             VIR_DOMAIN_XML_SECURE |
                                              VIR_DOMAIN_XML_MIGRATABLE)))
         goto cleanup;
 
@@ -6995,7 +6994,7 @@ static char
         goto cleanup;
 
     if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
-        flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS;
+        flags |= VIR_DOMAIN_XML_UPDATE_CPU;
 
     ret = qemuDomainFormatXML(driver, vm, flags);
 
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 492815b..db0b4c5 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2575,7 +2575,6 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
         int hookret;
 
         if (!(xml = qemuDomainDefFormatXML(driver, *def,
-                                           VIR_DOMAIN_XML_SECURE |
                                            VIR_DOMAIN_XML_MIGRATABLE)))
             goto cleanup;
 
@@ -3639,7 +3638,6 @@ qemuMigrationRun(virQEMUDriverPtr driver,
         } else {
             virDomainDefPtr def = vm->newDef ? vm->newDef : vm->def;
             if (!(persistDef = qemuDomainDefCopy(driver, def,
-                                                 VIR_DOMAIN_XML_SECURE |
                                                  VIR_DOMAIN_XML_MIGRATABLE)))
                 goto cleanup;
         }
@@ -4108,7 +4106,7 @@ static int doPeer2PeerMigrate2(virQEMUDriverPtr driver,
      * and pass it to Prepare2.
      */
     if (!(dom_xml = qemuDomainFormatXML(driver, vm,
-                                        QEMU_DOMAIN_FORMAT_LIVE_FLAGS |
+                                        VIR_DOMAIN_XML_UPDATE_CPU |
                                         VIR_DOMAIN_XML_MIGRATABLE)))
         return -1;
 
diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c
index 4914c77..38213ae 100644
--- a/src/qemu/qemu_migration_cookie.c
+++ b/src/qemu/qemu_migration_cookie.c
@@ -738,7 +738,6 @@ qemuMigrationCookieXMLFormat(virQEMUDriverPtr driver,
         if (qemuDomainDefFormatBuf(driver,
                                    mig->persistent,
                                    VIR_DOMAIN_XML_INACTIVE |
-                                   VIR_DOMAIN_XML_SECURE |
                                    VIR_DOMAIN_XML_MIGRATABLE,
                                    buf) < 0)
             return -1;
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: make explicit that formatting migratable imposes secure
Posted by Jiri Denemark 8 years, 4 months ago
On Thu, Sep 21, 2017 at 16:39:38 +0300, Nikolay Shirokovskiy wrote:
> qemu code always set VIR_DOMAIN_XML_SECURE flags when VIR_DOMAIN_XML_MIGRATABLE
> is set. At the same time qemu code itself does not analyse VIR_DOMAIN_XML_SECURE
> presense in any way. Thus we can just append secure flag conditionally
> down the stack before call to virDomainDefFormatInternal which actually checks
> secure flag. This drops a couple of adhoc macros also.

I think the adhoc macros are actually useful. They make sure that all
places where a definition is copied or formatted in a certain way will
use the same flags. Even if it's just a single flag.

> I would even put appending secure flag if migratable is present into
> virDomainDefFormatConvertXMLFlags but not all hypervisors drivers agree on
> this point.
...
> @@ -6995,7 +6994,7 @@ static char
>          goto cleanup;
>  
>      if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
> -        flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS;
> +        flags |= VIR_DOMAIN_XML_UPDATE_CPU;

Oh I se what was the local change now. The annoyingly slow mail delivery
from the mailing list caused me to see 2/2 without the rest of the
series.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: make explicit that formatting migratable imposes secure
Posted by Nikolay Shirokovskiy 8 years, 4 months ago

On 21.09.2017 17:32, Jiri Denemark wrote:
> On Thu, Sep 21, 2017 at 16:39:38 +0300, Nikolay Shirokovskiy wrote:
>> qemu code always set VIR_DOMAIN_XML_SECURE flags when VIR_DOMAIN_XML_MIGRATABLE
>> is set. At the same time qemu code itself does not analyse VIR_DOMAIN_XML_SECURE
>> presense in any way. Thus we can just append secure flag conditionally
>> down the stack before call to virDomainDefFormatInternal which actually checks
>> secure flag. This drops a couple of adhoc macros also.
> 
> I think the adhoc macros are actually useful. They make sure that all
> places where a definition is copied or formatted in a certain way will
> use the same flags. Even if it's just a single flag.

It is just flag QEMU_DOMAIN_FORMAT_LIVE_FLAGS was not very clear to me.
In one place it combined with VIR_DOMAIN_XML_INACTIVE which looks odd.

But the main purpose of this patch is not to remove macros but rather
to clarify the fact that migratable flag is always used with together
with secure flag.

Nikolay

> 
>> I would even put appending secure flag if migratable is present into
>> virDomainDefFormatConvertXMLFlags but not all hypervisors drivers agree on
>> this point.
> ...
>> @@ -6995,7 +6994,7 @@ static char
>>          goto cleanup;
>>  
>>      if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
>> -        flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS;
>> +        flags |= VIR_DOMAIN_XML_UPDATE_CPU;
> 
> Oh I se what was the local change now. The annoyingly slow mail delivery
> from the mailing list caused me to see 2/2 without the rest of the
> series.
> 
> Jirka
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list