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