Hi On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang <lhuang@redhat.com> wrote:

Introduce a new element in shmem device element, this could help users to change the shm label to a specified label.

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- docs/formatdomain.html.in | 7 ++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 55 ++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 5 ++++ 4 files changed, 59 insertions(+), 11 deletions(-)

It would be better with a small test, checking parsing and format.

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d0c1741..e02c67c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6098,6 +6098,13 @@ qemu-kvm -net nic,model=? /dev/null vectors. The <code>ioeventd</code> attribute enables/disables (values "on"/"off", respectively) ioeventfd. </dd> + <dt><code>seclabel</code></dt> + <dd> + The optional <code>seclabel</code> to override the way that labelling

The "element may contain an" optional <code>...

+ is done on the shm object path or shm server path. If this + element is not present, the <a href="#seclabel">security label is inherited + from the per-domain setting</a>. + </dd> </dl>

<h4><a name="elementsMemory">Memory devices</a></h4> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 1120003..f58e8de 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3323,6 +3323,9 @@ </optional> </element> </optional> + <zeroOrMore> + <ref name='devSeclabel'/> + </zeroOrMore> <optional> <ref name="address"/> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 73ac537..cb3d72a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11261,6 +11261,8 @@ virDomainNVRAMDefParseXML(xmlNodePtr node, static virDomainShmemDefPtr virDomainShmemDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, + virSecurityLabelDefPtr* vmSeclabels, + int nvmSeclabels, unsigned int flags) { char *tmp = NULL; @@ -11332,6 +11334,10 @@ virDomainShmemDefParseXML(xmlNodePtr node, if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto cleanup;

+ if (virSecurityDeviceLabelDefParseXML(&def->seclabels, &def->nseclabels, + vmSeclabels, nvmSeclabels, + ctxt, flags) < 0) + goto cleanup;

ret = def; def = NULL; @@ -12457,7 +12463,11 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_SHMEM: - if (!(dev->data.shmem = virDomainShmemDefParseXML(node, ctxt, flags))) + if (!(dev->data.shmem = virDomainShmemDefParseXML(node, + ctxt, + def->seclabels, + def->nseclabels, + flags))) goto error; break; case VIR_DOMAIN_DEVICE_TPM: @@ -16136,7 +16146,8 @@ virDomainDefParseXML(xmlDocPtr xml, for (i = 0; i < n; i++) { virDomainShmemDefPtr shmem; ctxt->node = nodes[i]; - shmem = virDomainShmemDefParseXML(nodes[i], ctxt, flags); + shmem = virDomainShmemDefParseXML(nodes[i], ctxt, def->seclabels, + def->nseclabels, flags); if (!shmem) goto error;

@@ -20308,6 +20319,8 @@ virDomainShmemDefFormat(virBufferPtr buf, virDomainShmemDefPtr def, unsigned int flags) { + size_t n; + virBufferEscapeString(buf, "<shmem name='%s'", def->name);

if (!def->size && @@ -20341,6 +20354,9 @@ virDomainShmemDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); }

+ for (n = 0; n < def->nseclabels; n++) + virSecurityDeviceLabelDefFormat(buf, def->seclabels[n], flags); + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1;

@@ -23851,11 +23867,25 @@ virDomainObjListExport(virDomainObjListPtr domlist, }

+static virSecurityDeviceLabelDefPtr +virDomainGetDeviceSecurityLabelDef(virSecurityDeviceLabelDefPtr *seclabels, + size_t nseclabels, + const char *model) +{ + size_t i; + + for (i = 0; i < nseclabels; i++) { + if (STREQ_NULLABLE(seclabels[i]->model, model)) + return seclabels[i]; + } + return NULL; +} + + virSecurityLabelDefPtr virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model) { size_t i; - virSecurityLabelDefPtr seclabel = NULL;

if (def == NULL || model == NULL) return NULL; @@ -23866,24 +23896,27 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model) if (STREQ(def->seclabels[i]->model, model)) return def->seclabels[i]; } - - return seclabel; + return NULL;

This looks like a seperate cleanup.

}

virSecurityDeviceLabelDefPtr virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model) { - size_t i; + if (def == NULL) + return NULL; + + return virDomainGetDeviceSecurityLabelDef(def->seclabels, def->nseclabels, model); +}

+ +virSecurityDeviceLabelDefPtr +virDomainShmemDefGetSecurityLabelDef(virDomainShmemDefPtr def, const char *model) +{ if (def == NULL) return NULL;

- for (i = 0; i < def->nseclabels; i++) { - if (STREQ_NULLABLE(def->seclabels[i]->model, model)) - return def->seclabels[i]; - } - return NULL; + return virDomainGetDeviceSecurityLabelDef(def->seclabels, def->nseclabels, model); }

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0fe6b1a..1a0475e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1608,6 +1608,8 @@ struct _virDomainShmemDef { unsigned vectors; virTristateSwitch ioeventfd; } msi; + size_t nseclabels; + virSecurityDeviceLabelDefPtr *seclabels; virDomainDeviceInfo info; };

@@ -2943,6 +2945,9 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model); virSecurityDeviceLabelDefPtr virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model);

+virSecurityDeviceLabelDefPtr +virDomainShmemDefGetSecurityLabelDef(virDomainShmemDefPtr def, const char *model); + typedef const char* (*virEventActionToStringFunc)(int type); typedef int (*virEventActionFromStringFunc)(const char *type);

-- 1.8.3.1

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

-- Marc-André Lureau

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

Introduce a new element in shmem device element, this could help users to change the shm label to a specified label.

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- docs/formatdomain.html.in | 7 ++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 55 ++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 5 ++++ 4 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d0c1741..e02c67c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6098,6 +6098,13 @@ qemu-kvm -net nic,model=? /dev/null vectors. The <code>ioeventd</code> attribute enables/disables (values "on"/"off", respectively) ioeventfd. </dd> + <dt><code>seclabel</code></dt> + <dd> + The optional <code>seclabel</code> to override the way that labelling + is done on the shm object path or shm server path. If this + element is not present, the <a href="#seclabel">security label is inherited + from the per-domain setting</a>. + </dd> </dl>

<h4><a name="elementsMemory">Memory devices</a></h4> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 1120003..f58e8de 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3323,6 +3323,9 @@ </optional> </element> </optional> + <zeroOrMore> + <ref name='devSeclabel'/> + </zeroOrMore> <optional> <ref name="address"/> </optional>

