[libvirt] [PATCH 0/9] Cleanup virConnectPtr usage
I've noticed this problem when reviewing a patch on the list [1]. Long story short, dom->conn is not guaranteed to have all driver pointers set (consider split daemons). I haven't identified any other violation than what I'm fixing here. But something might have slipped through my git grep. 1: https://www.redhat.com/archives/libvir-list/2019-December/msg00120.html Michal Prívozník (9): qemu_driver: Push qemuDomainInterfaceAddresses() a few lines down qemu: Don't use dom->conn to lookup virNetwork qemuGetDHCPInterfaces: Move some variables inside the loop qemuGetDHCPInterfaces: Switch to GLib libxl: Don't use dom->conn to lookup virNetwork libxlGetDHCPInterfaces: Move some variables inside the loop libxlGetDHCPInterfaces: Switch to GLib lxc: Cleanup virConnectPtr usage get_nonnull_domain: Drop useless comment src/libxl/libxl_driver.c | 71 ++++------ src/lxc/lxc_driver.c | 12 +- src/lxc/lxc_process.c | 40 +++--- src/lxc/lxc_process.h | 2 +- src/qemu/qemu_driver.c | 193 ++++++++++++---------------- src/remote/remote_daemon_dispatch.c | 3 - 6 files changed, 139 insertions(+), 182 deletions(-) -- 2.23.0
If we place qemuDomainInterfaceAddresses() a few lines below the two functions its using then we can drop forward declarations of those functions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 125 ++++++++++++++++++++--------------------- 1 file changed, 60 insertions(+), 65 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1911073f3e..1b9d6d16a1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -151,13 +151,6 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, const char *path, int oflags, bool *needUnlink); -static int qemuGetDHCPInterfaces(virDomainPtr dom, - virDomainObjPtr vm, - virDomainInterfacePtr **ifaces); - -static int qemuARPGetInterfaces(virDomainObjPtr vm, - virDomainInterfacePtr **ifaces); - static virQEMUDriverPtr qemu_driver; /* Looks up the domain object from snapshot and unlocks the @@ -21554,64 +21547,6 @@ qemuDomainGetFSInfo(virDomainPtr dom, return ret; } -static int -qemuDomainInterfaceAddresses(virDomainPtr dom, - virDomainInterfacePtr **ifaces, - unsigned int source, - unsigned int flags) -{ - virQEMUDriverPtr driver = dom->conn->privateData; - virDomainObjPtr vm = NULL; - qemuAgentPtr agent; - int ret = -1; - - virCheckFlags(0, -1); - - if (!(vm = qemuDomainObjFromDomain(dom))) - goto cleanup; - - if (virDomainInterfaceAddressesEnsureACL(dom->conn, vm->def) < 0) - goto cleanup; - - if (virDomainObjCheckActive(vm) < 0) - goto cleanup; - - switch (source) { - case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE: - ret = qemuGetDHCPInterfaces(dom, vm, ifaces); - break; - - case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT: - if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) - goto cleanup; - - if (!qemuDomainAgentAvailable(vm, true)) - goto endjob; - - agent = qemuDomainObjEnterAgent(vm); - ret = qemuAgentGetInterfaces(agent, ifaces); - qemuDomainObjExitAgent(vm, agent); - - endjob: - qemuDomainObjEndAgentJob(vm); - - break; - - case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_ARP: - ret = qemuARPGetInterfaces(vm, ifaces); - break; - - default: - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, - _("Unknown IP address data source %d"), - source); - break; - } - - cleanup: - virDomainObjEndAPI(&vm); - return ret; -} static int qemuGetDHCPInterfaces(virDomainPtr dom, @@ -21764,6 +21699,66 @@ qemuARPGetInterfaces(virDomainObjPtr vm, } +static int +qemuDomainInterfaceAddresses(virDomainPtr dom, + virDomainInterfacePtr **ifaces, + unsigned int source, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + qemuAgentPtr agent; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = qemuDomainObjFromDomain(dom))) + goto cleanup; + + if (virDomainInterfaceAddressesEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + switch (source) { + case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE: + ret = qemuGetDHCPInterfaces(dom, vm, ifaces); + break; + + case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT: + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) + goto cleanup; + + if (!qemuDomainAgentAvailable(vm, true)) + goto endjob; + + agent = qemuDomainObjEnterAgent(vm); + ret = qemuAgentGetInterfaces(agent, ifaces); + qemuDomainObjExitAgent(vm, agent); + + endjob: + qemuDomainObjEndAgentJob(vm); + + break; + + case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_ARP: + ret = qemuARPGetInterfaces(vm, ifaces); + break; + + default: + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("Unknown IP address data source %d"), + source); + break; + } + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + + static int qemuDomainSetUserPassword(virDomainPtr dom, const char *user, -- 2.23.0
When using the monolithic daemon, then dom->conn has all driver tables filled in properly and thus it's safe to call an API other than virDomain*(). However, when using split daemons then dom->conn has only hypervisor driver table set (dom->conn->driver) and the rest is NULL. Therefore, if we want to call a non-domain API (virNetworkLookupByName() in this case), we have obtain the cached connection object accessible via virGetConnectNetwork(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1b9d6d16a1..e5be74c461 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21549,10 +21549,10 @@ qemuDomainGetFSInfo(virDomainPtr dom, static int -qemuGetDHCPInterfaces(virDomainPtr dom, - virDomainObjPtr vm, +qemuGetDHCPInterfaces(virDomainObjPtr vm, virDomainInterfacePtr **ifaces) { + g_autoptr(virConnect) conn = NULL; int rv = -1; int n_leases = 0; size_t i, j; @@ -21563,21 +21563,20 @@ qemuGetDHCPInterfaces(virDomainPtr dom, virNetworkDHCPLeasePtr *leases = NULL; virDomainInterfacePtr *ifaces_ret = NULL; - if (!dom->conn->networkDriver || - !dom->conn->networkDriver->networkGetDHCPLeases) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Network driver does not support DHCP lease query")); + if (!(conn = virGetConnectNetwork())) return -1; - } for (i = 0; i < vm->def->nnets; i++) { if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue; virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); + virObjectUnref(network); - network = virNetworkLookupByName(dom->conn, + network = virNetworkLookupByName(conn, vm->def->nets[i]->data.network.name); + if (!network) + goto error; if ((n_leases = virNetworkGetDHCPLeases(network, macaddr, &leases, 0)) < 0) @@ -21723,7 +21722,7 @@ qemuDomainInterfaceAddresses(virDomainPtr dom, switch (source) { case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE: - ret = qemuGetDHCPInterfaces(dom, vm, ifaces); + ret = qemuGetDHCPInterfaces(vm, ifaces); break; case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT: -- 2.23.0
Some variables are not used outside of the for() loop. Move their declaration to clean up the code a bit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e5be74c461..2bc2081d0b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21553,26 +21553,27 @@ qemuGetDHCPInterfaces(virDomainObjPtr vm, virDomainInterfacePtr **ifaces) { g_autoptr(virConnect) conn = NULL; + virDomainInterfacePtr *ifaces_ret = NULL; + size_t ifaces_count = 0; int rv = -1; int n_leases = 0; - size_t i, j; - size_t ifaces_count = 0; - g_autoptr(virNetwork) network = NULL; - char macaddr[VIR_MAC_STRING_BUFLEN]; - virDomainInterfacePtr iface = NULL; virNetworkDHCPLeasePtr *leases = NULL; - virDomainInterfacePtr *ifaces_ret = NULL; + size_t i; if (!(conn = virGetConnectNetwork())) return -1; for (i = 0; i < vm->def->nnets; i++) { + g_autoptr(virNetwork) network = NULL; + char macaddr[VIR_MAC_STRING_BUFLEN]; + virDomainInterfacePtr iface = NULL; + size_t j; + if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue; virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); - virObjectUnref(network); network = virNetworkLookupByName(conn, vm->def->nets[i]->data.network.name); if (!network) -- 2.23.0
If we use glib alloc functions, we can drop the 'cleanup' label and @rv variable and also simplify the code a bit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 40 +++++++++++----------------------------- 1 file changed, 11 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2bc2081d0b..2d2a83cd8b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21555,9 +21555,6 @@ qemuGetDHCPInterfaces(virDomainObjPtr vm, g_autoptr(virConnect) conn = NULL; virDomainInterfacePtr *ifaces_ret = NULL; size_t ifaces_count = 0; - int rv = -1; - int n_leases = 0; - virNetworkDHCPLeasePtr *leases = NULL; size_t i; if (!(conn = virGetConnectNetwork())) @@ -21566,6 +21563,8 @@ qemuGetDHCPInterfaces(virDomainObjPtr vm, for (i = 0; i < vm->def->nnets; i++) { g_autoptr(virNetwork) network = NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; + virNetworkDHCPLeasePtr *leases = NULL; + int n_leases = 0; virDomainInterfacePtr iface = NULL; size_t j; @@ -21584,21 +21583,15 @@ qemuGetDHCPInterfaces(virDomainObjPtr vm, goto error; if (n_leases) { - if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) - goto error; - - if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) - goto error; + ifaces_ret = g_renew(typeof(*ifaces_ret), ifaces_ret, ifaces_count + 1); + ifaces_ret[ifaces_count] = g_new0(typeof(**ifaces_ret), 1); + iface = ifaces_ret[ifaces_count]; + ifaces_count++; - iface = ifaces_ret[ifaces_count - 1]; /* Assuming each lease corresponds to a separate IP */ iface->naddrs = n_leases; - - if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0) - goto error; - + iface->addrs = g_new0(typeof(*iface->addrs), iface->naddrs); iface->name = g_strdup(vm->def->nets[i]->ifname); - iface->hwaddr = g_strdup(macaddr); } @@ -21607,28 +21600,17 @@ qemuGetDHCPInterfaces(virDomainObjPtr vm, virDomainIPAddressPtr ip_addr = &iface->addrs[j]; ip_addr->addr = g_strdup(lease->ipaddr); - ip_addr->type = lease->type; ip_addr->prefix = lease->prefix; - } - for (j = 0; j < n_leases; j++) - virNetworkDHCPLeaseFree(leases[j]); + virNetworkDHCPLeaseFree(lease); + } VIR_FREE(leases); } *ifaces = g_steal_pointer(&ifaces_ret); - rv = ifaces_count; - - cleanup: - if (leases) { - for (i = 0; i < n_leases; i++) - virNetworkDHCPLeaseFree(leases[i]); - } - VIR_FREE(leases); - - return rv; + return ifaces_count; error: if (ifaces_ret) { @@ -21637,7 +21619,7 @@ qemuGetDHCPInterfaces(virDomainObjPtr vm, } VIR_FREE(ifaces_ret); - goto cleanup; + return -1; } -- 2.23.0
When using the monolithic daemon, then dom->conn has all driver tables filled in properly and thus it's safe to call an API other than virDomain*(). However, when using split daemons then dom->conn has only hypervisor driver table set (dom->conn->driver) and the rest is NULL. Therefore, if we want to call a non-domain API (virNetworkLookupByName() in this case), we have obtain the cached connection object accessible via virGetConnectNetwork(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/libxl_driver.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 44a74e8779..ccef4f3955 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -6235,10 +6235,10 @@ static int libxlNodeGetSecurityModel(virConnectPtr conn, } static int -libxlGetDHCPInterfaces(virDomainPtr dom, - virDomainObjPtr vm, +libxlGetDHCPInterfaces(virDomainObjPtr vm, virDomainInterfacePtr **ifaces) { + g_autoptr(virConnect) conn = NULL; int rv = -1; int n_leases = 0; size_t i, j; @@ -6249,12 +6249,8 @@ libxlGetDHCPInterfaces(virDomainPtr dom, virNetworkDHCPLeasePtr *leases = NULL; virDomainInterfacePtr *ifaces_ret = NULL; - if (!dom->conn->networkDriver || - !dom->conn->networkDriver->networkGetDHCPLeases) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Network driver does not support DHCP lease query")); + if (!(conn = virGetConnectNetwork())) return -1; - } for (i = 0; i < vm->def->nnets; i++) { if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) @@ -6262,8 +6258,10 @@ libxlGetDHCPInterfaces(virDomainPtr dom, virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); virObjectUnref(network); - network = virNetworkLookupByName(dom->conn, + network = virNetworkLookupByName(conn, vm->def->nets[i]->data.network.name); + if (!network) + goto error; if ((n_leases = virNetworkGetDHCPLeases(network, macaddr, &leases, 0)) < 0) @@ -6351,7 +6349,7 @@ libxlDomainInterfaceAddresses(virDomainPtr dom, switch (source) { case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE: - ret = libxlGetDHCPInterfaces(dom, vm, ifaces); + ret = libxlGetDHCPInterfaces(vm, ifaces); break; default: -- 2.23.0
Some variables are not used outside of the for() loop. Move their declaration to clean up the code a bit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/libxl_driver.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index ccef4f3955..6064436681 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -6239,25 +6239,27 @@ libxlGetDHCPInterfaces(virDomainObjPtr vm, virDomainInterfacePtr **ifaces) { g_autoptr(virConnect) conn = NULL; + virDomainInterfacePtr *ifaces_ret = NULL; + size_t ifaces_count = 0; int rv = -1; int n_leases = 0; - size_t i, j; - size_t ifaces_count = 0; - virNetworkPtr network = NULL; - char macaddr[VIR_MAC_STRING_BUFLEN]; - virDomainInterfacePtr iface = NULL; virNetworkDHCPLeasePtr *leases = NULL; - virDomainInterfacePtr *ifaces_ret = NULL; + size_t i; if (!(conn = virGetConnectNetwork())) return -1; for (i = 0; i < vm->def->nnets; i++) { + g_autoptr(virNetwork) network = NULL; + char macaddr[VIR_MAC_STRING_BUFLEN]; + virDomainInterfacePtr iface = NULL; + size_t j; + if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue; virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); - virObjectUnref(network); + network = virNetworkLookupByName(conn, vm->def->nets[i]->data.network.name); if (!network) -- 2.23.0
If we use glib alloc functions, we can drop the 'cleanup' label and @rv variable and also simplify the code a bit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/libxl_driver.c | 41 +++++++++++----------------------------- 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 6064436681..7467111618 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -6241,9 +6241,6 @@ libxlGetDHCPInterfaces(virDomainObjPtr vm, g_autoptr(virConnect) conn = NULL; virDomainInterfacePtr *ifaces_ret = NULL; size_t ifaces_count = 0; - int rv = -1; - int n_leases = 0; - virNetworkDHCPLeasePtr *leases = NULL; size_t i; if (!(conn = virGetConnectNetwork())) @@ -6252,6 +6249,8 @@ libxlGetDHCPInterfaces(virDomainObjPtr vm, for (i = 0; i < vm->def->nnets; i++) { g_autoptr(virNetwork) network = NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; + virNetworkDHCPLeasePtr *leases = NULL; + int n_leases = 0; virDomainInterfacePtr iface = NULL; size_t j; @@ -6270,21 +6269,16 @@ libxlGetDHCPInterfaces(virDomainObjPtr vm, goto error; if (n_leases) { - if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) - goto error; + ifaces_ret = g_renew(typeof(*ifaces_ret), ifaces_ret, ifaces_count + 1); + ifaces_ret[ifaces_count] = g_new0(typeof(**ifaces_ret), 1); + iface = ifaces_ret[ifaces_count]; + ifaces_count++; - if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) - goto error; - - iface = ifaces_ret[ifaces_count - 1]; /* Assuming each lease corresponds to a separate IP */ iface->naddrs = n_leases; - if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0) - goto error; - + iface->addrs = g_new0(typeof(*iface->addrs), iface->naddrs); iface->name = g_strdup(vm->def->nets[i]->ifname); - iface->hwaddr = g_strdup(macaddr); } @@ -6293,30 +6287,17 @@ libxlGetDHCPInterfaces(virDomainObjPtr vm, virDomainIPAddressPtr ip_addr = &iface->addrs[j]; ip_addr->addr = g_strdup(lease->ipaddr); - ip_addr->type = lease->type; ip_addr->prefix = lease->prefix; - } - for (j = 0; j < n_leases; j++) virNetworkDHCPLeaseFree(leases[j]); + } VIR_FREE(leases); } - *ifaces = ifaces_ret; - ifaces_ret = NULL; - rv = ifaces_count; - - cleanup: - virObjectUnref(network); - if (leases) { - for (i = 0; i < n_leases; i++) - virNetworkDHCPLeaseFree(leases[i]); - } - VIR_FREE(leases); - - return rv; + *ifaces = g_steal_pointer(&ifaces_ret); + return ifaces_count; error: if (ifaces_ret) { @@ -6325,7 +6306,7 @@ libxlGetDHCPInterfaces(virDomainObjPtr vm, } VIR_FREE(ifaces_ret); - goto cleanup; + return -1; } -- 2.23.0
There are some functions which pass virConnectPtr around for one reason and one reason only: to obtain virLXCDriverPtr in the end. Might replace the argument and pass a pointer to the driver right from the start. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_driver.c | 12 +++++------- src/lxc/lxc_process.c | 40 ++++++++++++++++++++++------------------ src/lxc/lxc_process.h | 2 +- 3 files changed, 28 insertions(+), 26 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 826bf074e3..e4be45fca2 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3803,9 +3803,8 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, } -/* XXX conn required for network -> bridge resolution */ static int -lxcDomainAttachDeviceNetLive(virConnectPtr conn, +lxcDomainAttachDeviceNetLive(virLXCDriverPtr driver, virDomainObjPtr vm, virDomainNetDefPtr net) { @@ -3866,7 +3865,7 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, goto cleanup; break; case VIR_DOMAIN_NET_TYPE_DIRECT: { - if (!(veth = virLXCProcessSetupInterfaceDirect(conn, vm->def, net))) + if (!(veth = virLXCProcessSetupInterfaceDirect(driver, vm->def, net))) goto cleanup; } break; case VIR_DOMAIN_NET_TYPE_USER: @@ -4233,8 +4232,7 @@ lxcDomainAttachDeviceHostdevLive(virLXCDriverPtr driver, static int -lxcDomainAttachDeviceLive(virConnectPtr conn, - virLXCDriverPtr driver, +lxcDomainAttachDeviceLive(virLXCDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { @@ -4248,7 +4246,7 @@ lxcDomainAttachDeviceLive(virConnectPtr conn, break; case VIR_DOMAIN_DEVICE_NET: - ret = lxcDomainAttachDeviceNetLive(conn, vm, + ret = lxcDomainAttachDeviceNetLive(driver, vm, dev->data.net); if (!ret) dev->data.net = NULL; @@ -4736,7 +4734,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, true) < 0) goto endjob; - if ((ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy)) < 0) + if ((ret = lxcDomainAttachDeviceLive(driver, vm, dev_copy)) < 0) goto endjob; /* * update domain status forcibly because the domain status may be diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 9b62a93096..f5d2fb0145 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -326,13 +326,13 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm, } -char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, - virDomainDefPtr def, - virDomainNetDefPtr net) +char * +virLXCProcessSetupInterfaceDirect(virLXCDriverPtr driver, + virDomainDefPtr def, + virDomainNetDefPtr net) { char *ret = NULL; char *res_ifname = NULL; - virLXCDriverPtr driver = conn->privateData; const virNetDevBandwidth *bw; const virNetDevVPortProfile *prof; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); @@ -392,9 +392,10 @@ static const char *nsInfoLocal[VIR_LXC_DOMAIN_NAMESPACE_LAST] = { [VIR_LXC_DOMAIN_NAMESPACE_SHAREUTS] = "uts", }; -static int virLXCProcessSetupNamespaceName(virConnectPtr conn, int ns_type, const char *name) +static int virLXCProcessSetupNamespaceName(virLXCDriverPtr driver, + int ns_type, + const char *name) { - virLXCDriverPtr driver = conn->privateData; int fd = -1; virDomainObjPtr vm; virLXCDomainObjPrivatePtr priv; @@ -474,7 +475,7 @@ static int virLXCProcessSetupNamespaceNet(int ns_type, const char *name) /** * virLXCProcessSetupNamespaces: - * @conn: pointer to connection + * @driver: pointer to driver structure * @def: pointer to virtual machines namespaceData * @nsFDs: out parameter to store the namespace FD * @@ -483,9 +484,10 @@ static int virLXCProcessSetupNamespaceNet(int ns_type, const char *name) * * Returns 0 on success or -1 in case of error */ -static int virLXCProcessSetupNamespaces(virConnectPtr conn, - lxcDomainDefPtr lxcDef, - int *nsFDs) +static int +virLXCProcessSetupNamespaces(virLXCDriverPtr driver, + lxcDomainDefPtr lxcDef, + int *nsFDs) { size_t i; @@ -500,7 +502,8 @@ static int virLXCProcessSetupNamespaces(virConnectPtr conn, case VIR_LXC_DOMAIN_NAMESPACE_SOURCE_NONE: continue; case VIR_LXC_DOMAIN_NAMESPACE_SOURCE_NAME: - if ((nsFDs[i] = virLXCProcessSetupNamespaceName(conn, i, lxcDef->ns_val[i])) < 0) + if ((nsFDs[i] = virLXCProcessSetupNamespaceName(driver, i, + lxcDef->ns_val[i])) < 0) return -1; break; case VIR_LXC_DOMAIN_NAMESPACE_SOURCE_PID: @@ -519,7 +522,7 @@ static int virLXCProcessSetupNamespaces(virConnectPtr conn, /** * virLXCProcessSetupInterfaces: - * @conn: pointer to connection + * @driver: pointer to driver structure * @def: pointer to virtual machine structure * @veths: string list of interface names * @@ -529,9 +532,10 @@ static int virLXCProcessSetupNamespaces(virConnectPtr conn, * * Returns 0 on success or -1 in case of error */ -static int virLXCProcessSetupInterfaces(virConnectPtr conn, - virDomainDefPtr def, - char ***veths) +static int +virLXCProcessSetupInterfaces(virLXCDriverPtr driver, + virDomainDefPtr def, + char ***veths) { int ret = -1; size_t i; @@ -585,7 +589,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, goto cleanup; break; case VIR_DOMAIN_NET_TYPE_DIRECT: - if (!(veth = virLXCProcessSetupInterfaceDirect(conn, def, net))) + if (!(veth = virLXCProcessSetupInterfaceDirect(driver, def, net))) goto cleanup; break; @@ -1344,11 +1348,11 @@ int virLXCProcessStart(virConnectPtr conn, } VIR_DEBUG("Setting up Interfaces"); - if (virLXCProcessSetupInterfaces(conn, vm->def, &veths) < 0) + if (virLXCProcessSetupInterfaces(driver, vm->def, &veths) < 0) goto cleanup; VIR_DEBUG("Setting up namespaces if any"); - if (virLXCProcessSetupNamespaces(conn, vm->def->namespaceData, nsInheritFDs) < 0) + if (virLXCProcessSetupNamespaces(driver, vm->def->namespaceData, nsInheritFDs) < 0) goto cleanup; VIR_DEBUG("Preparing to launch"); diff --git a/src/lxc/lxc_process.h b/src/lxc/lxc_process.h index 1bf359b229..383f6f714d 100644 --- a/src/lxc/lxc_process.h +++ b/src/lxc/lxc_process.h @@ -50,6 +50,6 @@ int virLXCProcessValidateInterface(virDomainNetDefPtr net); char *virLXCProcessSetupInterfaceTap(virDomainDefPtr vm, virDomainNetDefPtr net, const char *brname); -char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, +char *virLXCProcessSetupInterfaceDirect(virLXCDriverPtr driver, virDomainDefPtr def, virDomainNetDefPtr net); -- 2.23.0
The intent of get_nonnull_domain() is not to validate virDomain as sent by the client but just to construct the virDomain structure. The validation is then done in each API when looking up the domain in our internal hash tables. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_daemon_dispatch.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index f369ffb02a..a96e008242 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -7240,9 +7240,6 @@ remoteDispatchNetworkPortGetParameters(virNetServerPtr server G_GNUC_UNUSED, static virDomainPtr get_nonnull_domain(virConnectPtr conn, remote_nonnull_domain domain) { - /* Should we believe the domain.id sent by the client? Maybe - * this should be a check rather than an assignment? XXX - */ return virGetDomain(conn, domain.name, BAD_CAST domain.uuid, domain.id); } -- 2.23.0
On 12/4/19 4:55 AM, Michal Privoznik wrote:
I've noticed this problem when reviewing a patch on the list [1].
Long story short, dom->conn is not guaranteed to have all driver pointers set (consider split daemons). I haven't identified any other violation than what I'm fixing here. But something might have slipped through my git grep.
1: https://www.redhat.com/archives/libvir-list/2019-December/msg00120.html
Michal Prívozník (9): qemu_driver: Push qemuDomainInterfaceAddresses() a few lines down qemu: Don't use dom->conn to lookup virNetwork qemuGetDHCPInterfaces: Move some variables inside the loop qemuGetDHCPInterfaces: Switch to GLib libxl: Don't use dom->conn to lookup virNetwork libxlGetDHCPInterfaces: Move some variables inside the loop libxlGetDHCPInterfaces: Switch to GLib lxc: Cleanup virConnectPtr usage get_nonnull_domain: Drop useless comment
src/libxl/libxl_driver.c | 71 ++++------ src/lxc/lxc_driver.c | 12 +- src/lxc/lxc_process.c | 40 +++--- src/lxc/lxc_process.h | 2 +- src/qemu/qemu_driver.c | 193 ++++++++++++---------------- src/remote/remote_daemon_dispatch.c | 3 - 6 files changed, 139 insertions(+), 182 deletions(-)
Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
participants (2)
-
Cole Robinson -
Michal Privoznik