[libvirt] [PATCH 5/7] domain: Handle seclabel model with an enum

[Eric Blake]

On 01/12/2011 10:23 AM, Cole Robinson wrote:
> This allows us to explicitly handle the 'default' seclabel case, as
> well as provide easier model validation.
> 
> Signed-off-by: Cole Robinson <crobinso at redhat.com>
> ---

I'm not so sure about this one.  I didn't see any coding bugs in the
actual patch, but design-wise:


>  src/conf/domain_conf.c           |   38 ++++++++++++++++++++++++++++++--------
>  src/conf/domain_conf.h           |   14 ++++++++++++--
>  src/security/security_apparmor.c |    9 +++------
>  src/security/security_driver.c   |   15 ++++++++++-----
>  src/security/security_selinux.c  |    8 ++------
>  5 files changed, 57 insertions(+), 27 deletions(-)

1. it's already a net gain in lines of code

2. to me, it really seems like it will only be useful if we go with
patch 7/7 (which Daniel already NACK'd).

3. ...

> +VIR_ENUM_IMPL(virDomainSeclabelModel, VIR_DOMAIN_SECLABEL_MODEL_LAST,
> +              "default",
> +              "selinux",
> +              "apparmor",
> +              "none")

This means the FromString knows how to parse "default",

> @@ -4244,7 +4250,15 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def,
>                                   "%s", _("missing security model"));
>              goto error;
>          }
> -        def->seclabel.model = p;
> +
> +        def->seclabel.model = virDomainSeclabelModelTypeFromString(p);
> +        if (def->seclabel.model < 0) {
> +            virDomainReportError(VIR_ERR_XML_ERROR,
> +                                 _("unknown security model '%s'"), p);
> +            VIR_FREE(p);
> +            goto error;
> +        }

while this doesn't reject such a parse,

> @@ -7336,18 +7350,26 @@ char *virDomainDefFormat(virDomainDefPtr def,
>  
>      virBufferAddLit(&buf, "  </devices>\n");
>  
> -    if (def->seclabel.model) {
> -        const char *sectype = virDomainSeclabelTypeToString(def->seclabel.type);
> +    if (def->seclabel.model != VIR_DOMAIN_SECLABEL_MODEL_DEFAULT) {

yet this intentionally avoids outputting model='default',

and overall, you didn't modify the domain.rng or docs to mention the new
attribute value.  I'm assuming you didn't intend to extend the xml to
allow model='default', which means you either want:

>  
> +enum virDomainSeclabelModel {
> +    VIR_DOMAIN_SECLABEL_MODEL_DEFAULT,

this to be = -1,

> +    VIR_DOMAIN_SECLABEL_MODEL_SELINUX,

and this 0, and just omit "default" from the VIR_ENUM_IMPL; or that you
have to correct the parseXML function to reject an explicit
model='default' parse.  Since non-zero defaults have their own set of
interesting issues (you'd have to track down every place in code that
allocates a virDomainSeclabelDef and ensure that it gets initialized
correctly), I'd lean to the latter option, but both options add
complexity to this patch.

So unless we can come up with another reason why this is worth
including, I'm inclined to just omit this one.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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