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>
---
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>
John
other = virDomainSnapshotFindByName(vm->snapshots,
def->name);
if (other) {
if ((other->def->state == VIR_DOMAIN_RUNNING ||