Re: [PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions
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 > > > --- > > > > > > 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 :|
Re: [PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions
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 --- 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
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
Re: [PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions
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
> ---
>
> 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 :|
