Re: [libvirt] [PATCH 5/7] Add two new security label types

On 01/11/2012 09:33 AM, Daniel P. Berrange wrote: s/introduce/introduction/ Fails 'make syntax-check': po_check --- ./po/POTFILES.in +++ ./po/POTFILES.in @@ -89,6 +89,7 @@ src/security/security_apparmor.c src/security/security_dac.c src/security/security_driver.c +src/security/security_manager.c src/security/security_selinux.c src/security/virt-aa-helper.c src/storage/parthelper.c maint.mk: you have changed the set of files with translatable diagnostics; apply the above patch Fails 'make check': seclabeltest.c: In function 'main': seclabeltest.c:16:5: error: too few arguments to function 'virSecurityManagerNew' ../src/security/security_manager.h:34:23: note: declared here I had to squash this in to get tests to pass: diff --git i/po/POTFILES.in w/po/POTFILES.in index ca1db70..9b699bd 100644 --- i/po/POTFILES.in +++ w/po/POTFILES.in @@ -89,6 +89,7 @@ src/secret/secret_driver.c src/security/security_apparmor.c src/security/security_dac.c src/security/security_driver.c +src/security/security_manager.c src/security/security_selinux.c src/security/virt-aa-helper.c src/storage/parthelper.c diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index 07080ce..91b95e2 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -9852,7 +9852,7 @@ virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def) virBufferAsprintf(buf, "<seclabel type='%s'", sectype); if (def->model) - virBufferEscapeString(buf, " model='%s' ", def->model); + virBufferEscapeString(buf, " model='%s'", def->model); virBufferAsprintf(buf, " relabel='%s'", def->norelabel ? "no" : "yes"); diff --git i/tests/seclabeltest.c w/tests/seclabeltest.c index 5d87789..fca76b9 100644 --- i/tests/seclabeltest.c +++ w/tests/seclabeltest.c @@ -13,7 +13,7 @@ main (int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) virSecurityManagerPtr mgr; const char *doi, *model; - mgr = virSecurityManagerNew(NULL, false); + mgr = virSecurityManagerNew(NULL, false, true, false); if (mgr == NULL) { fprintf (stderr, "Failed to start security driver"); exit (-1); You also need to list the new XML in docs/schemas/domaincommon.rng, document it in docs/formatdomain.html.in, and add tests for the new XML being parsed correctly. I don't feel comfortable acking with that much missing, so I'll need a v2; but I can proceed to give code review. This explicitly rejects type='default', was that intentional (where you _want_ the user to omit the attribute to get the default)? It matches what we do elsewhere, but that does mean that you have to be careful in documenting it this way (type='none|dynamic|static' or omitted type to form the four possibilities). Bad grammar in the error message, but even with that fixed, it still doesn't make sense compared to the if conditional. I think you meant something more like: "resource relabeling not compatible with 'none' label type" So we explicitly ignore any <label> sub-elements with type of none; I can live with that. I like that you are dropping dependency on the flags argument. I will note that this means that 'virsh save-image-edit' might end up changing a save image file created pre-patch into a new form post-patch even when there are no changes made by the user (but that's not the first time we've done that). But as long as we aren't changing the output from the input _from the same version of libvirt_, I'm okay if upgrading the libvirt version causes a rewrite when editing an older save image. The old way omitted output if you use the defaults, but still generated output if you do this as short-hand input: <seclabel> <baselabel>...</baselabel> </seclabel> for the intended: <seclabel type='dynamic' model='selinux' relabel='yes'> <baselabel>...</baselabel> </seclabel> ...whereas your new code fails to look for the presence of <baselabel>, and would silently lose the user's input. I argue that the bug is in your parse routine, and not your output routine. That is, if the user omitted type, but then provides a <baselabel>, you need to explicitly set type to dynamic rather than leaving it at default, so that the output routine can blindly omit output when type is default precisely because <baselabel> no longer maps to default. Lose the trailing space. Also, you can exploit the fact that virBufferEscapeString does a NULL check for you, and lose the 'if (def->model)' with no change in output. This can result in: <seclabel type='dynamic' model='selinux' relabel='yes'> </seclabel> for an inactive domain. Do we want to go to any effort to shorten this to: <seclabel type='dynamic' model='selinux' relabel='yes'/> Do we want the US spelling of labeling here? Consistency argues that we should use the digit rather than the name of the number, and end with a full stop, as in: Defaults to 1. Defaults to 0. (that is, both variable descriptions need a tweak, but for different reasons) Conversion of long to boolean. I guess this is copy-and-paste from other places doing this, and also that since we require C99 it will be safe. But in C89 projects, the gnulib replacement for <stdbool.h> means that this is non-portable, even though it should work just fine with C99 bool. (That is, the gnulib replacement bool can compile (bool)256L to false instead of the desired true, depending on your platform and compiler). No need to fix it in your patch (if we do anything, it should be a global style scrub of all offenders in this file in one go). Might be safer to put a default case that errors out, so that we catch any future additions. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org