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@xxxxxxxxxx> > > --- > > 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
