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

2015-08-03 Thread lhuang 


On 07/30/2015 06:25 PM, Daniel P. Berrange wrote:

On Thu, Jul 23, 2015 at 06:13:49PM +0800, Luyao Huang wrote:

Signed-off-by: Luyao Huang lhu...@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(+)

+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) {
+ignore_value(virShmUnlink(shmem-name));

Why are you treating -1 differentl from -2 - in both cases we should
abort creation as that indicates the method either failed or is not
supported in this platform.


What i thought when i wrote this is : 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) but we could use shm_open, on 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.


I am not sure this is a good idea right now, since i am not sure this 
will work as except on different platform. Maybe i should remove it and 
make virShmBuildPath return -1 if not support on that platform.



Regards,
Daniel


Luyao

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-08-03 Thread 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 lhu...@redhat.com wrote:

Signed-off-by: Luyao Huang lhu...@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

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

2015-08-03 Thread lhuang 


On 07/30/2015 06:23 PM, Daniel P. Berrange wrote:

On Thu, Jul 23, 2015 at 06:13:49PM +0800, Luyao Huang wrote:

Signed-off-by: Luyao Huang lhu...@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_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) {
+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));
+goto cleanup;
+} else {
+othercreate = true;
+}
+type = VIR_SHM_TYPE_SERVER;
+}
+teardownshm = true;
+
+if (virSecurityManagerSetShmemLabel(driver-securityManager,
+vm-def, shmem, path)  0)
+goto cleanup;

You shouldn't be setting labelling at this point. That should be done
by the later call to virSecurityManagerSetAllLabel


Okay, i see, i will move it to virSecurityManagerSetAllLabel


+static int
+qemuCleanUpShmemDevice(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   virDomainShmemDefPtr shmem)
+{
+virShmObjectPtr tmp;
+virShmObjectListPtr list = driver-shmlist;
+int ret = -1;
+
+virObjectLock(list);
+
+if (!(tmp = virShmObjectFindByName(list, shmem-name))) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(Cannot find share memory named '%s'),
+   shmem-name);
+goto cleanup;
+}
+if ((shmem-server.enabled  tmp-type != VIR_SHM_TYPE_SERVER) ||
+(!shmem-server.enabled  tmp-type != VIR_SHM_TYPE_SHM)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(Shmem object and shmem device type is not equal));
+goto cleanup;
+}
+
+if (virShmRemoveUsedDomain(tmp, QEMU_DRIVER_NAME, vm-def-name)  0)
+goto cleanup;
+
+if (tmp-ndomains == 0) {
+if (virSecurityManagerRestoreShmemLabel(driver-securityManager,
+vm-def, shmem, tmp-path)  0)
+VIR_WARN(Unable to restore shared memory device labelling);

Likewise this should be left to the main label restore code


Okay, Thanks a lot for your review and advise.


+
+if (!shmem-server.enabled) {
+if (!tmp-othercreate 
+virShmUnlink(tmp-name)  0)
+VIR_WARN(Unable to unlink shared memory object);
+}
+
+if (virShmObjectRemoveStateFile(list, tmp-name)  0)
+goto cleanup;
+virShmObjectListDel(list, tmp);
+virShmObjectFree(tmp);
+}
+
+ret = 0;
+ cleanup:
+virObjectUnlock(list);
+return ret;
+}

Regards,
Daniel


Luyao

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-07-30 Thread Daniel P. Berrange 
On Thu, Jul 23, 2015 at 06:13:49PM +0800, Luyao Huang wrote:
 Signed-off-by: Luyao Huang lhu...@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_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) {
 +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));
 +goto cleanup;
 +} else {
 +othercreate = true;
 +}
 +type = VIR_SHM_TYPE_SERVER;
 +}
 +teardownshm = true;
 +
 +if (virSecurityManagerSetShmemLabel(driver-securityManager,
 +vm-def, shmem, path)  0)
 +goto cleanup;