So in the <disk> XML we have an explicit element to indicate whether the device is intended to be shared across multiple guests. <shareable/> I think we need to have the same flag added to the shm device too, so that we sanity check whether a particular shm was intended to be shared or whether it is a mistake when multiple guests use it. This will also allow us to integrate with the virtlockd to acquire exclusive locks against the shm device to actively prevent admin mistakes starting 2 guests with the same shm. It will also let us automatically choose the right default SELinux label ie svirt_image_t:s0:c214,c3242 for exclusive access vs svirt_image_t:s0 for shared access 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 :|

Hi Marc-André On 07/27/2015 11:39 PM, Marc-André Lureau wrote:

Hi

On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang<lhuang@redhat.com> wrote:

...

A new api to help set/restore the shmem deivce dac/selinux label. typo: deivce / device.

thanks, i will fix this in next version

...

Signed-off-by: Luyao Huang<lhuang@redhat.com> --- src/libvirt_private.syms | 2 ++ src/security/security_dac.c | 67 +++++++++++++++++++++++++++++++++++++++ src/security/security_driver.h | 11 +++++++ src/security/security_manager.c | 38 ++++++++++++++++++++++ src/security/security_manager.h | 8 +++++ src/security/security_selinux.c | 70 +++++++++++++++++++++++++++++++++++++++++ src/security/security_stack.c | 41 ++++++++++++++++++++++++ 7 files changed, 237 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 588b1c4..af73177 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1038,6 +1038,7 @@ virSecurityManagerRestoreDiskLabel; virSecurityManagerRestoreHostdevLabel; virSecurityManagerRestoreImageLabel; virSecurityManagerRestoreSavedStateLabel; +virSecurityManagerRestoreShmemLabel; virSecurityManagerSetAllLabel; virSecurityManagerSetChildProcessLabel; virSecurityManagerSetDaemonSocketLabel; @@ -1048,6 +1049,7 @@ virSecurityManagerSetImageFDLabel; virSecurityManagerSetImageLabel; virSecurityManagerSetProcessLabel; virSecurityManagerSetSavedStateLabel; +virSecurityManagerSetShmemLabel; virSecurityManagerSetSocketLabel; virSecurityManagerSetTapFDLabel; virSecurityManagerStackAddNested; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index deb6980..f954aa5 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -39,6 +39,7 @@ #include "virstoragefile.h" #include "virstring.h" #include "virutil.h" +#include "virshm.h" This header doesn't exist (yet)

I forgot remove this, thanks for pointing out. (and i won't remove this, since i need a function in virshm.h, so i will move it this patch after patch 3/4 in next version)

...

#define VIR_FROM_THIS VIR_FROM_SECURITY

@@ -922,6 +923,69 @@ virSecurityDACRestoreSecurityTPMFileLabel(virSecurityManagerPtr mgr,

static int +virSecurityDACSetShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainShmemDefPtr shmem, + char *path) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr shmem_seclabel = NULL; + char *tmppath; could make it const

Okay

...

+ uid_t user; + gid_t group; + + if (shmem->server.enabled) + tmppath = shmem->server.chr.data.nix.path; + else + tmppath = path; + + if (!tmppath) + return 0; + + shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, SECURITY_DAC_NAME); + + if (shmem_seclabel && !shmem_seclabel->relabel) + return 0; + + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + The function is similar to virSecurityDACSetSecurityImageLabel and yet subtly different: there is a early dynamicOwnership condition that seems to be general, the domain seclabel->relabel is checked first. It would be nice to align the behaviour.

Indeed, i will move the shmem_seclabel and seclabel check more early.

...

+ if (shmem_seclabel && shmem_seclabel->label) { + if (virParseOwnershipIds(shmem_seclabel->label, &user, &group) < 0) + return -1; + } else { + if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) + return -1; + } + + return virSecurityDACSetOwnership(tmppath, user, group); +} + + +static int +virSecurityDACRestoreShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainShmemDefPtr shmem, + char *path) +{ + virSecurityDeviceLabelDefPtr shmem_seclabel = NULL; + + shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, SECURITY_DAC_NAME); + + if (shmem_seclabel && !shmem_seclabel->relabel) + return 0; + + if (shmem->server.enabled) + return virSecurityDACRestoreChardevLabel(mgr, def, NULL, &shmem->server.chr); + + if (!path) + return 0; + + return virSecurityDACRestoreSecurityFileLabel(path); +} + + +static int virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, bool migrated) @@ -1433,4 +1497,7 @@ virSecurityDriver virSecurityDriverDAC = { .domainGetSecurityMountOptions = virSecurityDACGetMountOptions,

.getBaseLabel = virSecurityDACGetBaseLabel, + + .domainSetSecurityShmemLabel = virSecurityDACSetShmemLabel, + .domainRestoreSecurityShmemLabel = virSecurityDACRestoreShmemLabel, }; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index f0dca09..37e4527 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -118,6 +118,14 @@ typedef int (*virSecurityDomainSetImageLabel) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virStorageSourcePtr src); +typedef int (*virSecurityDomainSetShmemLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainShmemDefPtr shmem, + char *path); +typedef int (*virSecurityDomainRestoreShmemLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainShmemDefPtr shmem, + char *path);

struct _virSecurityDriver { @@ -168,6 +176,9 @@ struct _virSecurityDriver { virSecurityDomainSetHugepages domainSetSecurityHugepages;

virSecurityDriverGetBaseLabel getBaseLabel; + + virSecurityDomainSetShmemLabel domainSetSecurityShmemLabel; + virSecurityDomainRestoreShmemLabel domainRestoreSecurityShmemLabel; };

virSecurityDriverPtr virSecurityDriverLookup(const char *name, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index b0cd9e8..72ca7e2 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -991,3 +991,41 @@ virSecurityManagerSetHugepages(virSecurityManagerPtr mgr,

return 0; } + + +int +virSecurityManagerRestoreShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainShmemDefPtr shmem, + char *path) +{ + if (mgr->drv->domainRestoreSecurityShmemLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainRestoreSecurityShmemLabel(mgr, vm, shmem, path); + virObjectUnlock(mgr); + return ret; + } + + virReportUnsupportedError(); + return -1; +} + + +int +virSecurityManagerSetShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainShmemDefPtr shmem, + char *path) +{ + if (mgr->drv->domainSetSecurityShmemLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainSetSecurityShmemLabel(mgr, vm, shmem, path); + virObjectUnlock(mgr); + return ret; + } + + virReportUnsupportedError(); + return -1; +} diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 13468db..ce37c91 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -149,5 +149,13 @@ int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, virStorageSourcePtr src); +int virSecurityManagerRestoreShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainShmemDefPtr shmem, + char *path); +int virSecurityManagerSetShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainShmemDefPtr shmem, + char *path); const path

Okay

...

#endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6e67a86..cbf89ee 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -46,6 +46,7 @@ #include "virconf.h" #include "virtpm.h" #include "virstring.h" +#include "virshm.h" remove that too

