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

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

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