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 <mprivozn(a)redhat.com&gt; > > --- > > > > 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.