...

#define VIR_FROM_THIS VIR_FROM_SECURITY

@@ -1888,6 +1889,37 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def, }

+static int +virSecuritySELinuxRestoreShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainShmemDefPtr shmem, + char *path) const path

...

+{ + char *tmppath = NULL; make it const too

...

+ virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr shmem_seclabel = NULL; + + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (!seclabel || !seclabel->relabel) + return 0; + + shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, SECURITY_SELINUX_NAME); + + if (shmem_seclabel && !shmem_seclabel->relabel) + return 0; + + if (shmem->server.enabled) + tmppath = shmem->server.chr.data.nix.path; + else + tmppath = path; + + if (!tmppath) + return 0; + + return virSecuritySELinuxRestoreSecurityFileLabel(mgr, tmppath); +} + + static const char * virSecuritySELinuxGetBaseLabel(virSecurityManagerPtr mgr, int virtType) { @@ -2284,6 +2316,41 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def,

static int +virSecuritySELinuxSetShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainShmemDefPtr shmem, + char *path) +{ + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + char *tmppath = NULL; + virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr shmem_seclabel = NULL; + + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (!seclabel || !seclabel->relabel) + return 0; + + shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, SECURITY_SELINUX_NAME); + + if (shmem_seclabel && !shmem_seclabel->relabel) + return 0; + + if (shmem->server.enabled) + tmppath = shmem->server.chr.data.nix.path; + else + tmppath = path; I am not sure it's a good idea to either set the server socket policy or the shm. Why not set both?

Hmm...these $path are the shm path, if the shmem device is server enabled, the used shm is created by ivshmem-server, which will depends on ivshmem-server's label. And qemu process will connect to the socket created by ivshmem-server, so change the label for shm created by ivshmem-sever is useless (also we don't know the shm name created by ivshmem-server). However, never mind, i will do a refactor and remove the variable $path here to make this function easy to put in virSecuritySELinuxSetSecurityAllLabel and virSecuritySELinuxSetSecurityAllLabel.

...

+ if (!tmppath) + return 0; + + if (shmem_seclabel && shmem_seclabel->label) + return virSecuritySELinuxSetFilecon(tmppath, shmem_seclabel->label); + else + return virSecuritySELinuxSetFilecon(tmppath, data->file_context); +} + + +static int virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *stdin_path) @@ -2549,4 +2616,7 @@ virSecurityDriver virSecurityDriverSELinux = {

.domainGetSecurityMountOptions = virSecuritySELinuxGetSecurityMountOptions, .getBaseLabel = virSecuritySELinuxGetBaseLabel, + + .domainSetSecurityShmemLabel = virSecuritySELinuxSetShmemLabel, + .domainRestoreSecurityShmemLabel = virSecuritySELinuxRestoreShmemLabel, }; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 1ded57b..22c1b56 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -599,6 +599,44 @@ virSecurityStackRestoreSecurityImageLabel(virSecurityManagerPtr mgr, return rc; }

+static int +virSecurityStackSetShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainShmemDefPtr shmem, + char *path) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerSetShmemLabel(item->securityManager, + vm, shmem, path) < 0) + rc = -1; + } + + return rc; +} + +static int +virSecurityStackRestoreShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainShmemDefPtr shmem, + char *path) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerRestoreShmemLabel(item->securityManager, + vm, shmem, path) < 0) + rc = -1; + } + + return rc; +} + virSecurityDriver virSecurityDriverStack = { .privateDataLen = sizeof(virSecurityStackData), .name = "stack", @@ -648,4 +686,7 @@ virSecurityDriver virSecurityDriverStack = { .domainSetSecurityHugepages = virSecurityStackSetHugepages,

.getBaseLabel = virSecurityStackGetBaseLabel, + + .domainSetSecurityShmemLabel = virSecurityStackSetShmemLabel, + .domainRestoreSecurityShmemLabel = virSecurityStackRestoreShmemLabel, }; -- 1.8.3.1 Shouldn't it be implemented for the nop virSecurityDriver too? (note: I don't know what it is for)

Good catch, i didn't notice that, i will add it in next version. Luyao

...

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

On 07/30/2015 05:54 PM, Daniel P. Berrange wrote:

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

...

A new api to help set/restore the shmem deivce dac/selinux label. s/deivce/device/

Thanks, i will fix in next version

...

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 2 ++ src/security/security_dac.c | 67 +++++++++++++++++++++++++++++++++++++++ src/security/security_driver.h | 11 +++++++ src/security/security_manager.c | 38 ++++++++++++++++++++++ src/security/security_manager.h | 8 +++++ src/security/security_selinux.c | 70 +++++++++++++++++++++++++++++++++++++++++ src/security/security_stack.c | 41 ++++++++++++++++++++++++ 7 files changed, 237 insertions(+) Also need to add to the security_nop.c impl

Okay, i must missed this file at that time :)

...

diff --git a/src/security/security_dac.c b/src/security/security_dac.c index deb6980..f954aa5 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -39,6 +39,7 @@ #include "virstoragefile.h" #include "virstring.h" #include "virutil.h" +#include "virshm.h"

#define VIR_FROM_THIS VIR_FROM_SECURITY

@@ -922,6 +923,69 @@ virSecurityDACRestoreSecurityTPMFileLabel(virSecurityManagerPtr mgr,

static int +virSecurityDACSetShmemLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainShmemDefPtr shmem, + char *path) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr shmem_seclabel = NULL; + char *tmppath; + uid_t user; + gid_t group; + + if (shmem->server.enabled) + tmppath = shmem->server.chr.data.nix.path; + else + tmppath = path; Even when the server is enabled, QEMU still needs access to the path doesn't it. ... + if (shmem->server.enabled) + return virSecurityDACRestoreChardevLabel(mgr, def, NULL, &shmem->server.chr); We need to restore path, even when server is enabled ... Same comment as earlier

...

+ + if (shmem->server.enabled) + tmppath = shmem->server.chr.data.nix.path; + else + tmppath = path; And again

Hmm...these $path are the shm path, if the shmem device is server enabled, the used shm is created by ivshmem-server, which will depends on ivshmem-server's label. And qemu process will connect to the socket created by ivshmem-server, so change the label for shm created by ivshmem-sever is useless (also we don't know the shm name created by ivshmem-server). However, never mind, i will do a refactor and remove the variable $path here to make this function easy to put in virSecuritySELinuxSetSecurityAllLabel and virSecuritySELinuxSetSecurityAllLabel, since i noticed another reply from you for this patch.

...

