8:13 a.m.
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
8:13 a.m.
New subject: [PATCH 1/5] bridge_driver: Set @dnsmasqStarted only after successful dnsmasq spawn
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(a)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
8:13 a.m.
New subject: [PATCH 2/5] bridge_driver: Use g_autoptr() for virMacMap
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(a)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
8:13 a.m.
New subject: [PATCH 3/5] virNetworkObjSetMacMap: take double pointer of @macmap
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(a)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
8:13 a.m.
New subject: [PATCH 4/5] bridge_driver: Introduce a helper for virNetworkObjSetMacMap()
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(a)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
8:13 a.m.
New subject: [PATCH 5/5] bridge_driver: Enable virMacMap iff dnsmasq is started
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(a)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
10:24 a.m.
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(a)redhat.com>
Jano
839
days inactive
839
days old
6 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik