Re: [libvirt] [PATCH 4/7] Re-add domain device seclabel parsing / formatting

23 Jan 2012


      On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
...
From: "Daniel P. Berrange" <berrange@redhat.com>
This re-introduces parsing & formatting for per device seclabels.
There is a new virDomainDeviceSeclabelPtr struct and corresponding
APIs for parsing/formatting.
---
 src/conf/domain_conf.c |  115 ++++++++++++++++++++++++++++++++++++++++++++++--
 src/conf/domain_conf.h |   12 +++++-
 2 files changed, 122 insertions(+), 5 deletions(-)
Complex enough that I agree with your decision to separate the revert
from the new implementation, even if it means a potential for a failed
'git bisect'.
...
+static int
+virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr *def,
+                                  virSecurityLabelDefPtr vmDef,
+                                  xmlXPathContextPtr ctxt)
+{
+    char *p;
+
+    *def = NULL;
+
+    if (virXPathNode("./seclabel", ctxt) == NULL)
+        return 0;
Missing a check here that we aren't trying to override when the
top-level seclabel forbids it; something to replace this hunk removed in
3/7:

-    /* Can't use overrides if top-level doesn't allow relabeling.  */
-    if (default_seclabel && default_seclabel->norelabel) {
-        virDomainReportError(VIR_ERR_XML_ERROR, "%s",
-                             _("label overrides require relabeling to be "
-                               "enabled at the domain level"));
-            goto cleanup;

except that it would now be looking at 'vmDef->norelabel' instead of
'default_seclabel && default_seclabel->norelabel'.
...
+
+    if (VIR_ALLOC(*def) < 0) {
+        virReportOOMError();
+        return -1;
+    }
+
+    p = virXPathStringLimit("string(./seclabel/@relabel)",
+                            VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
+    if (p != NULL) {
+        if (STREQ(p, "yes")) {
+            (*def)->norelabel = false;
+        } else if (STREQ(p, "no")) {
+            (*def)->norelabel = true;
+        } else {
+            virDomainReportError(VIR_ERR_XML_ERROR,
+                                 _("invalid security relabel value %s"), p);
+            VIR_FREE(p);
+            VIR_FREE(*def);
+            return -1;
+        }
+        VIR_FREE(p);
+    } else {
+        if (vmDef->type == VIR_DOMAIN_SECLABEL_STATIC)
+            (*def)->norelabel = true;
+        else
+            (*def)->norelabel = false;
The code in 3/7 was defaulting (*def)->norelabel to false, regardless of
the top-level setting (that is, the only way to get a device label to
set norelabel is with an explicit attribute, so that a device label of:

<seclabel>
  <label>...</label>
</seclabel>

is then valid shorthand for:

<seclabel relabel='yes'>
  <label>...</label>
</seclabel>
...
+    }
+
+    p = virXPathStringLimit("string(./seclabel/label[1])",
+                            VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
+    (*def)->label = p;
Do we want to be saving this label if norelabel is true (that is, if the
user passes:

<seclabel relabel='no'>
  <label>...</label>
</seclabel>

do we reject that as invalid, or silently ignore the useless <label>,
instead of your behavior of accepting it)?
...
@@ -9767,6 +9844,19 @@ cleanup:
 }
+static void
+virSecurityDeviceLabelDefFormat(virBufferPtr buf,
+                                virSecurityDeviceLabelDefPtr def)
+{
+    virBufferAsprintf(buf, "<seclabel relabel='%s'>\n",
+                      def->norelabel ? "no" : "yes");
+    if (def->label)
+        virBufferEscapeString(buf, "  <label>%s</label>\n",
+                              def->label);
+    virBufferAddLit(buf, "</seclabel>\n");
With norelabel, this would generate the odd-looking:

<seclabel relabel='no'>
</seclabel>

How about instead doing:

virBufferAsprintf(buf, "<seclabel relabel='%s'",
                  def->norelabel ? "no" : "yes");
if (def->label) {
    virBufferAddLit(buf, ">\n");
    virBufferEscapeString(buf, "  <label>%s</label>\n",
                          def->label);
    virBufferAddLit(buf, "</seclabel>\n");
} else {
    virBufferAddLit(buf, "/>\n");
}

Close, but I'd feel a bit more comfortable seeing a v2.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

    