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

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



On 18.07.2012 03:28, 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.
> ---
>  src/conf/capabilities.c         |   17 ++++--
>  src/conf/capabilities.h         |    6 ++-
>  src/conf/domain_audit.c         |   14 +++--
>  src/conf/domain_conf.c          |   86 +++++++++++++++++---------
>  src/conf/domain_conf.h          |    9 ++-
>  src/lxc/lxc_conf.c              |    8 ++-
>  src/lxc/lxc_controller.c        |    8 +-
>  src/lxc/lxc_driver.c            |   35 ++++++-----
>  src/qemu/qemu_driver.c          |   15 +++--
>  src/qemu/qemu_process.c         |   18 +++---
>  src/security/security_manager.c |   10 ++--
>  src/security/security_selinux.c |  132 +++++++++++++++++++-------------------
>  src/test/test_driver.c          |   11 ++-
>  13 files changed, 217 insertions(+), 152 deletions(-)

We must update XML schema as well as we are going to allow more
<model> and <doi> elements under <secmodel>. And maybe we want to add a test case. But that can be a follow up patch.


> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 398f630..b468174 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -857,12 +857,15 @@ virDomainGraphicsListenDefClear(virDomainGraphicsListenDefPtr def)
>  }
>  
>  static void
> -virSecurityLabelDefClear(virSecurityLabelDefPtr def)
> +virSecurityLabelDefFree(virSecurityLabelDefPtr def)
>  {
> -    VIR_FREE(def->model);
> -    VIR_FREE(def->label);
> -    VIR_FREE(def->imagelabel);
> -    VIR_FREE(def->baselabel);
> +    if (def) {
> +        VIR_FREE(def->model);
> +        VIR_FREE(def->label);
> +        VIR_FREE(def->imagelabel);
> +        VIR_FREE(def->baselabel);
> +        VIR_FREE(def);
> +    }
>  }

We tend to write this a slightly different:
{
   if (!def)
     return;

   VIR_FREE(def->model);
   ...
}

> @@ -3669,10 +3679,15 @@ 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)) {

Long line.

> +            virReportOOMError();
> +            goto error;
> +        }
> +        if (virSecurityDeviceLabelDefParseXML(&def->seclabels[0],
>                                                vmSeclabel,
>                                                ctxt) < 0)
>              goto error;
> +        def->nseclabels = 1;
>          ctxt->node = saved_node;
>      }
>  
> @@ -7115,10 +7130,16 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps,
>          goto error;
>      }
>  
> +    if ((VIR_ALLOC(def->seclabels) < 0) ||
> +        (VIR_ALLOC(def->seclabels[0])) < 0 ) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +
>      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)))

Long line.

>              goto error;
>      } else if (xmlStrEqual(node->name, BAD_CAST "lease")) {
>          dev->type = VIR_DOMAIN_DEVICE_LEASE;
> @@ -8017,7 +8038,12 @@ 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;
> +    }

Again, I'd split this into two lines.

> +    def->nseclabels = 1;
> +    if (virSecurityLabelDefParseXML(def->seclabels[0], ctxt, flags) == -1)
>          goto error;
>  
>      /* Extract domain memory */

I think needs to be squashed in:

diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
index 06ff685..be9d295 100644
--- a/docs/schemas/capability.rng
+++ b/docs/schemas/capability.rng
@@ -52,12 +52,14 @@
 
   <define name='secmodel'>
     <element name='secmodel'>
-      <element name='model'>
-        <text/>
-      </element>
-      <element name='doi'>
-        <text/>
-      </element>
+      <oneOrMore>
+        <element name='model'>
+          <text/>
+        </element>
+        <element name='doi'>
+          <text/>
+        </element>
+      </oneOrMore>
     </element>
   </define>
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b468174..719efea 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -859,13 +859,14 @@ virDomainGraphicsListenDefClear(virDomainGraphicsListenDefPtr def)
 static void
 virSecurityLabelDefFree(virSecurityLabelDefPtr def)
 {
-    if (def) {
-        VIR_FREE(def->model);
-        VIR_FREE(def->label);
-        VIR_FREE(def->imagelabel);
-        VIR_FREE(def->baselabel);
-        VIR_FREE(def);
-    }
+    if (!def)
+        return;
+
+    VIR_FREE(def->model);
+    VIR_FREE(def->label);
+    VIR_FREE(def->imagelabel);
+    VIR_FREE(def->baselabel);
+    VIR_FREE(def);
 }
 
 
@@ -3679,7 +3680,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
     if (sourceNode) {
         xmlNodePtr saved_node = ctxt->node;
         ctxt->node = sourceNode;
-        if ((VIR_ALLOC(def->seclabels) < 0) || (VIR_ALLOC(def->seclabels[0]) < 0)) {
+        if ((VIR_ALLOC(def->seclabels) < 0) ||
+            (VIR_ALLOC(def->seclabels[0]) < 0)) {
             virReportOOMError();
             goto error;
         }
@@ -7138,8 +7140,9 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps,
 
     if (xmlStrEqual(node->name, BAD_CAST "disk")) {
         dev->type = VIR_DOMAIN_DEVICE_DISK;
-        if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt,
-                                                        NULL, def->seclabels[0], flags)))
+        if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt, NULL,
+                                                        def->seclabels[0],
+                                                        flags)))
             goto error;
     } else if (xmlStrEqual(node->name, BAD_CAST "lease")) {
         dev->type = VIR_DOMAIN_DEVICE_LEASE;
@@ -8038,7 +8041,8 @@ 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 ((VIR_ALLOC(def->seclabels) < 0) || (VIR_ALLOC(def->seclabels[0]) < 0)) {
+    if ((VIR_ALLOC(def->seclabels) < 0) ||
+        (VIR_ALLOC(def->seclabels[0]) < 0)) {
         virReportOOMError();
         goto error;
     }



But don't we rather want multiple <secmodel> elements than multiple <doi> and <model> inside one <secmodel>?


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