src/network/bridge_driver.c | 20 +++++++++++++++++--- src/network/bridge_driver_conf.h | 9 ++++++++- 2 files changed, 25 insertions(+), 4 deletions(-)
When the daemons were split out from the monolithic libvirtd, the
network driver didn't implement "inhibit idle timeout if there are any
active objects" as was done for other drivers, so virtnetworkd would
always exit after 120 seconds of no incoming connections. This didn't
every cause any visible problem, although it did mean that anytime a
network API was called after an idle time > 120 seconds, that the
restarting virtnetworkd would flush and reload all the
iptables/nftables rules for any active networks.
This patch replicates what is done in the QEMU driver - an nactive is
added to the network driver object, along with an inhibitCallback; the
latter is passed into networkStateInitialize when the driver is
loaded, and the former is incremented for each already-active network,
then incremented/decremented each time a network is started or
stopped. If nactive transitions from 0 to 1 or 1 to 0, inhibitCallback
is called, and it "does the right stuff" to prevent/enable the idle
timeout.
Signed-off-by: Laine Stump <laine@redhat.com>
---
I had made this patch as a part of a larger series that will require
it, but haven't sent that yet and keep being annoyed when virtnetworkd
exits out from under a gdb session, so I decided it has enough general
usefulness to send by itself.
src/network/bridge_driver.c | 20 +++++++++++++++++---
src/network/bridge_driver_conf.h | 9 ++++++++-
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 74ba59b4e9..6a48516fdf 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -501,9 +501,13 @@ networkUpdateState(virNetworkObj *obj,
return -1;
}
- if (virNetworkObjIsActive(obj))
+ if (virNetworkObjIsActive(obj)) {
virNetworkObjPortForEach(obj, networkUpdatePort, obj);
+ if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback)
+ driver->inhibitCallback(true, driver->inhibitOpaque);
+ }
+
/* Try and read dnsmasq pids of both active and inactive networks, just in
* case a network became inactive and we need to clean up. */
if (def->ips && (def->nips > 0)) {
@@ -617,8 +621,8 @@ static virDrvStateInitResult
networkStateInitialize(bool privileged,
const char *root,
bool monolithic G_GNUC_UNUSED,
- virStateInhibitCallback callback G_GNUC_UNUSED,
- void *opaque G_GNUC_UNUSED)
+ virStateInhibitCallback callback,
+ void *opaque)
{
virNetworkDriverConfig *cfg;
bool autostart = true;
@@ -640,6 +644,9 @@ networkStateInitialize(bool privileged,
goto error;
}
+ network_driver->inhibitCallback = callback;
+ network_driver->inhibitOpaque = opaque;
+
network_driver->privileged = privileged;
if (!(network_driver->xmlopt = networkDnsmasqCreateXMLConf()))
@@ -2427,6 +2434,9 @@ networkStartNetwork(virNetworkDriverState *driver,
obj, network_driver->xmlopt) < 0)
goto cleanup;
+ if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback)
+ driver->inhibitCallback(true, driver->inhibitOpaque);
+
virNetworkObjSetActive(obj, true);
VIR_INFO("Network '%s' started up", def->name);
ret = 0;
@@ -2500,6 +2510,10 @@ networkShutdownNetwork(virNetworkDriverState *driver,
VIR_HOOK_SUBOP_END);
virNetworkObjSetActive(obj, false);
+
+ if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback)
+ driver->inhibitCallback(false, driver->inhibitOpaque);
+
virNetworkObjUnsetDefTransient(obj);
return ret;
}
diff --git a/src/network/bridge_driver_conf.h b/src/network/bridge_driver_conf.h
index 8f221f391e..1beed01efb 100644
--- a/src/network/bridge_driver_conf.h
+++ b/src/network/bridge_driver_conf.h
@@ -21,7 +21,7 @@
#pragma once
-#include "internal.h"
+#include "libvirt_internal.h"
#include "virthread.h"
#include "virdnsmasq.h"
#include "virnetworkobj.h"
@@ -49,6 +49,13 @@ typedef struct _virNetworkDriverState virNetworkDriverState;
struct _virNetworkDriverState {
virMutex lock;
+ /* Atomic inc/dec only */
+ unsigned int nactive;
+
+ /* Immutable pointers. Caller must provide locking */
+ virStateInhibitCallback inhibitCallback;
+ void *inhibitOpaque;
+
/* Read-only */
bool privileged;
--
2.46.1On Tue, Oct 08, 2024 at 10:57:53AM -0400, Laine Stump wrote: > When the daemons were split out from the monolithic libvirtd, the > network driver didn't implement "inhibit idle timeout if there are any > active objects" as was done for other drivers, so virtnetworkd would > always exit after 120 seconds of no incoming connections. This didn't > every cause any visible problem, although it did mean that anytime a > network API was called after an idle time > 120 seconds, that the > restarting virtnetworkd would flush and reload all the > iptables/nftables rules for any active networks. My bad. When I introduced auto-timeouts (even to libvirtd), I looked at the network driver and thought there was no reason to stop it shutting down as the network could keep working just fine without libvirtd/virtnetworkd running. I totally forgot that we flush and reload firewall rules on every startup /facepalm > > This patch replicates what is done in the QEMU driver - an nactive is > added to the network driver object, along with an inhibitCallback; the > latter is passed into networkStateInitialize when the driver is > loaded, and the former is incremented for each already-active network, > then incremented/decremented each time a network is started or > stopped. If nactive transitions from 0 to 1 or 1 to 0, inhibitCallback > is called, and it "does the right stuff" to prevent/enable the idle > timeout. > > Signed-off-by: Laine Stump <laine@redhat.com> > --- > > I had made this patch as a part of a larger series that will require > it, but haven't sent that yet and keep being annoyed when virtnetworkd > exits out from under a gdb session, so I decided it has enough general > usefulness to send by itself. > > src/network/bridge_driver.c | 20 +++++++++++++++++--- > src/network/bridge_driver_conf.h | 9 ++++++++- > 2 files changed, 25 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With 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 :|
On Tue, Oct 08, 2024 at 10:57:53AM -0400, Laine Stump wrote: >When the daemons were split out from the monolithic libvirtd, the >network driver didn't implement "inhibit idle timeout if there are any >active objects" as was done for other drivers, so virtnetworkd would >always exit after 120 seconds of no incoming connections. This didn't >every cause any visible problem, although it did mean that anytime a >network API was called after an idle time > 120 seconds, that the >restarting virtnetworkd would flush and reload all the >iptables/nftables rules for any active networks. > >This patch replicates what is done in the QEMU driver - an nactive is >added to the network driver object, along with an inhibitCallback; the >latter is passed into networkStateInitialize when the driver is >loaded, and the former is incremented for each already-active network, >then incremented/decremented each time a network is started or >stopped. If nactive transitions from 0 to 1 or 1 to 0, inhibitCallback >is called, and it "does the right stuff" to prevent/enable the idle >timeout. > >Signed-off-by: Laine Stump <laine@redhat.com> >--- > >I had made this patch as a part of a larger series that will require >it, but haven't sent that yet and keep being annoyed when virtnetworkd >exits out from under a gdb session, so I decided it has enough general >usefulness to send by itself. > Keep in mind that it still needs to successfully survive the restart without changing anything even once this is merged. > src/network/bridge_driver.c | 20 +++++++++++++++++--- > src/network/bridge_driver_conf.h | 9 ++++++++- > 2 files changed, 25 insertions(+), 4 deletions(-) > >diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >index 74ba59b4e9..6a48516fdf 100644 >--- a/src/network/bridge_driver.c >+++ b/src/network/bridge_driver.c >@@ -2500,6 +2510,10 @@ networkShutdownNetwork(virNetworkDriverState *driver, > VIR_HOOK_SUBOP_END); > > virNetworkObjSetActive(obj, false); >+ >+ if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) pointless "!!", you're not doing any arithmetic with it Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
On 10/10/24 7:48 AM, Martin Kletzander wrote:
> On Tue, Oct 08, 2024 at 10:57:53AM -0400, Laine Stump wrote:
>> When the daemons were split out from the monolithic libvirtd, the
>> network driver didn't implement "inhibit idle timeout if there are any
>> active objects" as was done for other drivers, so virtnetworkd would
>> always exit after 120 seconds of no incoming connections. This didn't
>> every cause any visible problem, although it did mean that anytime a
>> network API was called after an idle time > 120 seconds, that the
>> restarting virtnetworkd would flush and reload all the
>> iptables/nftables rules for any active networks.
>>
>> This patch replicates what is done in the QEMU driver - an nactive is
>> added to the network driver object, along with an inhibitCallback; the
>> latter is passed into networkStateInitialize when the driver is
>> loaded, and the former is incremented for each already-active network,
>> then incremented/decremented each time a network is started or
>> stopped. If nactive transitions from 0 to 1 or 1 to 0, inhibitCallback
>> is called, and it "does the right stuff" to prevent/enable the idle
>> timeout.
>>
>> Signed-off-by: Laine Stump <laine@redhat.com>
>> ---
>>
>> I had made this patch as a part of a larger series that will require
>> it, but haven't sent that yet and keep being annoyed when virtnetworkd
>> exits out from under a gdb session, so I decided it has enough general
>> usefulness to send by itself.
>>
>
> Keep in mind that it still needs to successfully survive the restart
> without changing anything even once this is merged.
You mean daemon restart? Yeah, nothing has been (or will be) done to
break that, since we need to be able to force restarts during a software
update.
>
>> src/network/bridge_driver.c | 20 +++++++++++++++++---
>> src/network/bridge_driver_conf.h | 9 ++++++++-
>> 2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index 74ba59b4e9..6a48516fdf 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -2500,6 +2510,10 @@ networkShutdownNetwork(virNetworkDriverState
>> *driver,
>> VIR_HOOK_SUBOP_END);
>>
>> virNetworkObjSetActive(obj, false);
>> +
>> + if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver-
>> >inhibitCallback)
>
> pointless "!!", you're not doing any arithmetic with it
Good point - that's what I get for blindly copying what was done in QEMU
without thinking about it too much!
(For those interested in the origin, I looked it up and it turns out
that all other cases of "!!g_atomic_int_dec_and_test()" we added in a
commit that mechanically eliminated invocations of this macro:
#define virAtomicIntDecAndTest(i) (!!g_atomic_int_dec_and_test(i))
with its contents. This even led to several instances of:
ignore_value(!!g_atomic_int_dec_and_test(...));
I'm removing my usage of !! before pushing, but not touching the others.)
>
> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Thanks!
On a Thursday in 2024, Laine Stump wrote: >>>diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >>>index 74ba59b4e9..6a48516fdf 100644 >>>--- a/src/network/bridge_driver.c >>>+++ b/src/network/bridge_driver.c >>>@@ -2500,6 +2510,10 @@ >>>networkShutdownNetwork(virNetworkDriverState *driver, >>> VIR_HOOK_SUBOP_END); >>> >>> virNetworkObjSetActive(obj, false); >>>+ >>>+ if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver- >>>>inhibitCallback) >> >>pointless "!!", you're not doing any arithmetic with it > >Good point - that's what I get for blindly copying what was done in >QEMU without thinking about it too much! > >(For those interested in the origin, I looked it up and it turns out >that all other cases of "!!g_atomic_int_dec_and_test()" we added in a >commit that mechanically eliminated invocations of this macro: > > #define virAtomicIntDecAndTest(i) (!!g_atomic_int_dec_and_test(i)) > >with its contents. This even led to several instances of: > > ignore_value(!!g_atomic_int_dec_and_test(...)); > >I'm removing my usage of !! before pushing, but not touching the others.) > I touched the others. Jano >> >>Reviewed-by: Martin Kletzander <mkletzan@redhat.com> > >Thanks! >
© 2016 - 2026 Red Hat, Inc.