On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list