[PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

Michal Privoznik posted 1 patch 2 weeks ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1a935749004bcf3dd3eab3671ee13057eda82996.1593071091.git.mprivozn@redhat.com
src/qemu/qemu_domain.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

[PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

Posted by Michal Privoznik 2 weeks ago
A few commits back (in v6.4.0-131-gbdb8f2e418) the post parse
function for domain interface was changed so that it doesn't fill
in model for hostdev types of interfaces (including network type
interfaces which would end up hostdevs).

While the idea is sound, the execution can be a bit better:
virDomainNetResolveActualType() which is used to determine
runtime type of given interface is heavy gun - it connects to
network driver, fetches network XML, parses it. This all is
followed by check whether the interface doesn't already have
model set (from domain XML).

If we switch the order of these two checks then the short circuit
evaluation will ensure the expensive check is done only if really
needed.

This commit in fact fixes qemuxml2xmltest which due to lacking
fake network driver tries to connect to network:///session and
start the virtnetworkd. Fortunately, because of
v6.3.0-25-gf28fbb05d3 it fails to do so and
virDomainNetResolveActualType() returns -1. The only reason we
don't see the test failing is because our input XMLs have model
and thus we are saved by the latter (now former) check.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

While this patch is technically correct (the best way to be correct), it
can also be viewed as papering over the real issue. Question is, how to
address that. Nor xml2xml test nor xml2argv test are creating fake
network driver. Is mocking virDomainNetResolveActualType() the way to go
then? Or should we create the fake network driver with networks and
everything?

 src/qemu/qemu_domain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2efbf1a4b3..c5b8d91f9a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5230,8 +5230,8 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
                                 virQEMUCapsPtr qemuCaps)
 {
     if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
-        virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
-        !virDomainNetGetModelString(net))
+        !virDomainNetGetModelString(net) &&
+        virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV)
         net->model = qemuDomainDefaultNetModel(def, qemuCaps);
 
     return 0;
-- 
2.26.2

Re: [PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

Posted by Laine Stump 2 weeks ago
On 6/25/20 3:48 AM, Michal Privoznik wrote:
> A few commits back (in v6.4.0-131-gbdb8f2e418) the post parse
> function for domain interface was changed so that it doesn't fill
> in model for hostdev types of interfaces (including network type
> interfaces which would end up hostdevs).
>
> While the idea is sound, the execution can be a bit better:
> virDomainNetResolveActualType() which is used to determine
> runtime type of given interface is heavy gun - it connects to
> network driver, fetches network XML, parses it. This all is
> followed by check whether the interface doesn't already have
> model set (from domain XML).
>
> If we switch the order of these two checks then the short circuit
> evaluation will ensure the expensive check is done only if really
> needed.


Oops! I should have caught that when I reviewed the earlier commit.


Reviewed-by: Laine Stump <laine@redhat.com>


Re: [PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

Posted by Daniel P. Berrangé 2 weeks ago
On Thu, Jun 25, 2020 at 09:48:56AM +0200, Michal Privoznik wrote:
> A few commits back (in v6.4.0-131-gbdb8f2e418) the post parse
> function for domain interface was changed so that it doesn't fill
> in model for hostdev types of interfaces (including network type
> interfaces which would end up hostdevs).
> 
> While the idea is sound, the execution can be a bit better:
> virDomainNetResolveActualType() which is used to determine
> runtime type of given interface is heavy gun - it connects to
> network driver, fetches network XML, parses it. This all is
> followed by check whether the interface doesn't already have
> model set (from domain XML).
> 
> If we switch the order of these two checks then the short circuit
> evaluation will ensure the expensive check is done only if really
> needed.
> 
> This commit in fact fixes qemuxml2xmltest which due to lacking
> fake network driver tries to connect to network:///session and
> start the virtnetworkd. Fortunately, because of
> v6.3.0-25-gf28fbb05d3 it fails to do so and
> virDomainNetResolveActualType() returns -1. The only reason we
> don't see the test failing is because our input XMLs have model
> and thus we are saved by the latter (now former) check.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> While this patch is technically correct (the best way to be correct), it
> can also be viewed as papering over the real issue. Question is, how to
> address that. Nor xml2xml test nor xml2argv test are creating fake
> network driver. Is mocking virDomainNetResolveActualType() the way to go
> then? Or should we create the fake network driver with networks and
> everything?

The qemuxml2argtest calls virSetConnectNetwork() to provide a stub
network driver, likewise for other secondary drivers. This prevents
any risk of spawning daemons. We should probably do that for the
other tests too.

> 
>  src/qemu/qemu_domain.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2efbf1a4b3..c5b8d91f9a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5230,8 +5230,8 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
>                                  virQEMUCapsPtr qemuCaps)
>  {
>      if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> -        virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> -        !virDomainNetGetModelString(net))
> +        !virDomainNetGetModelString(net) &&
> +        virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV)
>          net->model = qemuDomainDefaultNetModel(def, qemuCaps);
>  
>      return 0;
> -- 
> 2.26.2
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

