[libvirt] [PATCH 4/4] qemu: call the helpers in virshm.c to manage shmem device

[lhuang]

Hi Marc-André

On 07/27/2015 11:52 PM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang <lhuang at redhat.com> wrote:
>> Signed-off-by: Luyao Huang <lhuang at redhat.com>
>> ---
>>   src/qemu/qemu_conf.h    |   3 +
>>   src/qemu/qemu_driver.c  |   4 ++
>>   src/qemu/qemu_process.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 165 insertions(+)
>>
>> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
>> index 3f73929..61d3462 100644
>> --- a/src/qemu/qemu_conf.h
>> +++ b/src/qemu/qemu_conf.h
>> @@ -46,6 +46,7 @@
>>   # include "virclosecallbacks.h"
>>   # include "virhostdev.h"
>>   # include "virfile.h"
>> +# include "virshm.h"
>>
>>   # ifdef CPU_SETSIZE /* Linux */
>>   #  define QEMUD_CPUMASK_LEN CPU_SETSIZE
>> @@ -235,6 +236,8 @@ struct _virQEMUDriver {
>>       /* Immutable pointer. Unsafe APIs. XXX */
>>       virHashTablePtr sharedDevices;
>>
>> +    virShmObjectListPtr shmlist;
>> +
>>       /* Immutable pointer, self-locking APIs */
>>       virPortAllocatorPtr remotePorts;
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 9dbe635..282ab45 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -778,6 +778,9 @@ qemuStateInitialize(bool privileged,
>>       if (qemuMigrationErrorInit(qemu_driver) < 0)
>>           goto error;
>>
>> +    if (!(qemu_driver->shmlist = virShmObjectListGetDefault()))
>> +        goto error;
>> +
>>       if (privileged) {
>>           char *channeldir;
>>
>> @@ -1087,6 +1090,7 @@ qemuStateCleanup(void)
>>       virObjectUnref(qemu_driver->config);
>>       virObjectUnref(qemu_driver->hostdevMgr);
>>       virHashFree(qemu_driver->sharedDevices);
>> +    virObjectUnref(qemu_driver->shmlist);
>>       virObjectUnref(qemu_driver->caps);
>>       virQEMUCapsCacheFree(qemu_driver->qemuCapsCache);
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 1c0c734..84b3b5e 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -4321,6 +4321,154 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
>>   }
>>
>>
>> +static int
>> +qemuPrepareShmemDevice(virQEMUDriverPtr driver,
>> +                       virDomainObjPtr vm,
>> +                       virDomainShmemDefPtr shmem)
>> +{
>> +    int ret = -1;
>> +    virShmObjectPtr tmp;
>> +    virShmObjectListPtr list = driver->shmlist;
>> +    bool othercreate = false;
>> +    char *path = NULL;
>> +    bool teardownlabel = false;
>> +    bool teardownshm = false;
>> +    int type, fd;
>> +
>> +    virObjectLock(list);
>> +
>> +    if ((tmp = virShmObjectFindByName(list, shmem->name))) {
>> +        if (shmem->size > tmp->size) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Shmem object %s is already exists and "
>> +                             "size is smaller than require size"),
>> +                           tmp->name);
>> +            goto cleanup;
>> +        }
>> +
>> +        if (virShmSetUsedDomain(tmp, QEMU_DRIVER_NAME, vm->def->name) < 0)
>> +            goto cleanup;
>> +
>> +        if (virShmObjectSaveState(tmp, list->stateDir) < 0)
>> +            goto cleanup;
>> +
>> +        virObjectUnlock(list);
>> +        return 0;
>> +    }
>> +
>> +    if (!shmem->server.enabled) {
>> +        if ((fd = virShmCreate(shmem->name, shmem->size, false, &othercreate, 0600)) < 0)
>> +            goto cleanup;
>> +        VIR_FORCE_CLOSE(fd);
>> +
>> +        if ((ret = virShmBuildPath(shmem->name, &path)) == -1) {
>> +            ignore_value(virShmUnlink(shmem->name));
>> +            goto cleanup;
>> +        } else if (ret == -2 && !othercreate) {
> What is ret == -2 ?

when ret = -2 this means we do not support virShmBuildPath in that 
platform (this could only happened on some other platform which support 
shm_open/shm_unlink ,just like solaris, freebsd), at that platform the 
shm obj is not in /dev/shm or not exist in the file system, if we help 
to create that shm obj but cannot find a way to relabel it, qemu will 
cannot use the shm in that case, so unlink the shm and let qemu create 
it will make guest can start success, and we could unlink the qemu 
create shm obj if there is no guest use it.

>
>> +            ignore_value(virShmUnlink(shmem->name));
>> +        }
>> +        type = VIR_SHM_TYPE_SHM;
>> +    } else {
>> +        if (!virFileExists(shmem->server.chr.data.nix.path)) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                           _("Shmem device server socket is not exist"));
> is not / does not
>
>> +            goto cleanup;
>> +        } else {
>> +            othercreate = true;
>> +        }
>> +        type = VIR_SHM_TYPE_SERVER;
>> +    }
>> +    teardownshm = true;
>> +
>> +    if (virSecurityManagerSetShmemLabel(driver->securityManager,
>> +                                        vm->def, shmem, path) < 0)
>> +        goto cleanup;
> So each time a ivshmem device starts, it will potentially change the
> labels. Not sure that's a good idea. Why not apply only when created?

Good question, we allow specify the label in the shmem device XML, and 
if the user create the shm with a label which only allow that guest 
access, then the other guest will cannot use it if we do not change the 
label, but if we change the label to a label which allow all libvirt 
guest can access, it will make the first guest not safe.(Also this will 
happen when different guest use one shared disk), i have an quick 
thought  to solve this issue:

Introduce the <shareable/> in the shmem device and forbid use one shm 
object in different shared policy (a example: two guest use one shm obj 
and one use it with a shared label and another not), and write some 
document to point out this.

> Btw, have you considered managing shm like storage pools? Would that
> bring any benefit?
>

I thought about this before, and in my opinion introduce a lot of public 
API (just like:list, delete, create...) is unnecessary: there are two 
usage of shmem device: use it for a guest-host connect or for 
guest-guest connect. if one guest is using the shmem resource 
(ivshmem-server/shm obj), we should make sure we won't remove the 
resource. and after no guest use it we will clean up the resource. Also 
we provide a way to keep the resource here (we still will relabel the 
resource) after all guest is not use it, and users could clean the 
resource when they want. As the requirement for set up and clean up 
shmem device resource is very clear, we could help users to do this(the 
users can just use it without prepare extra code to find a way to know 
when need set up/clean up the resource).

Thanks a lot for your review and help.

Luyao