+ + if (!tmppath) + return 0; + + if (shmem_seclabel && shmem_seclabel->label) + return virSecuritySELinuxSetFilecon(tmppath, shmem_seclabel->label); + else + return virSecuritySELinuxSetFilecon(tmppath, data->file_context); +} + + +static int virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *stdin_path) Regards, Daniel

Thanks a lot for your review and help. Luyao

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

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

...

A new api to help set/restore the shmem deivce dac/selinux label.

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 2 ++ src/security/security_dac.c | 67 +++++++++++++++++++++++++++++++++++++++ src/security/security_driver.h | 11 +++++++ src/security/security_manager.c | 38 ++++++++++++++++++++++ src/security/security_manager.h | 8 +++++ src/security/security_selinux.c | 70 +++++++++++++++++++++++++++++++++++++++++ src/security/security_stack.c | 41 ++++++++++++++++++++++++ 7 files changed, 237 insertions(+)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c index deb6980..f954aa5 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1433,4 +1497,7 @@ virSecurityDriver virSecurityDriverDAC = { .domainGetSecurityMountOptions = virSecurityDACGetMountOptions,

.getBaseLabel = virSecurityDACGetBaseLabel, + + .domainSetSecurityShmemLabel = virSecurityDACSetShmemLabel, + .domainRestoreSecurityShmemLabel = virSecurityDACRestoreShmemLabel, NB, you should also be modifying the virSecurityDACRestoreSecurityAllLabel and virSecurityDACSetSecurityAllLabel methods to call this code during

...

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6e67a86..cbf89ee 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2549,4 +2616,7 @@ virSecurityDriver virSecurityDriverSELinux = {

.domainGetSecurityMountOptions = virSecuritySELinuxGetSecurityMountOptions, .getBaseLabel = virSecuritySELinuxGetBaseLabel, + + .domainSetSecurityShmemLabel = virSecuritySELinuxSetShmemLabel, + .domainRestoreSecurityShmemLabel = virSecuritySELinuxRestoreShmemLabel, }; Likewise virSecuritySELinuxRestoreSecurityAllLabel and virSecuritySELinuxSetSecurityAllLabel

Doing this avoids the need to manually call these shmem specific security methods during general guest startup/shutdown. They only need to be called manually during hotplug/unplug.

Okay, i see, i move these function in virSecuritySELinuxRestoreSecurityAllLabel and virSecuritySELinuxSetSecurityAllLabel. Thanks a lot for your review and advise.

Regards, Daniel

Luyao

Hi On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang <lhuang@redhat.com> wrote:

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- configure.ac | 10 + po/POTFILES.in | 3 +- src/Makefile.am | 5 +- src/libvirt_private.syms | 16 ++ src/util/virshm.c | 623 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virshm.h | 104 ++++++++ 6 files changed, 759 insertions(+), 2 deletions(-) create mode 100644 src/util/virshm.c create mode 100644 src/util/virshm.h

(would be nicer to have some tests) After applying this patch, make check fails with Symbol block at ./libvirt_private.syms:2081: symbols not sorted virShmObjectFree virShmObjectNew virShmObjectListAdd virShmObjectListDel virShmObjectFindByName virShmObjectListGetDefault Correct ordering virShmObjectFindByName virShmObjectFree virShmObjectListAdd virShmObjectListDel virShmObjectListGetDefault virShmObjectNew Makefile:10195: recipe for target 'check-symsorting' failed

diff --git a/configure.ac b/configure.ac index a7f38e8..940eb66 100644 --- a/configure.ac +++ b/configure.ac @@ -1176,6 +1176,16 @@ if test "$with_linux" = "yes"; then ]]) fi

+dnl +dnl check for POSIX share memory functions +dnl +LIBRT_LIBS="" +AC_CHECK_LIB([rt],[shm_open],[LIBRT_LIBS="-lrt"]) +old_libs="$LIBS" +LIBS="$old_libs $LIBRT_LIBS" +AC_CHECK_FUNCS([shm_open]) +LIBS="$old_libs" +AC_SUBST([LIBRT_LIBS])

dnl Need to test if pkg-config exists PKG_PROG_PKG_CONFIG diff --git a/po/POTFILES.in b/po/POTFILES.in index a75f5ae..7687a82 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -215,8 +215,9 @@ src/util/virpolkit.c src/util/virportallocator.c src/util/virprocess.c src/util/virrandom.c -src/util/virsexpr.c src/util/virscsi.c +src/util/virsexpr.c +src/util/virshm.c src/util/virsocketaddr.c src/util/virstats.c src/util/virstorageencryption.c diff --git a/src/Makefile.am b/src/Makefile.am index 7338ab9..048a096 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -152,6 +152,7 @@ UTIL_SOURCES = \ util/virscsi.c util/virscsi.h \ util/virseclabel.c util/virseclabel.h \ util/virsexpr.c util/virsexpr.h \ + util/virshm.c util/virshm.h \ util/virsocketaddr.h util/virsocketaddr.c \ util/virstats.c util/virstats.h \ util/virstorageencryption.c util/virstorageencryption.h \ @@ -1048,7 +1049,7 @@ libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \ $(THREAD_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ $(LIB_CLOCK_GETTIME) $(DBUS_LIBS) $(MSCOM_LIBS) $(LIBXML_LIBS) \ $(SECDRIVER_LIBS) $(NUMACTL_LIBS) $(SYSTEMD_DAEMON_LIBS) \ - $(POLKIT_LIBS) + $(POLKIT_LIBS) $(LIBRT_LIBS)

noinst_LTLIBRARIES += libvirt_conf.la @@ -1284,6 +1285,7 @@ libvirt_driver_qemu_impl_la_LIBADD = $(CAPNG_LIBS) \ $(GNUTLS_LIBS) \ $(LIBNL_LIBS) \ $(LIBXML_LIBS) \ + $(LIBRT_LIBS) \ $(NULL) libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)

@@ -2264,6 +2266,7 @@ libvirt_setuid_rpc_client_la_LDFLAGS = \ $(AM_LDFLAGS) \ $(LIBXML_LIBS) \ $(SECDRIVER_LIBS) \ + $(LIBRT_LIBS) \ $(NULL) libvirt_setuid_rpc_client_la_CFLAGS = \ -DLIBVIRT_SETUID_RPC_CLIENT \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index af73177..977fd34 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2078,6 +2078,22 @@ sexpr_u64; string2sexpr;

+# util/virshm.h +virShmBuildPath; +virShmCreate; +virShmObjectFree; +virShmObjectNew; +virShmObjectListAdd; +virShmObjectListDel; +virShmObjectFindByName; +virShmObjectListGetDefault; +virShmObjectRemoveStateFile; +virShmObjectSaveState; +virShmOpen; +virShmRemoveUsedDomain; +virShmSetUsedDomain; +virShmUnlink;

