On 11/22/2012 11:48 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> The impls of virSecurityManagerGetMountOptions had no way to
> return errors, since the code was treating 'NULL' as a success
> value. This is somewhat pointless, since the calling code did
> not want NULL in the first place and has to translate it into
> the empty string "". So change the code so that the impls can
> return "" directly, allowing use of NULL for error reporting
> once again
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> src/lxc/lxc_container.c | 10 ++++++----
> src/security/security_apparmor.c | 17 +++++++++++++++++
> src/security/security_manager.c | 5 +----
> src/security/security_nop.c | 15 +++++++++++++--
> src/security/security_selinux.c | 28 +++++++++++++++++-----------
> 5 files changed, 54 insertions(+), 21 deletions(-)
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index db1f6ed..ebeaca1 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -571,7 +571,7 @@ static int lxcContainerMountBasicFS(bool pivotRoot,
> */
>
> ignore_value(virAsprintf(&opts,
> - "mode=755,size=65536%s",(sec_mount_options ? sec_mount_options : "")));
> + "mode=755,size=65536%s", sec_mount_options));
> if (!opts) {
> virReportOOMError();
> goto cleanup;
> @@ -1083,7 +1083,7 @@ static int lxcContainerMountFSTmpfs(virDomainFSDefPtr fs,
> char *data = NULL;
>
> if (virAsprintf(&data,
> - "size=%lldk%s", fs->usage, (sec_mount_options ? sec_mount_options : "")) < 0) {
> + "size=%lldk%s", fs->usage, sec_mount_options) < 0) {
> virReportOOMError();
> goto cleanup;
> }
> @@ -1456,7 +1456,7 @@ static int lxcContainerMountCGroups(struct lxcContainerCGroup *mounts,
> }
>
> if (virAsprintf(&opts,
> - "mode=755,size=65536%s",(sec_mount_options ? sec_mount_options : "")) < 0) {
> + "mode=755,size=65536%s", sec_mount_options) < 0) {
> virReportOOMError();
> return -1;
> }
> @@ -1689,7 +1689,9 @@ static int lxcContainerSetupMounts(virDomainDefPtr vmDef,
> if (lxcContainerResolveSymlinks(vmDef) < 0)
> return -1;
>
> - sec_mount_options = virSecurityManagerGetMountOptions(securityDriver, vmDef);
> + if (!(sec_mount_options = virSecurityManagerGetMountOptions(securityDriver, vmDef)))
> + return -1;
> +
> if (root && root->src)
> rc = lxcContainerSetupPivotRoot(vmDef, root, ttyPaths, nttyPaths, sec_mount_options);
> else
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index 1315fe1..b0cdb65 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -881,6 +881,21 @@ AppArmorSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> return 0;
> }
>
> +
> +static char *
> +AppArmorGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + virDomainDefPtr vm ATTRIBUTE_UNUSED)
> +{
> + char *opts;
> +
> + if (!(opts = strdup(""))) {
> + virReportOOMError();
> + return NULL;
> + }
> + return opts;
> +}
> +
> +
> virSecurityDriver virAppArmorSecurityDriver = {
> .privateDataLen = 0,
> .name = SECURITY_APPARMOR_NAME,
> @@ -918,4 +933,6 @@ virSecurityDriver virAppArmorSecurityDriver = {
>
> .domainSetSecurityImageFDLabel = AppArmorSetImageFDLabel,
> .domainSetSecurityTapFDLabel = AppArmorSetTapFDLabel,
> +
> + .domainGetSecurityMountOptions = AppArmorGetMountOptions,
> };
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index d446607..0ebd53b 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -486,10 +486,7 @@ char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr,
> if (mgr->drv->domainGetSecurityMountOptions)
> return mgr->drv->domainGetSecurityMountOptions(mgr, vm);
>
> - /*
> - I don't think this is an error, these should be optional
> - virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> - */
> + virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> return NULL;
> }
>
> diff --git a/src/security/security_nop.c b/src/security/security_nop.c
> index 86f644b..5f3270a 100644
> --- a/src/security/security_nop.c
> +++ b/src/security/security_nop.c
> @@ -21,6 +21,10 @@
>
> #include "security_nop.h"
>
> +#include "virterror_internal.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_SECURITY
> +
> static virSecurityDriverStatus virSecurityDriverProbeNop(const char *virtDriver ATTRIBUTE_UNUSED)
> {
> return SECURITY_DRIVER_ENABLE;
> @@ -165,8 +169,15 @@ static int virSecurityDomainSetFDLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UN
> }
>
> static char *virSecurityDomainGetMountOptionsNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> - virDomainDefPtr vm ATTRIBUTE_UNUSED) {
> - return NULL;
> + virDomainDefPtr vm ATTRIBUTE_UNUSED)
> +{
> + char *opts;
> +
> + if (!(opts = strdup(""))) {
> + virReportOOMError();
> + return NULL;
> + }
> + return opts;
> }
>
> virSecurityDriver virSecurityDriverNop = {
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 8fcaaa8..0e49ded 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1974,20 +1974,26 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManagerPtr mgr,
> char *opts = NULL;
> virSecurityLabelDefPtr secdef;
>
> - secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
> - if (secdef == NULL)
> - return NULL;
> -
> - if (! secdef->imagelabel)
> - secdef->imagelabel = virSecuritySELinuxGenImageLabel(mgr,def);
> + if ((secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME))) {
> + if (!secdef->imagelabel)
> + secdef->imagelabel = virSecuritySELinuxGenImageLabel(mgr,def);
Missing space after comma (was already there, but might as well fix it
while you're moving stuff).
> +
> + if (secdef->imagelabel &&
> + virAsprintf(&opts,
> + ",context=\"%s\"",
> + (const char*) secdef->imagelabel) < 0) {
> + virReportOOMError();
> + return NULL;
> + }
> + }
>
> - if (secdef->imagelabel) {
> - virAsprintf(&opts,
> - ",context=\"%s\"",
> - (const char*) secdef->imagelabel);
> + if (!opts &&
> + !(opts = strdup(""))) {
> + virReportOOMError();
> + return NULL;
> }
>
> - VIR_DEBUG("imageLabel=%s", secdef->imagelabel);
> + VIR_DEBUG("imageLabel=%s opts=%s", secdef->imagelabel, opts);
> return opts;
> }
>
ACK with the space added.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list