Posted by Michal Privoznik 2 weeks ago
On 6/25/20 10:38 AM, Daniel P. Berrangé wrote:
> On Thu, Jun 25, 2020 at 09:48:56AM +0200, Michal Privoznik wrote:
>> A few commits back (in v6.4.0-131-gbdb8f2e418) the post parse
>> function for domain interface was changed so that it doesn't fill
>> in model for hostdev types of interfaces (including network type
>> interfaces which would end up hostdevs).
>>
>> While the idea is sound, the execution can be a bit better:
>> virDomainNetResolveActualType() which is used to determine
>> runtime type of given interface is heavy gun - it connects to
>> network driver, fetches network XML, parses it. This all is
>> followed by check whether the interface doesn't already have
>> model set (from domain XML).
>>
>> If we switch the order of these two checks then the short circuit
>> evaluation will ensure the expensive check is done only if really
>> needed.
>>
>> This commit in fact fixes qemuxml2xmltest which due to lacking
>> fake network driver tries to connect to network:///session and
>> start the virtnetworkd. Fortunately, because of
>> v6.3.0-25-gf28fbb05d3 it fails to do so and
>> virDomainNetResolveActualType() returns -1. The only reason we
>> don't see the test failing is because our input XMLs have model
>> and thus we are saved by the latter (now former) check.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>
>> While this patch is technically correct (the best way to be correct), it
>> can also be viewed as papering over the real issue. Question is, how to
>> address that. Nor xml2xml test nor xml2argv test are creating fake
>> network driver. Is mocking virDomainNetResolveActualType() the way to go
>> then? Or should we create the fake network driver with networks and
>> everything?
> 
> The qemuxml2argtest calls virSetConnectNetwork() to provide a stub
> network driver, likewise for other secondary drivers. This prevents
> any risk of spawning daemons. We should probably do that for the
> other tests too.

What do you mean by "stub driver"? I can see conn->networkDriver = NULL. 
I mean, it works, but it's not a stub driver :-) Basically any 
virNetwork* call fails and we merely just let the code deal with it.

Anyway, I will post that as a follow up patch, but this opens an 
interesting question - every test that parses a domain XML will run post 
parse callbacks and thus potentially suffers from the same problem.
But that shouldn't block this patch because as I say, this can be viewed 
as performance improvement.

Michal

Re: [PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

Posted by Daniel P. Berrangé 2 weeks ago
On Thu, Jun 25, 2020 at 03:54:18PM +0200, Michal Privoznik wrote:
> On 6/25/20 10:38 AM, Daniel P. Berrangé wrote:
> > On Thu, Jun 25, 2020 at 09:48:56AM +0200, Michal Privoznik wrote:
> > > A few commits back (in v6.4.0-131-gbdb8f2e418) the post parse
> > > function for domain interface was changed so that it doesn't fill
> > > in model for hostdev types of interfaces (including network type
> > > interfaces which would end up hostdevs).
> > > 
> > > While the idea is sound, the execution can be a bit better:
> > > virDomainNetResolveActualType() which is used to determine
> > > runtime type of given interface is heavy gun - it connects to
> > > network driver, fetches network XML, parses it. This all is
> > > followed by check whether the interface doesn't already have
> > > model set (from domain XML).
> > > 
> > > If we switch the order of these two checks then the short circuit
> > > evaluation will ensure the expensive check is done only if really
> > > needed.
> > > 
> > > This commit in fact fixes qemuxml2xmltest which due to lacking
> > > fake network driver tries to connect to network:///session and
> > > start the virtnetworkd. Fortunately, because of
> > > v6.3.0-25-gf28fbb05d3 it fails to do so and
> > > virDomainNetResolveActualType() returns -1. The only reason we
> > > don't see the test failing is because our input XMLs have model
> > > and thus we are saved by the latter (now former) check.
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > ---
> > > 
> > > While this patch is technically correct (the best way to be correct), it
> > > can also be viewed as papering over the real issue. Question is, how to
> > > address that. Nor xml2xml test nor xml2argv test are creating fake
> > > network driver. Is mocking virDomainNetResolveActualType() the way to go
> > > then? Or should we create the fake network driver with networks and
> > > everything?
> > 
> > The qemuxml2argtest calls virSetConnectNetwork() to provide a stub
> > network driver, likewise for other secondary drivers. This prevents
> > any risk of spawning daemons. We should probably do that for the
> > other tests too.
> 
> What do you mean by "stub driver"? I can see conn->networkDriver = NULL. I
> mean, it works, but it's not a stub driver :-) Basically any virNetwork*
> call fails and we merely just let the code deal with it.

Yeah, by stub i mean something that prevents any network APIs from
working.

xs

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|