So these lines should be sorted

+ # util/virsocketaddr.h virSocketAddrBroadcast; virSocketAddrBroadcastByPrefix; diff --git a/src/util/virshm.c b/src/util/virshm.c new file mode 100644 index 0000000..7ab39be --- /dev/null +++ b/src/util/virshm.c @@ -0,0 +1,623 @@ +/* + * virshm.c: helper API for POSIX share memory + * + * Copyright (C) 2015 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include <fcntl.h> +#include <sys/stat.h> +#include <sys/types.h> + +#ifdef HAVE_SHM_OPEN +# include <sys/mman.h> +#endif + +#include "virshm.h" +#include "virxml.h" +#include "virbuffer.h" +#include "virerror.h" +#include "virstring.h" +#include "virlog.h" +#include "virutil.h" +#include "viralloc.h" +#include "virfile.h" +#include "configmake.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.shm"); + +#define SHMEM_STATE_DIR LOCALSTATEDIR "/run/libvirt/shmem" + +VIR_ENUM_IMPL(virShmObject, VIR_SHM_TYPE_LAST, + "shm", + "server"); + +static virClassPtr virShmObjectListClass; + +static virShmObjectListPtr mainlist; /* global shm object list */ + +static void virShmObjectListDispose(void *obj); + +static int +virShmObjectListLoadState(virShmObjectPtr *shmobj, + const char *stateDir, + const char *name) +{ + char *stateFile = NULL; + xmlXPathContextPtr ctxt = NULL; + xmlDocPtr xml = NULL; + virShmObjectPtr tmpshm; + xmlNodePtr *usagenode = NULL; + xmlNodePtr save = NULL;

save could be moved in the using scope

+ int ret = -1; + char *drivername = NULL; + char *shmtype = NULL; + int nusages; + + if (VIR_ALLOC(tmpshm) < 0) + return -1; + + if (!(stateFile = virFileBuildPath(stateDir, name, ".xml"))) + goto error; + + if (!(xml = virXMLParseFileCtxt(stateFile, &ctxt))) + goto error; + + tmpshm->name = virXPathString("string(./name)", ctxt); + if (!tmpshm->name) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem missing name attribute")); + goto error; + } + + shmtype = virXPathString("string(./type)", ctxt); + if (!shmtype) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem missing type attribute")); + goto error; + } + if ((tmpshm->type = virShmObjectTypeFromString(shmtype)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid shmem object type %s"), shmtype);

You leak shmtype here and in other goto error

+ goto error; + } + + if (virXPathULongLong("string(./size)", ctxt, &tmpshm->size) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem missing or have invalid size attribute")); + goto error; + } + + tmpshm->path = virXPathString("string(./path)", ctxt); + if (virXPathBoolean("boolean(./othercreate)", ctxt)) + tmpshm->othercreate = true; + + if (!(drivername = virXPathString("string(./@driver)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem usage element missing driver attribute")); + goto error; + } + nusages = virXPathNodeSet("./domain", ctxt, &usagenode); + if (nusages < 0) + goto error; + + if (nusages > 0) { + size_t i; + + for (i = 0; i < nusages; i++) { + char *domainname; + + save = ctxt->node; + ctxt->node = usagenode[i]; + + if (!(domainname = virXPathString("string(./@name)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem domain element missing name attribute")); + goto error; + } + + if (virShmSetUsedDomain(tmpshm, drivername, domainname) < 0) { + VIR_FREE(domainname); + goto error; + } + VIR_FREE(domainname); + ctxt->node = save; + } + } + *shmobj = tmpshm; + ret = 0; + cleanup: + VIR_FREE(stateFile); + VIR_FREE(drivername); + VIR_FREE(shmtype); + xmlFreeDoc(xml); + xmlXPathFreeContext(ctxt); + return ret; + + error: + virShmObjectFree(tmpshm); + goto cleanup; +} + +static int +virShmObjectListLoadAllState(virShmObjectListPtr list) +{ + DIR *dir; + struct dirent *entry; + + if (!(dir = opendir(list->stateDir))) { + if (errno == ENOENT) + return 0; + virReportSystemError(errno, _("Failed to open dir '%s'"), list->stateDir); + return -1; + } + + while (virDirRead(dir, &entry, list->stateDir) > 0) { + virShmObjectPtr shmobj; + + if (entry->d_name[0] == '.' || + !virFileStripSuffix(entry->d_name, ".xml")) + continue; + if (virShmObjectListLoadState(&shmobj, list->stateDir, entry->d_name) < 0) + continue; + if (virShmObjectListAdd(list, shmobj) < 0) + continue; + } + closedir(dir); + return 0; +} + +static virShmObjectListPtr +virShmObjectListNew(void) +{ + virShmObjectListPtr list; + bool privileged = geteuid() == 0; + + if (!(list = virObjectLockableNew(virShmObjectListClass))) + return NULL; + + virObjectLock(list);

I am not sure the lock is necessary in a New function.

+ if (privileged) { + if (VIR_STRDUP(list->stateDir, SHMEM_STATE_DIR) < 0) + goto error; + } else { + char *rundir = NULL; + + if (!(rundir = virGetUserRuntimeDirectory())) + goto error; + + if (virAsprintf(&list->stateDir, "%s/shmem", rundir) < 0) { + VIR_FREE(rundir); + goto error; + } + VIR_FREE(rundir); + } + if (virFileMakePath(list->stateDir) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to create state dir '%s'"), + list->stateDir); + goto error; + } + if (virShmObjectListLoadAllState(list) < 0) + goto error; + + virObjectUnlock(list); + return list; + + error: + virObjectUnlock(list); + virObjectUnref(list); + return NULL; +} + +static int +virShmOnceInit(void) +{ + if (!(virShmObjectListClass = virClassNew(virClassForObjectLockable(), + "virShmObjectList", + sizeof(virShmObjectList), + virShmObjectListDispose))) + return -1; + + if (!(mainlist = virShmObjectListNew())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virShm) + +virShmObjectListPtr +virShmObjectListGetDefault(void) +{ + if (virShmInitialize() < 0) + return NULL; + + return virObjectRef(mainlist); +} + +int +virShmSetUsedDomain(virShmObjectPtr shmobj, + const char *drvname, + const char *domname) +{ + char *tmpdomain = NULL; + + if (shmobj->drvname) { + if (STRNEQ(drvname, shmobj->drvname)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot use one shmem for different driver")); + goto error; + }

I don't I understand the limitation there. Imho, this could be just a warning.