You shouldn't be setting labelling at this point. That should be done
by the later call to virSecurityManagerSetAllLabel

 +static int
 +qemuCleanUpShmemDevice(virQEMUDriverPtr driver,
 +   virDomainObjPtr vm,
 +   virDomainShmemDefPtr shmem)
 +{
 +virShmObjectPtr tmp;
 +virShmObjectListPtr list = driver-shmlist;
 +int ret = -1;
 +
 +virObjectLock(list);
 +
 +if (!(tmp = virShmObjectFindByName(list, shmem-name))) {
 +virReportError(VIR_ERR_INTERNAL_ERROR,
 +   _(Cannot find share memory named '%s'),
 +   shmem-name);
 +goto cleanup;
 +}
 +if ((shmem-server.enabled  tmp-type != VIR_SHM_TYPE_SERVER) ||
 +(!shmem-server.enabled  tmp-type != VIR_SHM_TYPE_SHM)) {
 +virReportError(VIR_ERR_INTERNAL_ERROR, %s,
 +   _(Shmem object and shmem device type is not equal));
 +goto cleanup;
 +}
 +
 +if (virShmRemoveUsedDomain(tmp, QEMU_DRIVER_NAME, vm-def-name)  0)
 +goto cleanup;
 +
 +if (tmp-ndomains == 0) {
 +if (virSecurityManagerRestoreShmemLabel(driver-securityManager,
 +vm-def, shmem, tmp-path)  
 0)
 +VIR_WARN(Unable to restore shared memory device labelling);

Likewise this should be left to the main label restore code

 +
 +if (!shmem-server.enabled) {
 +if (!tmp-othercreate 
 +virShmUnlink(tmp-name)  0)
 +VIR_WARN(Unable to unlink shared memory object);
 +}
 +
 +if (virShmObjectRemoveStateFile(list, tmp-name)  0)
 +goto cleanup;
 +virShmObjectListDel(list, tmp);
 +virShmObjectFree(tmp);
 +}
 +
 +ret = 0;
 + cleanup:
 +virObjectUnlock(list);
 +return ret;
 +}

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-07-30 Thread Daniel P. Berrange 
On Thu, Jul 23, 2015 at 06:13:49PM +0800, Luyao Huang wrote:
 Signed-off-by: Luyao Huang lhu...@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(+)
 

 +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) {
 +ignore_value(virShmUnlink(shmem-name));

Why are you treating -1 differentl from -2 - in both cases we should
abort creation as that indicates the method either failed or is not
supported in this platform.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-07-27 Thread Marc-André Lureau 
Hi

On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang lhu...@redhat.com wrote:
 Signed-off-by: Luyao Huang lhu...@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 ?

 +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?

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

 +teardownlabel = true;
 +
 +if (!(tmp = virShmObjectNew(shmem-name, shmem-size, path, type, 
 othercreate,
 +QEMU_DRIVER_NAME, vm-def-name)))
 +goto cleanup;
 +
 +if (virShmObjectSaveState(tmp, list-stateDir)  0) {
 +virShmObjectFree(tmp);
 +goto cleanup;
 +}
 +
 +if (virShmObjectListAdd(list, tmp)  0) {
 +virShmObjectFree(tmp);
 +goto cleanup;
 +}
 +
 +ret = 0;
 +
 + cleanup:
 +if (ret  0) {
 +if (teardownlabel 
 +virSecurityManagerRestoreShmemLabel(driver-securityManager,
 +vm-def, shmem, path)  0)
 +VIR_WARN(Unable to restore shared memory device

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

2015-07-23 Thread Luyao Huang 
Signed-off-by: Luyao Huang lhu...@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) {
+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));
+goto cleanup;
+} else {
+othercreate = true;
+}
+type = VIR_SHM_TYPE_SERVER;
+}
+teardownshm = true;
+
+if (virSecurityManagerSetShmemLabel(driver-securityManager,
+vm-def, shmem, path)  0)
+goto cleanup;
+teardownlabel = true;
+
+if (!(tmp = virShmObjectNew(shmem-name, shmem-size, path, type, 
othercreate,
+QEMU_DRIVER_NAME, vm-def-name)))
+goto cleanup;
+
+if (virShmObjectSaveState(tmp, list-stateDir)  0) {
+virShmObjectFree(tmp);
+goto cleanup;
+}
+
+if (virShmObjectListAdd(list, tmp)  0) {
+virShmObjectFree(tmp);
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+if (ret  0) {
+if (teardownlabel 
+virSecurityManagerRestoreShmemLabel(driver-securityManager,
+vm-def, shmem, path)  0)
+VIR_WARN(Unable to restore shared memory device labelling);
+
+if (teardownshm  !shmem-server.enabled 
+!othercreate  virShmUnlink(shmem-name)  0)
+VIR_WARN(Unable to unlink shared memory object);
+}
+VIR_FREE(path);
+virObjectUnlock(list);
+return ret;
+}
+
+
+static int
+qemuCleanUpShmemDevice(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   virDomainShmemDefPtr shmem)
+{
+virShmObjectPtr tmp;
+virShmObjectListPtr list =