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

[Nikolay Shirokovskiy]

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 at 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 at redhat.com>
> 
> John
>