[libvirt] [PATCH 05/20] conf: Add support for shmem role

[Martin Kletzander]

On Fri, Sep 16, 2016 at 09:20:16AM +0200, Peter Krempa wrote:
>On Thu, Sep 15, 2016 at 18:14:30 +0200, Martin Kletzander wrote:
>> Role controls how the domain behaves on migration.
>>
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>>  docs/formatdomain.html.in                         | 10 +++++-
>>  docs/schemas/domaincommon.rng                     |  8 +++++
>>  src/conf/domain_conf.c                            | 39 ++++++++++++++++++++++-
>>  src/conf/domain_conf.h                            | 11 +++++++
>>  tests/qemuxml2argvdata/qemuxml2argv-shmem.xml     |  4 +--
>>  tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml |  4 +--
>>  6 files changed, 70 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index f48a4d8b813f..f4d08959c787 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>
>[...]
>
>> @@ -6764,6 +6764,14 @@ qemu-kvm -net nic,model=? /dev/null
>>      <dd>
>>        The <code>shmem</code> element has one mandatory attribute,
>>        <code>name</code> to identify the shared memory.
>> +      Optional attribute <code>role</code> can be set to values
>> +      "master" or "peer".  The former will mean that upon migration,
>> +      the data in the shared memory is migrated with the domain.
>> +      There should be only one "master" per shared memory object.
>> +      Migration with "peer" role is disabled.  If migration of such
>> +      domain is required, the shmem device needs to be unplugged
>> +      before migration and plugged in at the destination upon
>> +      successful migration.
>
>I'm not in favor of adding the workaround suggestion. I'd change it for
>a more thoroguh explanation what this configuration does besides
>controlling migratability.
>

Well, "technically", master controls the shm, where peer is just using
it.  When migrating, master will transfer that data in the stream.  But
as I understand it, peer would do the same due to the way how QEMU works
and it memory is mapped.  And since you cannot turn off copying that
part of memory, qemu added a boolean 'master' that controls whether you
can or cannot migrate.  So it doesn't do anything else from a technical
point of view, but it makes much more sense to have that separation from
user's point of view as it feel closer how people think (I don't really
know how to express what I mean any better, sorry).

>>      </dd>
>>      <dt><code>size</code></dt>
>>      <dd>
>
>[...]
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 97cb3de95529..2ccc10515f30 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>
>[...]
>
>> @@ -12201,6 +12206,23 @@ virDomainShmemDefParseXML(xmlNodePtr node,
>>
>>      ctxt->node = node;
>>
>> +    if (!(def->name = virXMLPropString(node, "name"))) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("shmem element must contain 'name' attribute"));
>> +        goto cleanup;
>> +    }
>
>This code is already in the function a few lines after this hunk. You
>probably forgot to delete it.
>

My bad, probably copy-paste or some other weird error.

>> +
>> +    tmp = virXMLPropString(node, "role");
>> +    if (tmp) {
>> +        if ((def->role = virDomainShmemRoleTypeFromString(tmp)) <= 0) {
>> +            virReportError(VIR_ERR_XML_ERROR,
>> +                           _("Unknown shmem role type '%s'"), tmp);
>> +            goto cleanup;
>> +        }
>> +
>> +        VIR_FREE(tmp);
>> +    }
>> +
>>      tmp = virXPathString("string(./model/@type)", ctxt);
>>      if (tmp) {
>>          /* If there's none, we will automatically have the first one
>> @@ -18704,6 +18726,15 @@ virDomainShmemDefCheckABIStability(virDomainShmemDefPtr src,
>>          return false;
>>      }
>>
>> +    if (src->role != dst->role) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                       _("Target shared memory role '%s' does not match "
>> +                         "source role '%s'"),
>> +                       virDomainShmemRoleTypeToString(dst->role),
>> +                       virDomainShmemRoleTypeToString(src->role));
>> +        return false;
>> +    }
>
>Is this really guest ABI? Since it's not used in this patch I'll see
>later.
>

Well, it is not visible from the guest.  Looking at it now (it's some
time since I wrote this part) I think whatever is here was meant to be
part of migration compatibility checks instead.  I'll fix that up in v2,
thanks for spotting that.

>> +
>>      if (src->model != dst->model) {
>>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>                         _("Target shared memory model '%s' does not match "
>> @@ -21790,8 +21821,14 @@ virDomainShmemDefFormat(virBufferPtr buf,
>>                          virDomainShmemDefPtr def,
>>                          unsigned int flags)
>>  {
>> -    virBufferEscapeString(buf, "<shmem name='%s'>\n", def->name);
>> +    virBufferEscapeString(buf, "<shmem name='%s'", def->name);
>>
>> +    if (def->role) {
>> +        virBufferEscapeString(buf, " role='%s'",
>> +                              virDomainShmemRoleTypeToString(def->role));
>> +    }
>> +
>> +    virBufferAddLit(buf, ">\n");
>>      virBufferAdjustIndent(buf, 2);
>>
>>      virBufferAsprintf(buf, "<model type='%s'/>\n",
>
>Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160916/3e93b093/attachment-0001.sig>