Fix error handling in virSecurityManagerGetMountOptions

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

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index db1f6ed18f2b37867967b780f278dff2f01e8d94..ebeaca1377565e4b4ee53b3c59074fcabff91573 100644 (file)
--- 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 1315fe14750806f27f6e58abe7fd8071f1cd2435..b0cdb65fbef1266020189d4ca350e6caa5835495 100644 (file)
--- 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 d446607fcd980573c66d386360c233ff55df3b95..0ebd53b563d8709c4ae31f3a0c3c1f185035d046 100644 (file)
--- 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 86f644bd2c82b24d09788e035cff75b55101faa9..5f3270a32dc6aadebd8fe6a77c1461c1e3d7c3f7 100644 (file)
--- 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 8fcaaa82837eb6df169d387230fa4773fa5dd9a0..5409e32a4058b881a2378e3b388a1329b5c65891 100644 (file)
--- 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);
+
+        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;
 }