[libvirt] [PATCH 2/9] conf: shmem: Add ABI stability check

[John Ferlan]

On 10/14/2014 03:29 AM, Peter Krempa wrote:
> Although the device will probably inhibit migration add checks to make
> sure that the configuration change gets caught.
> ---
>  src/conf/domain_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 55a1cc5..5e3c389 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14998,6 +14998,44 @@ virDomainPanicCheckABIStability(virDomainPanicDefPtr src,
>  }
> 
> 
> +static bool
> +virDomainShmemDefCheckABIStability(virDomainShmemDefPtr src,
> +                                   virDomainShmemDefPtr dst)
> +{
> +    if (STRNEQ_NULLABLE(src->name, dst->name)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("Target shared memory name '%s' does not match source "
> +                         "'%s'"), dst->name, src->name);
> +        return false;
> +    }
> +
> +    if (src->size != dst->size) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("Target shared memory size '%llu' does not match "
> +                         "source size '%llu'"), dst->size, src->size);
> +        return false;
> +    }

Again assuming the device even allows migration...

Could it be reasonable to allow the target to be larger?  I'm assuming
that "something" copying <stuff> from source to target during the
migration.  As long as the target is at least a source size, then does
it really matter if the target is larger?

I didn't dig into the migration code, but I'm assuming what happens is
the target gets whatever is currently in the shared memory device of the
source.

Obviously going larger to smaller would cause a problem, but if as a
guest admin I wanted to have a larger shared memory on the target and
knew I could modify my (now) former source memory segment later (or not
at all if I wasn't going to migrate back), then does it hurt?  Testing
won't like it, but I can see a use case.  Whether it's a reasonable
option based on the shmem code - I'm not sure.


> +
> +    if (src->server.enabled != dst->server.enabled) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Target shared memory server usage doesn't match "
> +                         "source"));
> +        return false;
> +    }
> +
> +    if (src->msi.vectors != dst->msi.vectors ||
> +        src->msi.enabled != dst->msi.enabled ||
> +        src->msi.ioeventfd != dst->msi.ioeventfd) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Target shared memory MSI configuration doesn't match "
> +                         "source"));
> +        return false;
> +    }

I think I've convinced myself it's not a good idea to switch between
server vs. non-server environment, but maybe someone has such a use case.

Just don't want to see us restrict something that someone may find as a
feature...  I haven't closely followed all the shmem discussions so I
could be off in the weeds...

ACK in general to what's here - unless of course my comments trigger
someone else to indicate we should allow these "differences".

John
> +
> +    return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info);
> +}
> +
> +
>  /* This compares two configurations and looks for any differences
>   * which will affect the guest ABI. This is primarily to allow
>   * validation of custom XML config passed in during migration
> @@ -15399,6 +15437,18 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
>      if (!virDomainPanicCheckABIStability(src->panic, dst->panic))
>          goto error;
> 
> +    if (src->nshmems != dst->nshmems) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("Target domain shared memory device count %zu "
> +                         "does not match source %zu"), dst->nshmems, src->nshmems);
> +        goto error;
> +    }
> +
> +    for (i = 0; i < src->nshmems; i++) {
> +        if (!virDomainShmemDefCheckABIStability(src->shmems[i], dst->shmems[i]))
> +            goto error;
> +    }
> +
>      return true;
> 
>   error:
>