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

Re: [libvirt] [PATCH v3 1/5] Internal refactory of data structures



On 03.08.2012 16:18, Marcelo Cerri wrote:
> This patch updates the structures that store information about each
> domain and each hypervisor to support multiple security labels and
> drivers. It also updates all the remaining code to use the new fields.
> 
> Signed-off-by: Marcelo Cerri <mhcerri linux vnet ibm com>
> ---
>  src/conf/capabilities.c         |   17 ++++--
>  src/conf/capabilities.h         |    6 ++-
>  src/conf/domain_audit.c         |   14 +++--
>  src/conf/domain_conf.c          |   81 ++++++++++++++++-------
>  src/conf/domain_conf.h          |    9 ++-
>  src/lxc/lxc_conf.c              |    8 ++-
>  src/lxc/lxc_controller.c        |    8 +-
>  src/lxc/lxc_driver.c            |   11 ++--
>  src/lxc/lxc_process.c           |   23 ++++---
>  src/qemu/qemu_driver.c          |   15 +++--
>  src/qemu/qemu_process.c         |   18 +++---
>  src/security/security_manager.c |   12 ++--
>  src/security/security_selinux.c |  133 ++++++++++++++++++++-------------------
>  src/test/test_driver.c          |   11 ++-
>  14 files changed, 216 insertions(+), 150 deletions(-)
> 
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 8b9b516..9a0bc3d 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -181,8 +181,13 @@ virCapabilitiesFree(virCapsPtr caps) {
>      VIR_FREE(caps->host.migrateTrans);
>  
>      VIR_FREE(caps->host.arch);
> -    VIR_FREE(caps->host.secModel.model);
> -    VIR_FREE(caps->host.secModel.doi);
> +
> +    for (i = 0; i < caps->host.nsecModels; i++) {
> +        VIR_FREE(caps->host.secModels[i].model);
> +        VIR_FREE(caps->host.secModels[i].doi);
> +    }
> +    VIR_FREE(caps->host.secModels);
> +
>      virCPUDefFree(caps->host.cpu);
>  
>      VIR_FREE(caps);
> @@ -767,10 +772,12 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>          virBufferAddLit(&xml, "    </topology>\n");
>      }
>  
> -    if (caps->host.secModel.model) {
> +    if (caps->host.nsecModels) {
>          virBufferAddLit(&xml, "    <secmodel>\n");
> -        virBufferAsprintf(&xml, "      <model>%s</model>\n", caps->host.secModel.model);
> -        virBufferAsprintf(&xml, "      <doi>%s</doi>\n", caps->host.secModel.doi);
> +        virBufferAsprintf(&xml, "      <model>%s</model>\n",
> +                          caps->host.secModels[0].model);
> +        virBufferAsprintf(&xml, "      <doi>%s</doi>\n",
> +                          caps->host.secModels[0].doi);
>          virBufferAddLit(&xml, "    </secmodel>\n");
>      }
>  
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index 460b273..e43f404 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -93,6 +93,7 @@ struct _virCapsHostNUMACell {
>  };
>  
>  typedef struct _virCapsHostSecModel virCapsHostSecModel;
> +typedef virCapsHostSecModel *virCapsHostSecModelPtr;
>  struct _virCapsHostSecModel {
>      char *model;
>      char *doi;
> @@ -116,7 +117,10 @@ struct _virCapsHost {
>      size_t nnumaCell;
>      size_t nnumaCell_max;
>      virCapsHostNUMACellPtr *numaCell;
> -    virCapsHostSecModel secModel;
> +
> +    size_t nsecModels;
> +    virCapsHostSecModelPtr secModels;
> +
>      virCPUDefPtr cpu;
>      unsigned char host_uuid[VIR_UUID_BUFLEN];
>  };
> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
> index 5300371..59db649 100644
> --- a/src/conf/domain_audit.c
> +++ b/src/conf/domain_audit.c
> @@ -618,6 +618,7 @@ virDomainAuditSecurityLabel(virDomainObjPtr vm, bool success)
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>      char *vmname;
>      const char *virt;
> +    int i;
>  
>      virUUIDFormat(vm->def->uuid, uuidstr);
>      if (!(vmname = virAuditEncode("vm", vm->def->name))) {
> @@ -630,11 +631,14 @@ virDomainAuditSecurityLabel(virDomainObjPtr vm, bool success)
>          virt = "?";
>      }
>  
> -    VIR_AUDIT(VIR_AUDIT_RECORD_MACHINE_ID, success,
> -              "virt=%s %s uuid=%s vm-ctx=%s img-ctx=%s",
> -              virt, vmname, uuidstr,
> -              VIR_AUDIT_STR(vm->def->seclabel.label),
> -              VIR_AUDIT_STR(vm->def->seclabel.imagelabel));
> +    for (i = 0; i < vm->def->nseclabels; i++) {
> +        VIR_AUDIT(VIR_AUDIT_RECORD_MACHINE_ID, success,
> +                  "virt=%s %s uuid=%s vm-ctx=%s img-ctx=%s model=%s",
> +                  virt, vmname, uuidstr,
> +                  VIR_AUDIT_STR(vm->def->seclabels[i]->label),
> +                  VIR_AUDIT_STR(vm->def->seclabels[i]->imagelabel),
> +                  VIR_AUDIT_STR(vm->def->seclabels[i]->model));
> +    }
>  
>      VIR_FREE(vmname);
>  }
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 58603a3..1c318a0 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -855,12 +855,15 @@ virDomainGraphicsListenDefClear(virDomainGraphicsListenDefPtr def)
>  }
>  
>  static void
> -virSecurityLabelDefClear(virSecurityLabelDefPtr def)
> +virSecurityLabelDefFree(virSecurityLabelDefPtr def)
>  {
> +    if (!def)
> +        return;
>      VIR_FREE(def->model);
>      VIR_FREE(def->label);
>      VIR_FREE(def->imagelabel);
>      VIR_FREE(def->baselabel);
> +    VIR_FREE(def);
>  }
>  
>  
> @@ -869,6 +872,7 @@ virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDefPtr def)
>  {
>      if (!def)
>          return;
> +    VIR_FREE(def->model);
>      VIR_FREE(def->label);
>      VIR_FREE(def);
>  }
> @@ -954,7 +958,11 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
>      virStorageEncryptionFree(def->encryption);
>      virDomainDeviceInfoClear(&def->info);
>  
> -    virSecurityDeviceLabelDefFree(def->seclabel);
> +    if (def->seclabels) {
> +        for (i = 0; i < def->nseclabels; i++)
> +            virSecurityDeviceLabelDefFree(def->seclabels[i]);
> +        VIR_FREE(def->seclabels);
> +    }
>  
>      for (i = 0 ; i < def->nhosts ; i++)
>          virDomainDiskHostDefFree(&def->hosts[i]);
> @@ -1620,7 +1628,9 @@ void virDomainDefFree(virDomainDefPtr def)
>  
>      virDomainMemballoonDefFree(def->memballoon);
>  
> -    virSecurityLabelDefClear(&def->seclabel);
> +    for (i = 0; i < def->nseclabels; i++)
> +        virSecurityLabelDefFree(def->seclabels[i]);
> +    VIR_FREE(def->seclabels);
>  
>      virCPUDefFree(def->cpu);
>  
> @@ -3075,10 +3085,10 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
>  {
>      char *p;
>  
> -    if (virXPathNode("./seclabel", ctxt) == NULL)
> +    if (virXPathNode("./seclabel[1]", ctxt) == NULL)
>          return 0;
>  
> -    p = virXPathStringLimit("string(./seclabel/@type)",
> +    p = virXPathStringLimit("string(./seclabel[1]/@type)",
>                              VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
>      if (p == NULL) {
>          def->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
> @@ -3092,7 +3102,7 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
>          }
>      }
>  
> -    p = virXPathStringLimit("string(./seclabel/@relabel)",
> +    p = virXPathStringLimit("string(./seclabel[1]/@relabel)",
>                              VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
>      if (p != NULL) {
>          if (STREQ(p, "yes")) {
> @@ -3132,7 +3142,7 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
>      if (def->type == VIR_DOMAIN_SECLABEL_STATIC ||
>          (!(flags & VIR_DOMAIN_XML_INACTIVE) &&
>           def->type != VIR_DOMAIN_SECLABEL_NONE)) {
> -        p = virXPathStringLimit("string(./seclabel/label[1])",
> +        p = virXPathStringLimit("string(./seclabel[1]/label[1])",
>                                  VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
>          if (p == NULL) {
>              virReportError(VIR_ERR_XML_ERROR,
> @@ -3147,7 +3157,7 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
>      if (!def->norelabel &&
>          (!(flags & VIR_DOMAIN_XML_INACTIVE) &&
>           def->type != VIR_DOMAIN_SECLABEL_NONE)) {
> -        p = virXPathStringLimit("string(./seclabel/imagelabel[1])",
> +        p = virXPathStringLimit("string(./seclabel[1]/imagelabel[1])",
>                                  VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
>          if (p == NULL) {
>              virReportError(VIR_ERR_XML_ERROR,
> @@ -3159,7 +3169,7 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
>  
>      /* Only parse baselabel for dynamic label type */
>      if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
> -        p = virXPathStringLimit("string(./seclabel/baselabel[1])",
> +        p = virXPathStringLimit("string(./seclabel[1]/baselabel[1])",
>                                  VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
>          def->baselabel = p;
>      }
> @@ -3171,7 +3181,7 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
>          def->baselabel ||
>          (!(flags & VIR_DOMAIN_XML_INACTIVE) &&
>           def->type != VIR_DOMAIN_SECLABEL_NONE)) {
> -        p = virXPathStringLimit("string(./seclabel/@model)",
> +        p = virXPathStringLimit("string(./seclabel[1]/@model)",
>                                  VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
>          if (p == NULL) {
>              virReportError(VIR_ERR_XML_ERROR,
> @@ -3184,7 +3194,7 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
>      return 0;
>  
>  error:
> -    virSecurityLabelDefClear(def);
> +    virSecurityLabelDefFree(def);
>      return -1;
>  }
>  
> @@ -3198,7 +3208,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr *def,
>  
>      *def = NULL;
>  
> -    if (virXPathNode("./seclabel", ctxt) == NULL)
> +    if (virXPathNode("./seclabel[1]", ctxt) == NULL)
>          return 0;
>  
>      /* Can't use overrides if top-level doesn't allow relabeling.  */
> @@ -3214,7 +3224,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr *def,
>          return -1;
>      }
>  
> -    p = virXPathStringLimit("string(./seclabel/@relabel)",
> +    p = virXPathStringLimit("string(./seclabel[1]/@relabel)",
>                              VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
>      if (p != NULL) {
>          if (STREQ(p, "yes")) {
> @@ -3233,7 +3243,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr *def,
>          (*def)->norelabel = false;
>      }
>  
> -    p = virXPathStringLimit("string(./seclabel/label[1])",
> +    p = virXPathStringLimit("string(./seclabel[1]/label[1])",
>                              VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
>      (*def)->label = p;
>  
> @@ -3667,10 +3677,16 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>      if (sourceNode) {
>          xmlNodePtr saved_node = ctxt->node;
>          ctxt->node = sourceNode;
> -        if (virSecurityDeviceLabelDefParseXML(&def->seclabel,
> +        if ((VIR_ALLOC(def->seclabels) < 0) ||
> +            (VIR_ALLOC(def->seclabels[0]) < 0)) {
> +            virReportOOMError();
> +            goto error;
> +        }
> +        if (virSecurityDeviceLabelDefParseXML(&def->seclabels[0],
>                                                vmSeclabel,
>                                                ctxt) < 0)
>              goto error;
> +        def->nseclabels = 1;
>          ctxt->node = saved_node;
>      }
>  
> @@ -7120,10 +7136,17 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps,
>          goto error;
>      }
>  
> +    if ((VIR_ALLOC(def->seclabels) < 0) ||
> +        (VIR_ALLOC(def->seclabels[0])) < 0 ) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +

'def' is passed as 'const virDomainDefPtr def' here. I guess we could
preserve const correctness here.
Moreover, are we guaranteed that these haven't been allocated yet? I
mean, virDomainDeviceDefParse is called from virDomainDeviceDefCopy()
too which is called when doing live+config device attach 'virsh
attach-devoce --persistent device.xml'

>      if (xmlStrEqual(node->name, BAD_CAST "disk")) {
>          dev->type = VIR_DOMAIN_DEVICE_DISK;
>          if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt,
> -                                                        NULL, &def->seclabel, flags)))
> +                                                        NULL, def->seclabels[0],
> +                                                        flags)))
>              goto error;
>      } else if (xmlStrEqual(node->name, BAD_CAST "lease")) {
>          dev->type = VIR_DOMAIN_DEVICE_LEASE;
> @@ -8061,7 +8084,13 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>  
>      /* analysis of security label, done early even though we format it
>       * late, so devices can refer to this for defaults */
> -    if (virSecurityLabelDefParseXML(&def->seclabel, ctxt, flags) == -1)
> +    if ((VIR_ALLOC(def->seclabels) < 0) ||
> +        (VIR_ALLOC(def->seclabels[0]) < 0)) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +    def->nseclabels = 1;
> +    if (virSecurityLabelDefParseXML(def->seclabels[0], ctxt, flags) == -1)
>          goto error;
>  
>      /* Extract domain memory */
> @@ -8661,7 +8690,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>                                                              nodes[i],
>                                                              ctxt,
>                                                              bootMap,
> -                                                            &def->seclabel,
> +                                                            def->seclabels[0],
>                                                              flags);
>          if (!disk)
>              goto error;
> @@ -11181,10 +11210,10 @@ virDomainDiskDefFormat(virBufferPtr buf,
>              if (def->startupPolicy)
>                  virBufferEscapeString(buf, " startupPolicy='%s'",
>                                        startupPolicy);
> -            if (def->seclabel) {
> +            if (def->seclabels && def->seclabels[0]) {

I guess def->nseclabels would be sufficient here.

>                  virBufferAddLit(buf, ">\n");
>                  virBufferAdjustIndent(buf, 8);
> -                virSecurityDeviceLabelDefFormat(buf, def->seclabel);
> +                virSecurityDeviceLabelDefFormat(buf, def->seclabels[0]);
>                  virBufferAdjustIndent(buf, -8);
>                  virBufferAddLit(buf, "      </source>\n");
>              } else {
> @@ -11194,10 +11223,10 @@ virDomainDiskDefFormat(virBufferPtr buf,
>          case VIR_DOMAIN_DISK_TYPE_BLOCK:
>              virBufferEscapeString(buf, "      <source dev='%s'",
>                                    def->src);
> -            if (def->seclabel) {
> +            if (def->seclabels && def->seclabels[0]) {

ditto.

>                  virBufferAddLit(buf, ">\n");
>                  virBufferAdjustIndent(buf, 8);
> -                virSecurityDeviceLabelDefFormat(buf, def->seclabel);
> +                virSecurityDeviceLabelDefFormat(buf, def->seclabels[0]);
>                  virBufferAdjustIndent(buf, -8);
>                  virBufferAddLit(buf, "      </source>\n");
>              } else {


Otherwise I haven't spotted anything wrong.

ACK modulo problem with 'def' within virDomainDeviceDefParse().

Michal


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