Reject Xen VM config containing nwfilter references

[PATCH 2/2] libxl: Reject VM config referencing nwfilters

Posted by Jim Fehlig via Devel 1 year, 5 months ago

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,
but ignoring a user's request to filter VM network traffic can be viewed
as a security issue.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 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;
+    }
+
     if (dev->type == VIR_DOMAIN_DEVICE_CHR &&
         dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
         dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE &&
-- 
2.35.3

Re: [PATCH 2/2] libxl: Reject VM config referencing nwfilters

Posted by Laine Stump 1 year, 5 months ago

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@suse.com>
> ---
>   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)

>       if (dev->type == VIR_DOMAIN_DEVICE_CHR &&
>           dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
>           dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE &&

Re: [PATCH 2/2] libxl: Reject VM config referencing nwfilters

Posted by Peter Krempa 1 year, 4 months ago

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@suse.com>
> > ---
> >   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.

Re: [PATCH 2/2] libxl: Reject VM config referencing nwfilters

Posted by Jim Fehlig via Devel 1 year, 4 months ago

On 9/12/24 01:37, Peter Krempa wrote:
> 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@suse.com>
>>> ---
>>>    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.

Nod. I had already seen that in my testing. Not nice, but I was prepared to 
swallow the pill.

> 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.

The validate callback plus a similar check at VM start is much nicer! I'll add 
that to V2. Thanks!

Regards,
Jim

Re: [PATCH 2/2] libxl: Reject VM config referencing nwfilters

Posted by Peter Krempa 1 year, 4 months ago

On Thu, Sep 12, 2024 at 15:54:04 -0600, Jim Fehlig wrote:
> On 9/12/24 01:37, Peter Krempa wrote:
> > 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:

[...]

> > 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.
> 
> The validate callback plus a similar check at VM start is much nicer! I'll
> add that to V2. Thanks!

At startup time the code should simply re-call the validate
infrastructure. We do that in the qemu driver so that there's no
duplication.

I'm not sure whether this was done in libxl, but that would be the
proper solution.

Re: [PATCH 2/2] libxl: Reject VM config referencing nwfilters

Posted by Jim Fehlig via Devel 1 year, 5 months ago

On 9/11/24 16:24, 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@suse.com>
>> ---
>>   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,

Yes, good point! The libxl driver already has domainValidateCallback, but now 
needs a deviceValidateCallback for this code. I'll make that change in V2.

Before sending another version, I'd like to hear opinions on Demi's question 
about the other hypervisor drivers. Do they need a similar change?

Regards,
Jim

Re: [PATCH 2/2] libxl: Reject VM config referencing nwfilters

Posted by Demi Marie Obenour 1 year, 5 months ago

On Wed, Sep 11, 2024 at 05:09:03PM -0600, Jim Fehlig wrote:
> On 9/11/24 16:24, 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@suse.com>
> > > ---
> > >   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,
> 
> Yes, good point! The libxl driver already has domainValidateCallback, but
> now needs a deviceValidateCallback for this code. I'll make that change in
> V2.
> 
> Before sending another version, I'd like to hear opinions on Demi's question
> about the other hypervisor drivers. Do they need a similar change?

I'm not familiar with how libvirt works internally, but to me it seems
like one option would be to have a flag set on the drivers that support
network filtering.  Generic code would then check the flag and fail if
filtering is requested with a driver that doesn't support it.

This has the advantage of not requiring changes to each and every
driver.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH 2/2] libxl: Reject VM config referencing nwfilters

Posted by Laine Stump 1 year, 4 months ago

On 9/11/24 7:42 PM, Demi Marie Obenour wrote:
> On Wed, Sep 11, 2024 at 05:09:03PM -0600, Jim Fehlig wrote:
>> On 9/11/24 16:24, 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@suse.com>
>>>> ---
>>>>    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,
>>
>> Yes, good point! The libxl driver already has domainValidateCallback, but
>> now needs a deviceValidateCallback for this code. I'll make that change in
>> V2.
>>
>> Before sending another version, I'd like to hear opinions on Demi's question
>> about the other hypervisor drivers. Do they need a similar change?

Now that we're talking more about this, I'm having flashbacks of new 
features being added in, and the author (e.g. me) basically updating the 
hypervisor drivers they were personally interested in to support the 
feature (or else explicitly log an error), but leaving the other drivers 
untouched because "I can't test it, so I don't want to add code that 
could potentially end up failing when somebody actually *can* test it" 
(or some such other copout :-P) (e.g. in my case that used to be qemu 
and lxc, but for a long time has been only qemu).

> 
> I'm not familiar with how libvirt works internally, but to me it seems
> like one option would be to have a flag set on the drivers that support
> network filtering.  Generic code would then check the flag and fail if
> filtering is requested with a driver that doesn't support it.
> 
> This has the advantage of not requiring changes to each and every
> driver.

An interesting idea, but then we would really want to do that for every 
individual XML knob; essentially generic "capabilities flags" similar to 
the QEMU capabilities flags we use to determine whether a particular 
feature is available for a particular QEMU binary.

Re: [PATCH 2/2] libxl: Reject VM config referencing nwfilters

Posted by Demi Marie Obenour 1 year, 4 months ago

On Wed, Sep 11, 2024 at 09:16:41PM -0400, Laine Stump wrote:
> On 9/11/24 7:42 PM, Demi Marie Obenour wrote:
> > On Wed, Sep 11, 2024 at 05:09:03PM -0600, Jim Fehlig wrote:
> > > On 9/11/24 16:24, 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@suse.com>
> > > > > ---
> > > > >    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,
> > > 
> > > Yes, good point! The libxl driver already has domainValidateCallback, but
> > > now needs a deviceValidateCallback for this code. I'll make that change in
> > > V2.
> > > 
> > > Before sending another version, I'd like to hear opinions on Demi's question
> > > about the other hypervisor drivers. Do they need a similar change?
> 
> Now that we're talking more about this, I'm having flashbacks of new
> features being added in, and the author (e.g. me) basically updating the
> hypervisor drivers they were personally interested in to support the feature
> (or else explicitly log an error), but leaving the other drivers untouched
> because "I can't test it, so I don't want to add code that could potentially
> end up failing when somebody actually *can* test it" (or some such other
> copout :-P) (e.g. in my case that used to be qemu and lxc, but for a long
> time has been only qemu).

In this case I think this should be pretty low-risk, but I would leave
that to the authors of the other drivers to decide.  For what it is
worth, the only other driver I can think of for which filtering could
potentially be added is VirtualBox on Linux, unless libvirt wants to
interface with whatever Hyper-V, VMware ESXi, VMware on desktop, and
other platforms use for their filtering.

> > I'm not familiar with how libvirt works internally, but to me it seems
> > like one option would be to have a flag set on the drivers that support
> > network filtering.  Generic code would then check the flag and fail if
> > filtering is requested with a driver that doesn't support it.
> > 
> > This has the advantage of not requiring changes to each and every
> > driver.
> 
> An interesting idea, but then we would really want to do that for every
> individual XML knob; essentially generic "capabilities flags" similar to the
> QEMU capabilities flags we use to determine whether a particular feature is
> available for a particular QEMU binary.

This could be the first, but I also understand if this is out of scope
for the current change.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab