On 21.12.2018 00:01, John Ferlan wrote:
>
>
> On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
>> Renaming domain which has snapshots is prohibited. Also reverting
>> to ABI compatible active domain with a different name can have
>> issues later I guess. So let's prohibit changing domain name on snapshot
>> metadata redefine as well.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
>> ---
>> src/conf/snapshot_conf.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>
> This is separable patch and I'm definitely not the expert here. I think
> it should go before you make patch4 and beyond changes.
>
>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>> index 34fcf64..fa1cd6b 100644
>> --- a/src/conf/snapshot_conf.c
>> +++ b/src/conf/snapshot_conf.c
>> @@ -1307,6 +1307,14 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>> goto cleanup;
>> }
>>
>> + if (def->dom &&
>> + STRNEQ(def->dom->name, domain->name)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("definition for snapshot %s must use name %s"),
>> + def->name, domain->name);
>> + goto cleanup;
>> + }
>> +
>
> You could have also done the "if (def->dom) { ... } " type change...
> Not that it matters that much.
>
> Although it seems reasonable to me, hopefully Peter will pipe in as
> well. Reading the commit message makes me believe that this would not be
> possible, but then again the UUID *and* name are used as keys to look up
> the domain object so the names had better match. If they don't then we
> could be in a heap of trouble. Not sure how it would happen though.
Yeah it is a bit defensive check as redefine is supposed to be feed by
xmls we get on dumping existing snapshot metadata so we should have
consistent names and uuids. I don't think anyone sane is going to change
uuid or name in xml so may be this check is excessive. UUID check along
is fine as this way we won't use snapshot metadata from different domain
by mistake. On the other hand it is good to have more checks as xmls
are editable in the end and only limited set of editings are valid.
Nikolay
>
> You have my R-By, but I'd certainly feel better with even more eyes and
> thoughts on this.
>
> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
>
> John
>
>> other = virDomainSnapshotFindByName(vm->snapshots, def->name);
>> if (other) {
>> if ((other->def->state == VIR_DOMAIN_RUNNING ||
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