+ } else { + if (VIR_STRDUP(shmobj->drvname, drvname) < 0) + goto error; + } + + if (VIR_STRDUP(tmpdomain, domname) < 0) + goto error; + + if (VIR_APPEND_ELEMENT(shmobj->domains, shmobj->ndomains, tmpdomain) < 0) + goto error; + + return 0; + + error: + VIR_FREE(tmpdomain); + return -1; +} + +void +virShmObjectFree(virShmObjectPtr shmobj) +{ + size_t i; + + if (!shmobj) + return; + + VIR_FREE(shmobj->name); + VIR_FREE(shmobj->path); + VIR_FREE(shmobj->drvname); + for (i = 0; i < shmobj->ndomains; i++) + VIR_FREE(shmobj->domains[i]); + VIR_FREE(shmobj->domains); + VIR_FREE(shmobj); +} + +virShmObjectPtr +virShmObjectNew(const char *name, + unsigned long long size, + const char *path, + int type, + bool othercreate, + const char *drvname, + const char *domname) +{ + virShmObjectPtr shmobj; + + if (VIR_ALLOC(shmobj) < 0) + return NULL; + + shmobj->size = size; + shmobj->type = type; + + if (VIR_STRDUP(shmobj->name, name) < 0) + goto error; + + if (path) { + if (VIR_STRDUP(shmobj->path, path) < 0) + goto error; + } else { + VIR_FREE(shmobj->path); + } + shmobj->othercreate = othercreate; + + if (virShmSetUsedDomain(shmobj, drvname, domname) < 0) + goto error; + + return shmobj; + + error: + virShmObjectFree(shmobj); + return NULL; +} + +static void +virShmObjectListDispose(void *obj) +{ + virShmObjectListPtr list = obj; + size_t i; + + for (i = 0; i < list->nshmobjs; i++) + virShmObjectFree(list->shmobjs[i]); + + VIR_FREE(list->shmobjs); +} + +int +virShmObjectSaveState(virShmObjectPtr shmobj, + const char *stateDir) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int ret = -1; + size_t i; + char *xml = NULL; + char *stateFile = NULL; + + virBufferAddLit(&buf, "<shmem>\n"); + virBufferAdjustIndent(&buf, 2); + + virBufferAsprintf(&buf, "<name>%s</name>\n", shmobj->name); + + virBufferAsprintf(&buf, "<type>%s</type>\n", virShmObjectTypeToString(shmobj->type)); + + virBufferAsprintf(&buf, "<size>%llu</size>\n", shmobj->size); + if (shmobj->path) + virBufferAsprintf(&buf, "<path>%s</path>\n", shmobj->path); + if (shmobj->othercreate) + virBufferAddLit(&buf, "<othercreate/>\n"); + virBufferAsprintf(&buf, "<driver>%s</driver>\n", shmobj->drvname); + for (i = 0; i < shmobj->ndomains; i++) + virBufferAsprintf(&buf, "<domain name='%s'/>\n", shmobj->domains[i]); + + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</shmem>\n"); + + if (virBufferCheckError(&buf) < 0) + goto cleanup; + + if (!(xml = virBufferContentAndReset(&buf))) + goto cleanup; + + if (!(stateFile = virFileBuildPath(stateDir, shmobj->name, ".xml"))) + goto cleanup; + + if (virXMLSaveFile(stateFile, NULL, NULL, xml) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(stateFile); + VIR_FREE(xml); + return ret; +} + +int +virShmRemoveUsedDomain(virShmObjectPtr shmobj, + const char *drvname, + const char *domname) +{ + size_t i; + + if (shmobj->drvname) { + if (STRNEQ(drvname, shmobj->drvname)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot use one shmem for different driver")); + return -1; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get shmem object driver name")); + return -1; + } + + for (i = 0; i < shmobj->ndomains; i++) { + if (STREQ(shmobj->domains[i], domname)) { + VIR_FREE(shmobj->domains[i]); + VIR_DELETE_ELEMENT(shmobj->domains, i, shmobj->ndomains); + return 0; + } + } + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot find domname in shmem object domain list")); + return -1; +} + +int +virShmObjectRemoveStateFile(virShmObjectListPtr list, + const char *name) +{ + char *stateFile = NULL; + + if (!(stateFile = virFileBuildPath(list->stateDir, name, ".xml"))) + return -1; + unlink(stateFile); + VIR_FREE(stateFile); + return 0; +} + +int +virShmObjectListDel(virShmObjectListPtr list, + virShmObjectPtr shmobj) +{ + size_t i; + + for (i = 0; i < list->nshmobjs; i++) { + virShmObjectPtr tmp = list->shmobjs[i]; + + if (STREQ(tmp->name, shmobj->name)) { + VIR_DELETE_ELEMENT(list->shmobjs, i, list->nshmobjs); + return 0; + } + } + return -1; +} + +virShmObjectPtr +virShmObjectFindByName(virShmObjectListPtr list, + const char *name) +{ + size_t i; + + for (i = 0; i < list->nshmobjs; i++) { + if (STREQ_NULLABLE(list->shmobjs[i]->name, name)) + return list->shmobjs[i]; + } + + return NULL; +} + +int +virShmObjectListAdd(virShmObjectListPtr list, + virShmObjectPtr shmobj) +{ + if (virShmObjectFindByName(list, shmobj->name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("A share memory object named '%s' already exists"), + shmobj->name); + return -1; + } + + return VIR_APPEND_ELEMENT(list->shmobjs, list->nshmobjs, shmobj); +} + +#ifdef HAVE_SHM_OPEN +int +virShmOpen(const char *name, + unsigned long long size, + mode_t mode) +{ + int fd = -1; + struct stat sb; + + if ((fd = shm_open(name, O_RDWR, mode)) < 0) { + virReportSystemError(errno, + _("Unable to open shared memory" + " objects '%s'"), + name); + return -1; + } + + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, + _("cannot stat file '%s'"), name); + goto error; + } + + if (sb.st_size < size) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("already exist shared memory object" + " size %ju is smaller than required size %llu"), + (uintmax_t)sb.st_size, size); + goto error; + } + + return fd; + + error: + VIR_FORCE_CLOSE(fd); + return -1; +} + +int +virShmCreate(const char *name, + unsigned long long size, + bool outerr, + bool *othercreate, + mode_t mode) +{ + int fd = -1; + + fd = shm_open(name, O_RDWR|O_CREAT|O_EXCL, mode); + + if (fd > 0) { + if (ftruncate(fd, size) != 0) { + virReportSystemError(errno, + _("Unable to truncate" + " file descriptor to %llu"), + size); + ignore_value(shm_unlink(name)); + VIR_FORCE_CLOSE(fd); + return -1; + } + } else if (errno == EEXIST) { + if (outerr) { + virReportSystemError(errno, + _("shared memory objects" + " '%s' is already exist"), + name); + return -1; + } + + VIR_WARN("shared memory objects '%s' is already exist", name); + *othercreate = true; + + return virShmOpen(name, size, mode); + } else { + virReportSystemError(errno, + _("Unable to create shared memory" + " objects '%s'"), + name); + return -1; + } + + return fd; +} + +int +virShmUnlink(const char *name) +{ + int ret; + + if ((ret = shm_unlink(name)) < 0) { + virReportSystemError(errno, + _("Unable to delete shared memory" + " objects '%s'"), + name); + } + return ret; +} + +# if defined(__linux__) +# define SHM_DEFAULT_PATH "/dev/shm" + +int +virShmBuildPath(const char *name, char **path) +{ + + if (virAsprintf(path, "%s/%s", SHM_DEFAULT_PATH, name) < 0) + return -1; + + if (!virFileExists(*path)) { + virReportSystemError(errno, + _("could not access %s"), + *path); + VIR_FREE(*path); + return -1; + } + return 0; +} + +# else + +int +virShmBuildPath(const char *name ATTRIBUTE_UNUSED, + char **path ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Cannot get share memory object path on this platform")); + return -2; +} + +# endif +#endif /* HAVE_SHM_OPEN */ diff --git a/src/util/virshm.h b/src/util/virshm.h new file mode 100644 index 0000000..945a541 --- /dev/null +++ b/src/util/virshm.h @@ -0,0 +1,104 @@ +/* + * virshm.h: helper API for POSIX share memory + * + * Copyright (C) 2015 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#ifndef __VIR_SHM_H__ +# define __VIR_SHM_H__ + +# include "internal.h" +# include "virutil.h" +# include "virobject.h" + +typedef enum { + VIR_SHM_TYPE_SHM = 0, + VIR_SHM_TYPE_SERVER, + + VIR_SHM_TYPE_LAST +} virShmObjectType; + +typedef struct _virShmObject virShmObject; +typedef virShmObject *virShmObjectPtr; +struct _virShmObject { + char *name; /* shmem object name */ + int type; /* shmem object type */ + unsigned long long size; /* size of shmem object */ + char *path; /* shmem path */ + bool othercreate; /* a bool parameter record if the shm is created by libvirt */ + + char *drvname; /* which driver */ + char **domains; /* domain(s) using this shm */ + size_t ndomains; /* number of useds */ +}; + +VIR_ENUM_DECL(virShmObject); + +typedef struct _virShmObjectList virShmObjectList; +typedef virShmObjectList *virShmObjectListPtr; +struct _virShmObjectList { + virObjectLockable parent; + char *stateDir; + size_t nshmobjs; + virShmObjectPtr *shmobjs; +}; + +virShmObjectListPtr virShmObjectListGetDefault(void); + +int virShmSetUsedDomain(virShmObjectPtr shmobj, const char *drvname, const char *domname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +int virShmRemoveUsedDomain(virShmObjectPtr shmobj, const char *drvname, const char *domname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +void virShmObjectFree(virShmObjectPtr shmobj); +virShmObjectPtr virShmObjectNew(const char *name, unsigned long long size, + const char *path, int type, bool othercreate, + const char *drvname, const char *domname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_NONNULL(7); +int virShmObjectSaveState(virShmObjectPtr shmobj, const char *stateDir) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virShmObjectRemoveStateFile(virShmObjectListPtr list, const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virShmObjectListAdd(virShmObjectListPtr list, virShmObjectPtr shmobj) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virShmObjectListDel(virShmObjectListPtr list, virShmObjectPtr shmobj) + ATTRIBUTE_NONNULL(1); +virShmObjectPtr virShmObjectFindByName(virShmObjectListPtr list, + const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +# ifdef HAVE_SHM_OPEN +int virShmCreate(const char *name, unsigned long long size, + bool outerr, bool *othercreate, mode_t mode) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); +int virShmOpen(const char *name, + unsigned long long size, + mode_t mode) + ATTRIBUTE_NONNULL(1); +int virShmUnlink(const char *name) + ATTRIBUTE_NONNULL(1); +int virShmBuildPath(const char *name, char **path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +# else /* HAVE_SHM_OPEN */ +# define virShmCreate(name, size, outerr, othercreate, mode) -2 +# define virShmOpen(name, size, mode) -2 +# define virShmUnlink(name) -2 +# define virShmBuildPath(name, path) -2 + +# endif /* HAVE_SHM_OPEN */ +#endif /* __VIR_SHM_H__ */ -- 1.8.3.1

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

