[PATCH 0/5] bridge_driver: Enable virMacMap iff dnsmasq is started
See 5/5 for explanation. Michal Prívozník (5): bridge_driver: Set @dnsmasqStarted only after successful dnsmasq spawn bridge_driver: Use g_autoptr() for virMacMap virNetworkObjSetMacMap: take double pointer of @macmap bridge_driver: Introduce a helper for virNetworkObjSetMacMap() bridge_driver: Enable virMacMap iff dnsmasq is started src/conf/virnetworkobj.c | 4 +-- src/conf/virnetworkobj.h | 2 +- src/network/bridge_driver.c | 54 ++++++++++++++++++++----------------- 3 files changed, 32 insertions(+), 28 deletions(-) -- 2.35.1
The networkStartNetworkVirtual() function handles starting of networks of different forward types (none, nat, route, open). And as a part of startup process dnsmasq might be spawned but doesn't have to be (depending on the network configuration). The @dnsmasqStarted variable is supposed to track whether dnsmasq was started or not (so that it can be killed when starting network fails after it was started). But the variable is set even when the code decided not to start it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 11696a9459..7ad9f278a2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2052,11 +2052,12 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, /* start dnsmasq if there are any IP addresses (v4 or v6) */ - if ((v4present || v6present) && - networkStartDhcpDaemon(driver, obj) < 0) - goto error; + if (v4present || v6present) { + if (networkStartDhcpDaemon(driver, obj) < 0) + goto error; - dnsmasqStarted = true; + dnsmasqStarted = true; + } if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) goto error; -- 2.35.1
Let's annotate virMacMap variables in bridge_driver.c with g_autoptr() so that they are automatically freed upon error. This may look like a needless commit, since there's no memory leak currently, but it simplifies the next commit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7ad9f278a2..a07af55390 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -450,7 +450,7 @@ networkUpdateState(virNetworkObj *obj, virNetworkDef *def; virNetworkDriverState *driver = opaque; g_autoptr(dnsmasqCaps) dnsmasq_caps = networkGetDnsmasqCaps(driver); - virMacMap *macmap; + g_autoptr(virMacMap) macmap = NULL; g_autofree char *macMapFile = NULL; VIR_LOCK_GUARD lock = virObjectLockGuard(obj); @@ -476,6 +476,7 @@ networkUpdateState(virNetworkObj *obj, return -1; virNetworkObjSetMacMap(obj, macmap); + macmap = NULL; break; @@ -1938,7 +1939,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, virErrorPtr save_err = NULL; virNetworkIPDef *ipdef; virNetDevIPRoute *routedef; - virMacMap *macmap; + g_autoptr(virMacMap) macmap = NULL; g_autofree char *macMapFile = NULL; bool dnsmasqStarted = false; bool devOnline = false; -- 2.35.1
The virNetworkObjSetMacMap() API effectively steals passed @macmap argument. However, the argument is a plain, first order pointer. This requires every caller to set the argument to NULL after the function was called. Let's make the function take double pointer instead to make it obvious that the argument is consumed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virnetworkobj.c | 4 ++-- src/conf/virnetworkobj.h | 2 +- src/network/bridge_driver.c | 6 ++---- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 46b499db58..cc3b93db6d 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -240,9 +240,9 @@ virNetworkObjSetFloorSum(virNetworkObj *obj, void virNetworkObjSetMacMap(virNetworkObj *obj, - virMacMap *macmap) + virMacMap **macmap) { - obj->macmap = macmap; + obj->macmap = g_steal_pointer(macmap); } diff --git a/src/conf/virnetworkobj.h b/src/conf/virnetworkobj.h index fadd277cbd..7d34fa3204 100644 --- a/src/conf/virnetworkobj.h +++ b/src/conf/virnetworkobj.h @@ -78,7 +78,7 @@ virNetworkObjSetFloorSum(virNetworkObj *obj, void virNetworkObjSetMacMap(virNetworkObj *obj, - virMacMap *macmap); + virMacMap **macmap); void virNetworkObjUnrefMacMap(virNetworkObj *obj); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a07af55390..024487b359 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -475,8 +475,7 @@ networkUpdateState(virNetworkObj *obj, if (!(macmap = virMacMapNew(macMapFile))) return -1; - virNetworkObjSetMacMap(obj, macmap); - macmap = NULL; + virNetworkObjSetMacMap(obj, &macmap); break; @@ -1972,8 +1971,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, !(macmap = virMacMapNew(macMapFile))) goto error; - virNetworkObjSetMacMap(obj, macmap); - macmap = NULL; + virNetworkObjSetMacMap(obj, &macmap); /* Set bridge options */ -- 2.35.1
Currently, whenever virNetworkObjSetMacMap() is called the same pattern is used: 1) call virMacMapFileName() to generate a filename, 2) pass this filename to virMacMapNew(), and finally 3) pass retval from previous step to virNetworkObjSetMacMap(). Move this code into a helper (networkSetMacMap()) and replace both pattern occurrences with its call. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 024487b359..7098054f77 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -443,6 +443,24 @@ networkUpdatePort(virNetworkPortDef *port, return false; } +static int +networkSetMacMap(virNetworkDriverState *driver, + virNetworkObj *obj) +{ + virNetworkDef *def = virNetworkObjGetDef(obj); + g_autoptr(virMacMap) macmap = NULL; + g_autofree char *macMapFile = NULL; + + if (!(macMapFile = virMacMapFileName(driver->dnsmasqStateDir, + def->bridge))) + return -1; + if (!(macmap = virMacMapNew(macMapFile))) + return -1; + + virNetworkObjSetMacMap(obj, &macmap); + return 0; +} + static int networkUpdateState(virNetworkObj *obj, void *opaque) @@ -450,8 +468,6 @@ networkUpdateState(virNetworkObj *obj, virNetworkDef *def; virNetworkDriverState *driver = opaque; g_autoptr(dnsmasqCaps) dnsmasq_caps = networkGetDnsmasqCaps(driver); - g_autoptr(virMacMap) macmap = NULL; - g_autofree char *macMapFile = NULL; VIR_LOCK_GUARD lock = virObjectLockGuard(obj); if (!virNetworkObjIsActive(obj)) @@ -468,15 +484,9 @@ networkUpdateState(virNetworkObj *obj, if (!(def->bridge && virNetDevExists(def->bridge) == 1)) virNetworkObjSetActive(obj, false); - if (!(macMapFile = virMacMapFileName(driver->dnsmasqStateDir, - def->bridge))) + if (networkSetMacMap(driver, obj) < 0) return -1; - if (!(macmap = virMacMapNew(macMapFile))) - return -1; - - virNetworkObjSetMacMap(obj, &macmap); - break; case VIR_NETWORK_FORWARD_BRIDGE: @@ -1938,8 +1948,6 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, virErrorPtr save_err = NULL; virNetworkIPDef *ipdef; virNetDevIPRoute *routedef; - g_autoptr(virMacMap) macmap = NULL; - g_autofree char *macMapFile = NULL; bool dnsmasqStarted = false; bool devOnline = false; bool firewalRulesAdded = false; @@ -1966,13 +1974,9 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, if (virNetDevBridgeCreate(def->bridge, &def->mac) < 0) return -1; - if (!(macMapFile = virMacMapFileName(driver->dnsmasqStateDir, - def->bridge)) || - !(macmap = virMacMapNew(macMapFile))) + if (networkSetMacMap(driver, obj) < 0) goto error; - virNetworkObjSetMacMap(obj, &macmap); - /* Set bridge options */ if (def->mtu && virNetDevSetMTU(def->bridge, def->mtu) < 0) -- 2.35.1
The virMacMap module is used only for libvirt_guests NSS module as it records list of MAC addresses used by certain guest. But the module itself is usable if and only if the network assigns IP addresses (i.e. has dnsmasq running). If it's some other authority that assigns IP addresses then we do not need the virMacMap module at all. For instance, a network with no <forward/> type and no DHCP set won't create /var/lib/libvirt/dnsmasq/ dir which is what the module expects to exist. But there's no need for the module to even care about such network. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/348 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7098054f77..f6538d2638 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -484,9 +484,6 @@ networkUpdateState(virNetworkObj *obj, if (!(def->bridge && virNetDevExists(def->bridge) == 1)) virNetworkObjSetActive(obj, false); - if (networkSetMacMap(driver, obj) < 0) - return -1; - break; case VIR_NETWORK_FORWARD_BRIDGE: @@ -521,6 +518,9 @@ networkUpdateState(virNetworkObj *obj, if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { pid_t dnsmasqPid; + if (networkSetMacMap(driver, obj) < 0) + return -1; + ignore_value(virPidFileReadIfAlive(driver->pidDir, def->name, &dnsmasqPid, @@ -1974,9 +1974,6 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, if (virNetDevBridgeCreate(def->bridge, &def->mac) < 0) return -1; - if (networkSetMacMap(driver, obj) < 0) - goto error; - /* Set bridge options */ if (def->mtu && virNetDevSetMTU(def->bridge, def->mtu) < 0) @@ -2056,6 +2053,9 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, /* start dnsmasq if there are any IP addresses (v4 or v6) */ if (v4present || v6present) { + if (networkSetMacMap(driver, obj) < 0) + goto error; + if (networkStartDhcpDaemon(driver, obj) < 0) goto error; -- 2.35.1
On a Tuesday in 2022, Michal Privoznik wrote:
See 5/5 for explanation.
Michal Prívozník (5): bridge_driver: Set @dnsmasqStarted only after successful dnsmasq spawn bridge_driver: Use g_autoptr() for virMacMap virNetworkObjSetMacMap: take double pointer of @macmap bridge_driver: Introduce a helper for virNetworkObjSetMacMap() bridge_driver: Enable virMacMap iff dnsmasq is started
src/conf/virnetworkobj.c | 4 +-- src/conf/virnetworkobj.h | 2 +- src/network/bridge_driver.c | 54 ++++++++++++++++++++----------------- 3 files changed, 32 insertions(+), 28 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik