Re: [libvirt] [PATCH 4/7] Re-add domain device seclabel parsing / formatting
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)