Re: [libvirt] [PATCH 7/9] conf: snapshot: check domain name on redefine

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(a)virtuozzo.com&gt; > --- > 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.

You have my R-By, but I'd certainly feel better with even more eyes and thoughts on this. Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John > other = virDomainSnapshotFindByName(vm->snapshots, def->name); > if (other) { > if ((other->def->state == VIR_DOMAIN_RUNNING || >