1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/9] qemu: sav/restore inactive persistent config
  4. View patch

[libvirt] [PATCH 8/9] conf: snapshot: support persistent config on redefine

Nikolay Shirokovskiy posted 9 patches 5 years, 11 months ago
[libvirt] [PATCH 8/9] conf: snapshot: support persistent config on redefine
Posted by Nikolay Shirokovskiy 5 years, 11 months ago 
This patch just adds basic checks for persistent domain config
on snapshot metadata redefine. It also lets use previous version
of config if it exists in previous version of metadata and
not explicitly specified in new one just as in case of top level
config.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 src/conf/snapshot_conf.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index fa1cd6b..b003a07 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -1315,6 +1315,22 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
         goto cleanup;
     }
 
+    if (def->persistDom &&
+        memcmp(def->persistDom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("persistent definition for snapshot %s must use uuid %s"),
+                       def->name, uuidstr);
+        goto cleanup;
+    }
+
+    if (def->persistDom &&
+        STRNEQ(def->persistDom->name, domain->name)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("persistent definition for snapshot %s must use name %s"),
+                       def->name, domain->name);
+        goto cleanup;
+    }
+
     other = virDomainSnapshotFindByName(vm->snapshots, def->name);
     if (other) {
         if ((other->def->state == VIR_DOMAIN_RUNNING ||
@@ -1349,6 +1365,11 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
             }
         }
 
+        if (other->def->persistDom && !def->persistDom) {
+            def->persistDom = other->def->persistDom;
+            other->def->persistDom = NULL;
+        }
+
         if (def->dom) {
             if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
                 virDomainSnapshotDefIsExternal(def)) {
@@ -1363,6 +1384,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
                     other->def->dom = def->dom;
                     def->dom = NULL;
                 }
+                if (def->persistDom && !other->def->persistDom) {
+                    other->def->persistDom = def->persistDom;
+                    def->persistDom = NULL;
+                }
                 goto cleanup;
             }
         }
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] conf: snapshot: support persistent config on redefine
Posted by John Ferlan 5 years, 11 months ago 

On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
> This patch just adds basic checks for persistent domain config
> on snapshot metadata redefine. It also lets use previous version
> of config if it exists in previous version of metadata and
> not explicitly specified in new one just as in case of top level
> config.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>  src/conf/snapshot_conf.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 

Should this actually be part of patch5? Although nice to have patches
separated for review - functionality wise it's paired with managing the
persistDom for the snapshot_conf.


> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index fa1cd6b..b003a07 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -1315,6 +1315,22 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>          goto cleanup;
>      }
>  
> +    if (def->persistDom &&
> +        memcmp(def->persistDom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("persistent definition for snapshot %s must use uuid %s"),
> +                       def->name, uuidstr);
> +        goto cleanup;
> +    }
> +
> +    if (def->persistDom &&
> +        STRNEQ(def->persistDom->name, domain->name)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("persistent definition for snapshot %s must use name %s"),
> +                       def->name, domain->name);
> +        goto cleanup;
> +    }
> +

Could go with the:

    if (def->persistDom) {
    }

here too, but IDC that much.

Based on any fallout from other comments received on patch7 and since
this essentially "copies" things for the persistDom,

>      other = virDomainSnapshotFindByName(vm->snapshots, def->name);
>      if (other) {
>          if ((other->def->state == VIR_DOMAIN_RUNNING ||
> @@ -1349,6 +1365,11 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>              }
>          }
>  
> +        if (other->def->persistDom && !def->persistDom) {
> +            def->persistDom = other->def->persistDom;
> +            other->def->persistDom = NULL;
> +        }

Isn't this just

    VIR_STEAL_PTR(def->persistDom, other->def->persistDom);

?

> +
>          if (def->dom) {
>              if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
>                  virDomainSnapshotDefIsExternal(def)) {
> @@ -1363,6 +1384,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>                      other->def->dom = def->dom;
>                      def->dom = NULL;
>                  }
> +                if (def->persistDom && !other->def->persistDom) {
> +                    other->def->persistDom = def->persistDom;
> +                    def->persistDom = NULL;
> +                }

Likewise...

>                  goto cleanup;
>              }
>          }
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] conf: snapshot: support persistent config on redefine
Posted by Nikolay Shirokovskiy 5 years, 11 months ago 

On 21.12.2018 00:15, John Ferlan wrote:
> 
> 
> On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
>> This patch just adds basic checks for persistent domain config
>> on snapshot metadata redefine. It also lets use previous version
>> of config if it exists in previous version of metadata and
>> not explicitly specified in new one just as in case of top level
>> config.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>> ---
>>  src/conf/snapshot_conf.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
> 
> Should this actually be part of patch5? Although nice to have patches
> separated for review - functionality wise it's paired with managing the
> persistDom for the snapshot_conf.
> 
> 
>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>> index fa1cd6b..b003a07 100644
>> --- a/src/conf/snapshot_conf.c
>> +++ b/src/conf/snapshot_conf.c
>> @@ -1315,6 +1315,22 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>>          goto cleanup;
>>      }
>>  
>> +    if (def->persistDom &&
>> +        memcmp(def->persistDom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
>> +        virReportError(VIR_ERR_INVALID_ARG,
>> +                       _("persistent definition for snapshot %s must use uuid %s"),
>> +                       def->name, uuidstr);
>> +        goto cleanup;
>> +    }
>> +
>> +    if (def->persistDom &&
>> +        STRNEQ(def->persistDom->name, domain->name)) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                       _("persistent definition for snapshot %s must use name %s"),
>> +                       def->name, domain->name);
>> +        goto cleanup;
>> +    }
>> +
> 
> Could go with the:
> 
>     if (def->persistDom) {
>     }
> 
> here too, but IDC that much.
> 
> Based on any fallout from other comments received on patch7 and since
> this essentially "copies" things for the persistDom,
> 
>>      other = virDomainSnapshotFindByName(vm->snapshots, def->name);
>>      if (other) {
>>          if ((other->def->state == VIR_DOMAIN_RUNNING ||
>> @@ -1349,6 +1365,11 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>>              }
>>          }
>>  
>> +        if (other->def->persistDom && !def->persistDom) {
>> +            def->persistDom = other->def->persistDom;
>> +            other->def->persistDom = NULL;
>> +        }
> 
> Isn't this just
> 
>     VIR_STEAL_PTR(def->persistDom, other->def->persistDom);
> 
> ?
> 
>> +
>>          if (def->dom) {
>>              if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
>>                  virDomainSnapshotDefIsExternal(def)) {
>> @@ -1363,6 +1384,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>>                      other->def->dom = def->dom;
>>                      def->dom = NULL;
>>                  }
>> +                if (def->persistDom && !other->def->persistDom) {
>> +                    other->def->persistDom = def->persistDom;
>> +                    def->persistDom = NULL;
>> +                }
> 
> Likewise...
> 
>>                  goto cleanup;

Yeah, a bit of copy-paste. In this case I would like to add patch to use VIR_STEAL_PTR
for existing code in virDomainSnapshotRedefinePrep too.

Nikolay

>>              }
>>          }
>>
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> John
> 

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.