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

[Eric Blake]

On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at 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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120123/6a2ef4ec/attachment-0001.sig>