[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/4] conf: move seclabel for chardev source to the correct sturcture




On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina redhat com>
> ---
> 
> Notes:
>     new in v2
> 
>  src/conf/domain_conf.c          | 46 +++++++++++++++++++----------------------
>  src/conf/domain_conf.h          |  9 ++++----
>  src/security/security_dac.c     | 26 ++++++++++-------------
>  src/security/security_manager.c |  4 ++--
>  src/security/security_selinux.c | 24 +++++++++------------
>  5 files changed, 49 insertions(+), 60 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c7e20b8ba1..68dc2832cb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2076,12 +2076,21 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
>  
>  void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
>  {
> +    size_t i;
> +
>      if (!def)
>          return;
>  
>      virDomainChrSourceDefClear(def);
>      virObjectUnref(def->privateData);
>  
> +    if (def->seclabels) {
> +        for (i = 0; i < def->nseclabels; i++)
> +            virSecurityDeviceLabelDefFree(def->seclabels[i]);
> +        VIR_FREE(def->seclabels);
> +    }
> +
> +
>      VIR_FREE(def);
>  }
>  
> @@ -2150,8 +2159,6 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src,
>  
>  void virDomainChrDefFree(virDomainChrDefPtr def)
>  {
> -    size_t i;
> -
>      if (!def)
>          return;
>  
> @@ -2176,12 +2183,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
>      virDomainChrSourceDefFree(def->source);
>      virDomainDeviceInfoClear(&def->info);
>  
> -    if (def->seclabels) {
> -        for (i = 0; i < def->nseclabels; i++)
> -            virSecurityDeviceLabelDefFree(def->seclabels[i]);
> -        VIR_FREE(def->seclabels);
> -    }
> -
>      VIR_FREE(def);
>  }
>  
> @@ -10688,8 +10689,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>                  if (chr_def) {

Is the @chr_def check necessary still?  I assume it's there because
chr_def can be passed as NULL in some cases.

Looks like all this was added as part of commit 'f8b08d0e'

The chr_def would be NULL for Smartcard, RNG, and Redirdev callers which
each now doesn't pass a NULL to virDomainChrSourceDefFormat, so that
leads me to believe that the @chr_def should be removed.

The rest looks good, so

Reviewed-by: John Ferlan <jferlan redhat com>


John

>                      xmlNodePtr saved_node = ctxt->node;
>                      ctxt->node = cur;
> -                    if (virSecurityDeviceLabelDefParseXML(&chr_def->seclabels,
> -                                                          &chr_def->nseclabels,
> +                    if (virSecurityDeviceLabelDefParseXML(&def->seclabels,
> +                                                          &def->nseclabels,
>                                                            vmSeclabels,
>                                                            nvmSeclabels,
>                                                            ctxt,
> @@ -22399,19 +22400,11 @@ virDomainNetDefFormat(virBufferPtr buf,
>   * output at " type='type'>". */
>  static int
>  virDomainChrSourceDefFormat(virBufferPtr buf,
> -                            virDomainChrDefPtr chr_def,
>                              virDomainChrSourceDefPtr def,
>                              bool tty_compat,
>                              unsigned int flags)
>  {
>      const char *type = virDomainChrTypeToString(def->type);
> -    size_t nseclabels = 0;
> -    virSecurityDeviceLabelDefPtr *seclabels = NULL;
> -
> -    if (chr_def) {
> -        nseclabels = chr_def->nseclabels;
> -        seclabels = chr_def->seclabels;
> -    }
>  
>      if (!type) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -22449,7 +22442,8 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>                  def->data.file.append != VIR_TRISTATE_SWITCH_ABSENT)
>                  virBufferAsprintf(buf, " append='%s'",
>                      virTristateSwitchTypeToString(def->data.file.append));
> -            virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
> +            virDomainSourceDefFormatSeclabel(buf, def->nseclabels,
> +                                             def->seclabels, flags);
>          }
>          break;
>  
> @@ -22504,7 +22498,8 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>              virBufferAsprintf(buf, "<source mode='%s'",
>                                def->data.nix.listen ? "bind" : "connect");
>              virBufferEscapeString(buf, " path='%s'", def->data.nix.path);
> -            virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
> +            virDomainSourceDefFormatSeclabel(buf, def->nseclabels,
> +                                             def->seclabels, flags);
>          }
>          break;
>  
> @@ -22553,7 +22548,7 @@ virDomainChrDefFormat(virBufferPtr buf,
>                    def->source->type == VIR_DOMAIN_CHR_TYPE_PTY &&
>                    !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
>                    def->source->data.file.path);
> -    if (virDomainChrSourceDefFormat(buf, def, def->source, tty_compat, flags) < 0)
> +    if (virDomainChrSourceDefFormat(buf, def->source, tty_compat, flags) < 0)
>          return -1;
>  
>      /* Format <target> block */
> @@ -22675,7 +22670,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
>          break;
>  
>      case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
> -        if (virDomainChrSourceDefFormat(buf, NULL, def->data.passthru, false,
> +        if (virDomainChrSourceDefFormat(buf, def->data.passthru, false,
>                                          flags) < 0)
>              return -1;
>          break;
> @@ -22981,7 +22976,7 @@ virDomainRNGDefFormat(virBufferPtr buf,
>  
>      case VIR_DOMAIN_RNG_BACKEND_EGD:
>          virBufferAdjustIndent(buf, 2);
> -        if (virDomainChrSourceDefFormat(buf, NULL, def->source.chardev,
> +        if (virDomainChrSourceDefFormat(buf, def->source.chardev,
>                                          false, flags) < 0)
>              return -1;
>          virBufferAdjustIndent(buf, -2);
> @@ -23797,7 +23792,7 @@ virDomainRedirdevDefFormat(virBufferPtr buf,
>  
>      virBufferAsprintf(buf, "<redirdev bus='%s'", bus);
>      virBufferAdjustIndent(buf, 2);
> -    if (virDomainChrSourceDefFormat(buf, NULL, def->source, false, flags) < 0)
> +    if (virDomainChrSourceDefFormat(buf, def->source, false, flags) < 0)
>          return -1;
>      if (virDomainDeviceInfoFormat(buf, &def->info,
>                                    flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) < 0)
> @@ -26195,7 +26190,8 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model)
>  
>  
>  virSecurityDeviceLabelDefPtr
> -virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model)
> +virDomainChrSourceDefGetSecurityLabelDef(virDomainChrSourceDefPtr def,
> +                                         const char *model)
>  {
>      size_t i;
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 83e0672691..1951ba74bb 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1166,6 +1166,9 @@ struct _virDomainChrSourceDef {
>      } data;
>      char *logfile;
>      int logappend;
> +
> +    size_t nseclabels;
> +    virSecurityDeviceLabelDefPtr *seclabels;
>  };
>  
>  /* A complete character device, both host and domain views.  */
> @@ -1188,9 +1191,6 @@ struct _virDomainChrDef {
>      virDomainChrSourceDefPtr source;
>  
>      virDomainDeviceInfo info;
> -
> -    size_t nseclabels;
> -    virSecurityDeviceLabelDefPtr *seclabels;
>  };
>  
>  typedef enum {
> @@ -3068,7 +3068,8 @@ virSecurityLabelDefPtr
>  virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model);
>  
>  virSecurityDeviceLabelDefPtr
> -virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model);
> +virDomainChrSourceDefGetSecurityLabelDef(virDomainChrSourceDefPtr def,
> +                                         const char *model);
>  
>  typedef const char* (*virEventActionToStringFunc)(int type);
>  typedef int (*virEventActionFromStringFunc)(const char *type);
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 7dcf4c15f7..fd4d8f5047 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -1159,7 +1159,6 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
>  static int
>  virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
>                                virDomainDefPtr def,
> -                              virDomainChrDefPtr dev,
>                                virDomainChrSourceDefPtr dev_source)
>  
>  {
> @@ -1173,9 +1172,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
>  
>      seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
>  
> -    if (dev)
> -        chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
> -                                                          SECURITY_DAC_NAME);
> +    chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
> +                                                            SECURITY_DAC_NAME);
>  
>      if (chr_seclabel && !chr_seclabel->relabel)
>          return 0;
> @@ -1245,7 +1243,6 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
>  static int
>  virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
>                                    virDomainDefPtr def ATTRIBUTE_UNUSED,
> -                                  virDomainChrDefPtr dev,
>                                    virDomainChrSourceDefPtr dev_source)
>  {
>      virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> @@ -1253,9 +1250,8 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
>      char *in = NULL, *out = NULL;
>      int ret = -1;
>  
> -    if (dev)
> -        chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
> -                                                          SECURITY_DAC_NAME);
> +    chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
> +                                                            SECURITY_DAC_NAME);
>  
>      if (chr_seclabel && !chr_seclabel->relabel)
>          return 0;
> @@ -1304,12 +1300,12 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr,
>  
>  static int
>  virSecurityDACRestoreChardevCallback(virDomainDefPtr def,
> -                                     virDomainChrDefPtr dev,
> +                                     virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
>                                       void *opaque)
>  {
>      virSecurityManagerPtr mgr = opaque;
>  
> -    return virSecurityDACRestoreChardevLabel(mgr, def, dev, dev->source);
> +    return virSecurityDACRestoreChardevLabel(mgr, def, dev->source);
>  }
>  
>  
> @@ -1322,7 +1318,7 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,
>  
>      switch (tpm->type) {
>      case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> -        ret = virSecurityDACSetChardevLabel(mgr, def, NULL,
> +        ret = virSecurityDACSetChardevLabel(mgr, def,
>                                              &tpm->data.passthrough.source);
>          break;
>      case VIR_DOMAIN_TPM_TYPE_LAST:
> @@ -1342,8 +1338,8 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,
>  
>      switch (tpm->type) {
>      case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> -        ret = virSecurityDACRestoreChardevLabel(mgr, def, NULL,
> -                                          &tpm->data.passthrough.source);
> +        ret = virSecurityDACRestoreChardevLabel(mgr, def,
> +                                                &tpm->data.passthrough.source);
>          break;
>      case VIR_DOMAIN_TPM_TYPE_LAST:
>          break;
> @@ -1506,12 +1502,12 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
>  
>  static int
>  virSecurityDACSetChardevCallback(virDomainDefPtr def,
> -                                 virDomainChrDefPtr dev,
> +                                 virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
>                                   void *opaque)
>  {
>      virSecurityManagerPtr mgr = opaque;
>  
> -    return virSecurityDACSetChardevLabel(mgr, def, dev, dev->source);
> +    return virSecurityDACSetChardevLabel(mgr, def, dev->source);
>  }
>  
>  
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 6c777db1e6..90d491c1bc 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -811,8 +811,8 @@ virSecurityManagerCheckChardevLabel(virSecurityManagerPtr mgr,
>  {
>      size_t i;
>  
> -    for (i = 0; i < dev->nseclabels; i++) {
> -        if (virSecurityManagerCheckModel(mgr, dev->seclabels[i]->model) < 0)
> +    for (i = 0; i < dev->source->nseclabels; i++) {
> +        if (virSecurityManagerCheckModel(mgr, dev->source->seclabels[i]->model) < 0)
>              return -1;
>      }
>  
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 9504a4be34..75f387b3fa 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -2179,7 +2179,6 @@ virSecuritySELinuxRestoreHostdevLabel(virSecurityManagerPtr mgr,
>  static int
>  virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
>                                    virDomainDefPtr def,
> -                                  virDomainChrDefPtr dev,
>                                    virDomainChrSourceDefPtr dev_source)
>  
>  {
> @@ -2193,9 +2192,8 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
>      if (!seclabel || !seclabel->relabel)
>          return 0;
>  
> -    if (dev)
> -        chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
> -                                                          SECURITY_SELINUX_NAME);
> +    chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
> +                                                            SECURITY_SELINUX_NAME);
>  
>      if (chr_seclabel && !chr_seclabel->relabel)
>          return 0;
> @@ -2254,7 +2252,6 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr,
>  static int
>  virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
>                                        virDomainDefPtr def,
> -                                      virDomainChrDefPtr dev,
>                                        virDomainChrSourceDefPtr dev_source)
>  
>  {
> @@ -2267,9 +2264,8 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
>      if (!seclabel || !seclabel->relabel)
>          return 0;
>  
> -    if (dev)
> -        chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev,
> -                                                          SECURITY_SELINUX_NAME);
> +    chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source,
> +                                                            SECURITY_SELINUX_NAME);
>      if (chr_seclabel && !chr_seclabel->relabel)
>          return 0;
>  
> @@ -2318,12 +2314,12 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr,
>  
>  static int
>  virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def,
> -                                                 virDomainChrDefPtr dev,
> +                                                 virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
>                                                   void *opaque)
>  {
>      virSecurityManagerPtr mgr = opaque;
>  
> -    return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev, dev->source);
> +    return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->source);
>  }
>  
>  
> @@ -2346,7 +2342,7 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def,
>          return virSecuritySELinuxRestoreFileLabel(mgr, database);
>  
>      case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
> -        return virSecuritySELinuxRestoreChardevLabel(mgr, def, NULL, dev->data.passthru);
> +        return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev->data.passthru);
>  
>      default:
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -2707,12 +2703,12 @@ virSecuritySELinuxClearSocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
>  
>  static int
>  virSecuritySELinuxSetSecurityChardevCallback(virDomainDefPtr def,
> -                                             virDomainChrDefPtr dev,
> +                                             virDomainChrDefPtr dev ATTRIBUTE_UNUSED,
>                                               void *opaque)
>  {
>      virSecurityManagerPtr mgr = opaque;
>  
> -    return virSecuritySELinuxSetChardevLabel(mgr, def, dev, dev->source);
> +    return virSecuritySELinuxSetChardevLabel(mgr, def, dev->source);
>  }
>  
>  
> @@ -2736,7 +2732,7 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def,
>          return virSecuritySELinuxSetFilecon(mgr, database, data->content_context);
>  
>      case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
> -        return virSecuritySELinuxSetChardevLabel(mgr, def, NULL,
> +        return virSecuritySELinuxSetChardevLabel(mgr, def,
>                                                   dev->data.passthru);
>  
>      default:
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]