1:23 a.m.
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(a)redhat.com> wrote:
> Signed-off-by: Luyao Huang <lhuang(a)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