On Wed, Sep 11, 2024 at 18:24:07 -0400, Laine Stump wrote:
> On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
> > The Xen libxl driver does not support nwfilter. Add a check for nwfilters
> > to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if
> > any are found.
> >
> > It's generally preferred for drivers to ignore unsupported XML features,
>
> I would instead characterize it as "drivers generally ignore *unrecognized*
> XML", but it's quite common for a bit of XML that's understood and supported
> in one context within libvirt to generate an UNSUPPORTED error when
> attempting to use it in a place where it isn't supported.
>
> > but ignoring a user's request to filter VM network traffic can be viewed
> > as a security issue.
>
> Definitely.
>
> >
> > Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> > ---
> > src/libxl/libxl_domain.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> > index 0f129ec69c..2f6cebb8ae 100644
> > --- a/src/libxl/libxl_domain.c
> > +++ b/src/libxl/libxl_domain.c
> > @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev,
> > void *opaque G_GNUC_UNUSED,
> > void *parseOpaque G_GNUC_UNUSED)
> > {
> > + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("filterref is not supported in %1$s"),
> > + virDomainVirtTypeToString(def->virtType));
> > + return -1;
> > + }
> > +
>
> This more properly should be in a function called
> libxlValidateDomainDeviceDef(), which would look something like
> qemuValidateDomainDeviceDef() and be added into libxlDomainDefParserConfig
> with this initialization:
>
> .deviceValidateCallback = libxlValidateDomainDeviceDef,
>
> (my understanding of the purpose of the two has always been that the
> PostParse callback is intended for performing post-parse
> modifications/fixups to the domain def, while the Validate only checks for
> correctness, without modifying anything. There are multiple cases of having
> validation in the postparse function, but I think those are the 1) leftovers
> from before the introduction of the validate callbacks, and 2) the result of
> misunderstanding and/or sloth (e.g. in cases where you want to validate
> something, but the driver you're adding this validation to doesn't already
> have a deviceValidateCallback)
The main difference between the two is that 'postParse' is called on
every parse of a XML. That means also for parsing the XMLs of
persistently defined domains.
If you reject parsing of a XML in a 'postParse' it fails to load the
persistent definition so the VM "vanishes", which is something we don't
want to do.
Thus doing validation in postParse is really valid only when it's a new
attribute or configuration that can't exist in "defined" state.
In contrast the validate callback is applied in a limited set of XML
entrypoints which then don't make the VM to vanish and certain other
situations. For that reason the 'Validate' callback/step needs to be
re-tried when starting the VM.
Other than that, what you've said is correct.
