[libvirt] [PATCH v3 02/36] network: pass a virNetworkPtr to port management APIs

[Daniel P. Berrangé]

On Thu, Mar 21, 2019 at 08:49:38PM -0400, Cole Robinson wrote:
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> > The APIs for allocating/notifying/removing network ports just take
> > an internal domain interface struct right now. As a step towards
> > turning these into public facing APIs, add a virNetworkPtr argument
> > to all of them.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > ---
> >  src/conf/domain_conf.c      | 40 ++++++++++++++++++++----
> >  src/conf/domain_conf.h      | 18 +++++++----
> >  src/libxl/libxl_domain.c    | 30 +++++++++++++-----
> >  src/libxl/libxl_driver.c    | 26 +++++++++++-----
> >  src/lxc/lxc_driver.c        | 24 +++++++++++---
> >  src/lxc/lxc_process.c       | 24 +++++++++-----
> >  src/network/bridge_driver.c | 54 ++++++++++++++++++--------------
> >  src/qemu/qemu_hotplug.c     | 62 +++++++++++++++++++++++++++----------
> >  src/qemu/qemu_process.c     | 30 +++++++++++++-----
> >  9 files changed, 223 insertions(+), 85 deletions(-)
> >
> 
> Like I mentioned in patch #1, it seems like we could move the
> virConnectPtr conn = virGetConnectNetwork() into the domain_conf.c
> functions. virDomainNetResolveActualType in domain_conf.c already does
> similar.
> 
> The only reason I can think of is that it saves double opening the
> connection in some error paths but that doesn't seem to be enough to
> justify the nastyness of sprinkling these calls everywhere

Avoiding opening the connection many times is exactly the reason.
It gets very inefficient when having many NICs. I don't think
this code is nasty as it is & we've done much the same for the
other drivers we've already split.

> Also I'd suggest naming the 'conn' variable consistently 'netconn' if
> only to make it more clear what driver we are talking about.
> 
> One other side bit: virGetConnectNetwork() calls virConnectOpen() which
> is a public API, complete with calling ResetLastError and DispatchError.
> That seems wrong? Seems like it should call an internal function that
> doesn't skips those calls. Not the fault of this patch series though

Hmm, applies to all the other helpers too. Will have to think about
fixing that.


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