[libvirt] [PATCH 5/6] Fix error handling in virSecurityManagerGetMountOptions

[Laine Stump]

On 11/22/2012 11:48 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
>
> 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 at redhat.com>
> ---
>  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.