[libvirt] [PATCH] network: don't even call networkRunHook if there is no network
networkAllocateActualDevice() is called for *all* interfaces, not just those with type='network'. In that case, it will jump down to its validate: label immediately, without allocating anything. After validation is done, two counters are potentially updated (one for the network, and one for any particular physical device that is chosen), and then networkRunHook() is called. This patch refactors that code a slight bit so that networkRunHook() doesn't get called if netdef is NULL (i.e. type != network) and to place the conditional increment of dev->connections inside the "if (netdef)" as well - dev can never be non-null if netdef is null (because "dev" is the pointer to a device in a network's pool of devices), so this doesn't have any functional effect, it just makes the code clearer. --- src/network/bridge_driver.c | 47 ++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 152bd06..3fb5ad3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3662,36 +3662,35 @@ validate: } } - if (dev) { - /* mark the allocation */ - dev->connections++; - if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV) { - VIR_DEBUG("Using physical device %s, %d connections", - dev->device.dev, dev->connections); - } else { - VIR_DEBUG("Using physical device %04x:%02x:%02x.%x, connections %d", - dev->device.pci.domain, dev->device.pci.bus, - dev->device.pci.slot, dev->device.pci.function, - dev->connections); - } - } - if (netdef) { netdef->connections++; VIR_DEBUG("Using network %s, %d connections", netdef->name, netdef->connections); - } - /* finally we can call the 'plugged' hook script if any */ - if (networkRunHook(network, dom, iface, - VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, - VIR_HOOK_SUBOP_BEGIN) < 0) { - /* adjust for failure */ - if (dev) - dev->connections--; - if (netdef) + if (dev) { + /* mark the allocation */ + dev->connections++; + if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV) { + VIR_DEBUG("Using physical device %s, %d connections", + dev->device.dev, dev->connections); + } else { + VIR_DEBUG("Using physical device %04x:%02x:%02x.%x, connections %d", + dev->device.pci.domain, dev->device.pci.bus, + dev->device.pci.slot, dev->device.pci.function, + dev->connections); + } + } + + /* finally we can call the 'plugged' hook script if any */ + if (networkRunHook(network, dom, iface, + VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, + VIR_HOOK_SUBOP_BEGIN) < 0) { + /* adjust for failure */ netdef->connections--; - goto error; + if (dev) + dev->connections--; + goto error; + } } ret = 0; -- 1.8.5.3
On 02/25/2014 08:46 AM, Laine Stump wrote:
networkAllocateActualDevice() is called for *all* interfaces, not just those with type='network'. In that case, it will jump down to its validate: label immediately, without allocating anything. After validation is done, two counters are potentially updated (one for the network, and one for any particular physical device that is chosen), and then networkRunHook() is called.
This patch refactors that code a slight bit so that networkRunHook() doesn't get called if netdef is NULL (i.e. type != network) and to place the conditional increment of dev->connections inside the "if (netdef)" as well - dev can never be non-null if netdef is null (because "dev" is the pointer to a device in a network's pool of devices), so this doesn't have any functional effect, it just makes the code clearer. --- src/network/bridge_driver.c | 47 ++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 24 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 25.02.2014 16:46, Laine Stump wrote:
networkAllocateActualDevice() is called for *all* interfaces, not just those with type='network'. In that case, it will jump down to its validate: label immediately, without allocating anything. After validation is done, two counters are potentially updated (one for the network, and one for any particular physical device that is chosen), and then networkRunHook() is called.
This patch refactors that code a slight bit so that networkRunHook() doesn't get called if netdef is NULL (i.e. type != network) and to place the conditional increment of dev->connections inside the "if (netdef)" as well - dev can never be non-null if netdef is null (because "dev" is the pointer to a device in a network's pool of devices), so this doesn't have any functional effect, it just makes the code clearer. --- src/network/bridge_driver.c | 47 ++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 152bd06..3fb5ad3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3662,36 +3662,35 @@ validate: } }
- if (dev) { - /* mark the allocation */ - dev->connections++; - if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV) { - VIR_DEBUG("Using physical device %s, %d connections", - dev->device.dev, dev->connections); - } else { - VIR_DEBUG("Using physical device %04x:%02x:%02x.%x, connections %d", - dev->device.pci.domain, dev->device.pci.bus, - dev->device.pci.slot, dev->device.pci.function, - dev->connections); - } - } - if (netdef) { netdef->connections++; VIR_DEBUG("Using network %s, %d connections", netdef->name, netdef->connections); - }
- /* finally we can call the 'plugged' hook script if any */ - if (networkRunHook(network, dom, iface, - VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, - VIR_HOOK_SUBOP_BEGIN) < 0) { - /* adjust for failure */ - if (dev) - dev->connections--; - if (netdef) + if (dev) { + /* mark the allocation */ + dev->connections++; + if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV) { + VIR_DEBUG("Using physical device %s, %d connections", + dev->device.dev, dev->connections); + } else { + VIR_DEBUG("Using physical device %04x:%02x:%02x.%x, connections %d", + dev->device.pci.domain, dev->device.pci.bus, + dev->device.pci.slot, dev->device.pci.function, + dev->connections); + } + } + + /* finally we can call the 'plugged' hook script if any */ + if (networkRunHook(network, dom, iface, + VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, + VIR_HOOK_SUBOP_BEGIN) < 0) { + /* adjust for failure */ netdef->connections--; - goto error; + if (dev) + dev->connections--; + goto error; + } }
ret = 0;
I thought this problem was solved by 83c404ff. I wonder what went wrong. Or maybe nothing is actually wrong, and this is merely just a refactoring. Michal
On 02/26/2014 12:42 PM, Michal Privoznik wrote:
On 25.02.2014 16:46, Laine Stump wrote:
[...] This patch refactors that code a slight bit so that networkRunHook() doesn't get called if netdef is NULL (i.e. type != network) and to place the conditional increment of dev->connections inside the "if (netdef)" as well - dev can never be non-null if netdef is null (because "dev" is the pointer to a device in a network's pool of devices), so this doesn't have any functional effect, it just makes the code clearer. ---
I thought this problem was solved by 83c404ff. I wonder what went wrong. Or maybe nothing is actually wrong, and this is merely just a refactoring.
Just refactoring to make it clearer what was happening (although it also makes that fix unnecessary); I coincidentally found it while working on the "unplug bandwidth and call networkRunHook only when appropriate" patch. I spent time figuring out exactly when dev and netdef would/wouldn't be NULL, and decided to make the code reflect my findings so that the next person to come along wouldn't need to go through the same investigation.
participants (3)
-
Eric Blake -
Laine Stump -
Michal Privoznik