[PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

Michal Privoznik mprivozn at redhat.com
Thu Jun 25 13:54:18 UTC 2020


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




More information about the libvir-list mailing list