-- Marc-André Lureau

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

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- configure.ac | 10 + po/POTFILES.in | 3 +- src/Makefile.am | 5 +- src/libvirt_private.syms | 16 ++ src/util/virshm.c | 623 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virshm.h | 104 ++++++++ 6 files changed, 759 insertions(+), 2 deletions(-) create mode 100644 src/util/virshm.c create mode 100644 src/util/virshm.h

diff --git a/src/Makefile.am b/src/Makefile.am index 7338ab9..048a096 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -152,6 +152,7 @@ UTIL_SOURCES = \ noinst_LTLIBRARIES += libvirt_conf.la @@ -1284,6 +1285,7 @@ libvirt_driver_qemu_impl_la_LIBADD = $(CAPNG_LIBS) \ $(GNUTLS_LIBS) \ $(LIBNL_LIBS) \ $(LIBXML_LIBS) \ + $(LIBRT_LIBS) \ $(NULL) libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)

@@ -2264,6 +2266,7 @@ libvirt_setuid_rpc_client_la_LDFLAGS = \ $(AM_LDFLAGS) \ $(LIBXML_LIBS) \ $(SECDRIVER_LIBS) \ + $(LIBRT_LIBS) \ $(NULL) libvirt_setuid_rpc_client_la_CFLAGS = \ -DLIBVIRT_SETUID_RPC_CLIENT \

Indentation is messed up for these

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index af73177..977fd34 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2078,6 +2078,22 @@ sexpr_u64; string2sexpr;

+# util/virshm.h +virShmBuildPath; +virShmCreate; +virShmObjectFree; +virShmObjectNew; +virShmObjectListAdd; +virShmObjectListDel; +virShmObjectFindByName; +virShmObjectListGetDefault; +virShmObjectRemoveStateFile; +virShmObjectSaveState; +virShmOpen; +virShmRemoveUsedDomain; +virShmSetUsedDomain; +virShmUnlink; +

Did you run 'make check' because they should fail on the sort ordering test.

