On Wed, Jan 12, 2011 at 01:48:24PM -0700, Eric Blake wrote:
> 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@xxxxxxxxxx>
> > ---
>
> 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).
Even though the code is a bit longer, I think it is good
practice to use an ENUM for handling this.
> > +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',
Overall, this kind of state of affairs is OK. I'm fairly
sure we've other areas where the 'default' is silently
accepted in parse, but not output when formatting for
sake of brevity.
> 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
No, we really do want the default value to be 0. It is important
for defaults to be zero, so that when you VIR_ALLOC you get the
correct defaults automatically.
Regards,
Daniel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list