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

[John Ferlan]

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

John

>      other = virDomainSnapshotFindByName(vm->snapshots, def->name);
>      if (other) {
>          if ((other->def->state == VIR_DOMAIN_RUNNING ||
>