diff --git a/src/util/virshm.c b/src/util/virshm.c new file mode 100644 index 0000000..7ab39be --- /dev/null +++ b/src/util/virshm.c +#include <config.h> + +#include <fcntl.h> +#include <sys/stat.h> +#include <sys/types.h> + +#ifdef HAVE_SHM_OPEN +# include <sys/mman.h> +#endif + +#include "virshm.h" +#include "virxml.h" +#include "virbuffer.h" +#include "virerror.h" +#include "virstring.h" +#include "virlog.h" +#include "virutil.h" +#include "viralloc.h" +#include "virfile.h" +#include "configmake.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.shm"); + +#define SHMEM_STATE_DIR LOCALSTATEDIR "/run/libvirt/shmem" + +VIR_ENUM_IMPL(virShmObject, VIR_SHM_TYPE_LAST, + "shm", + "server"); + +static virClassPtr virShmObjectListClass; + +static virShmObjectListPtr mainlist; /* global shm object list */ + +static void virShmObjectListDispose(void *obj);

+int +virShmSetUsedDomain(virShmObjectPtr shmobj, + const char *drvname, + const char *domname) +{ + char *tmpdomain = NULL; + + if (shmobj->drvname) { + if (STRNEQ(drvname, shmobj->drvname)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot use one shmem for different driver")); + goto error; + } + } else { + if (VIR_STRDUP(shmobj->drvname, drvname) < 0) + goto error; + } + + if (VIR_STRDUP(tmpdomain, domname) < 0) + goto error; + + if (VIR_APPEND_ELEMENT(shmobj->domains, shmobj->ndomains, tmpdomain) < 0) + goto error; + + return 0; + + error: + VIR_FREE(tmpdomain);

In this error codepath you have possibly already set shmobj->drvname, so you need to be able to undo that.

+ return -1; +}

+#ifdef HAVE_SHM_OPEN +int +virShmOpen(const char *name, + unsigned long long size, + mode_t mode) +{ + int fd = -1; + struct stat sb; + + if ((fd = shm_open(name, O_RDWR, mode)) < 0) { + virReportSystemError(errno, + _("Unable to open shared memory" + " objects '%s'"), + name); + return -1; + } + + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, + _("cannot stat file '%s'"), name); + goto error; + } + + if (sb.st_size < size) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("already exist shared memory object" + " size %ju is smaller than required size %llu"), + (uintmax_t)sb.st_size, size); + goto error;

I think this needs to be != instead of < Consider Domain A with shmsize=1024 and Domain B with shmsize=2048 The check will raise an error if domain A is started before domain B preventing both A & B running at same time. If B is started before A though, it will happily let both run.

+int +virShmCreate(const char *name, + unsigned long long size, + bool outerr, + bool *othercreate, + mode_t mode) +{ + int fd = -1; + + fd = shm_open(name, O_RDWR|O_CREAT|O_EXCL, mode); + + if (fd > 0) { + if (ftruncate(fd, size) != 0) { + virReportSystemError(errno, + _("Unable to truncate" + " file descriptor to %llu"), + size); + ignore_value(shm_unlink(name)); + VIR_FORCE_CLOSE(fd); + return -1; + } + } else if (errno == EEXIST) { + if (outerr) { + virReportSystemError(errno, + _("shared memory objects" + " '%s' is already exist"), + name); + return -1; + } + + VIR_WARN("shared memory objects '%s' is already exist", name); + *othercreate = true; +

You're leaking 'fd' in this case

+ return virShmOpen(name, size, mode); + } else { + virReportSystemError(errno, + _("Unable to create shared memory" + " objects '%s'"), + name); + return -1; + } + + return fd; +} + +int +virShmUnlink(const char *name) +{ + int ret; + + if ((ret = shm_unlink(name)) < 0) { + virReportSystemError(errno, + _("Unable to delete shared memory" + " objects '%s'"), + name); + } + return ret; +} + +# if defined(__linux__) +# define SHM_DEFAULT_PATH "/dev/shm" + +int +virShmBuildPath(const char *name, char **path) +{ + + if (virAsprintf(path, "%s/%s", SHM_DEFAULT_PATH, name) < 0) + return -1; + + if (!virFileExists(*path)) { + virReportSystemError(errno, + _("could not access %s"), + *path); + VIR_FREE(*path); + return -1; + } + return 0; +} + +# else + +int +virShmBuildPath(const char *name ATTRIBUTE_UNUSED, + char **path ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Cannot get share memory object path on this platform")); + return -2; +}

Why -2 instead of -1 ?

diff --git a/src/util/virshm.h b/src/util/virshm.h new file mode 100644 index 0000000..945a541 --- /dev/null +++ b/src/util/virshm.h

+# include "internal.h" +# include "virutil.h" +# include "virobject.h" + +typedef enum { + VIR_SHM_TYPE_SHM = 0, + VIR_SHM_TYPE_SERVER, + + VIR_SHM_TYPE_LAST +} virShmObjectType;

I'm unclear why we need the different type constants here ?

+ +typedef struct _virShmObject virShmObject; +typedef virShmObject *virShmObjectPtr; +struct _virShmObject { + char *name; /* shmem object name */ + int type; /* shmem object type */ + unsigned long long size; /* size of shmem object */ + char *path; /* shmem path */ + bool othercreate; /* a bool parameter record if the shm is created by libvirt */ + + char *drvname; /* which driver */ + char **domains; /* domain(s) using this shm */ + size_t ndomains; /* number of useds */ +}; + +VIR_ENUM_DECL(virShmObject); + +typedef struct _virShmObjectList virShmObjectList; +typedef virShmObjectList *virShmObjectListPtr; +struct _virShmObjectList { + virObjectLockable parent; + char *stateDir; + size_t nshmobjs; + virShmObjectPtr *shmobjs; +};

I think of the struct definitions should be private. If callers need access to the contents we can provide APIs

+# ifdef HAVE_SHM_OPEN +int virShmCreate(const char *name, unsigned long long size, + bool outerr, bool *othercreate, mode_t mode) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); +int virShmOpen(const char *name, + unsigned long long size, + mode_t mode) + ATTRIBUTE_NONNULL(1); +int virShmUnlink(const char *name) + ATTRIBUTE_NONNULL(1); +int virShmBuildPath(const char *name, char **path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +# else /* HAVE_SHM_OPEN */ +# define virShmCreate(name, size, outerr, othercreate, mode) -2 +# define virShmOpen(name, size, mode) -2 +# define virShmUnlink(name) -2 +# define virShmBuildPath(name, path) -2 + +# endif /* HAVE_SHM_OPEN */

You can't just #define away the APIs - this will lead to undefined symbols being referenced in the linker script which breaks on Win32. You need to provide stub implementations. Also you need to use virReportError() any time you return an error status, as you already did for virShmBuildPath. 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 :|

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@redhat.com> wrote:

...

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

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 <lhuang@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